Skip to content

Add tracked command abstraction to bootstrap #126564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/bootstrap/src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@ use std::{
};

use bootstrap::{
find_recent_config_change_ids, human_readable_changes, t, Build, Config, Subcommand,
find_recent_config_change_ids, human_readable_changes, t, Build, Config, Context, Subcommand,
CONFIG_CHANGE_HISTORY,
};

fn main() {
let args = env::args().skip(1).collect::<Vec<_>>();
let config = Config::parse(&args);
let ctx = Context {};
let config = Config::parse(&ctx, &args);

let mut build_lock;
let _build_lock_guard;
Expand Down Expand Up @@ -76,7 +77,7 @@ fn main() {
let dump_bootstrap_shims = config.dump_bootstrap_shims;
let out_dir = config.out.clone();

Build::new(config).build();
Build::new(ctx, config).build();

if suggest_setup {
println!("WARNING: you have not made a `config.toml`");
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/build_steps/tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ pub fn prepare_tool_cargo(
cargo.env("CFG_VER_HASH", ver_hash);
}

let info = GitInfo::new(builder.config.omit_git_hash, &dir);
let info = GitInfo::new(&builder.ctx, builder.config.omit_git_hash, &dir);
if let Some(sha) = info.sha() {
cargo.env("CFG_COMMIT_HASH", sha);
}
Expand Down
13 changes: 9 additions & 4 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use serde::{Deserialize, Deserializer};
use serde_derive::Deserialize;

pub use crate::core::config::flags::Subcommand;
use crate::Context;
use build_helper::git::GitConfig;

macro_rules! check_ci_llvm {
Expand Down Expand Up @@ -1186,7 +1187,7 @@ impl Config {
}
}

pub fn parse(args: &[String]) -> Config {
pub fn parse(ctx: &Context, args: &[String]) -> Config {
#[cfg(test)]
fn get_toml(_: &Path) -> TomlConfig {
TomlConfig::default()
Expand Down Expand Up @@ -1216,10 +1217,14 @@ impl Config {
exit!(2);
})
}
Self::parse_inner(args, get_toml)
Self::parse_inner(ctx, args, get_toml)
}

pub(crate) fn parse_inner(args: &[String], get_toml: impl Fn(&Path) -> TomlConfig) -> Config {
pub(crate) fn parse_inner(
ctx: &Context,
args: &[String],
get_toml: impl Fn(&Path) -> TomlConfig,
) -> Config {
let mut flags = Flags::parse(args);
let mut config = Config::default_opts();

Expand Down Expand Up @@ -1716,7 +1721,7 @@ impl Config {
// rust_info must be set before is_ci_llvm_available() is called.
let default = config.channel == "dev";
config.omit_git_hash = omit_git_hash.unwrap_or(default);
config.rust_info = GitInfo::new(config.omit_git_hash, &config.src);
config.rust_info = GitInfo::new(ctx, config.omit_git_hash, &config.src);

if config.rust_info.is_from_tarball() && !is_user_configured_rust_channel {
ci_channel.clone_into(&mut config.channel);
Expand Down
7 changes: 4 additions & 3 deletions src/bootstrap/src/core/config/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use clap::{CommandFactory, Parser, ValueEnum};
use crate::core::build_steps::setup::Profile;
use crate::core::builder::{Builder, Kind};
use crate::core::config::{target_selection_list, Config, TargetSelectionList};
use crate::{Build, DocTests};
use crate::{Build, Context, DocTests};

#[derive(Copy, Clone, Default, Debug, ValueEnum)]
pub enum Color {
Expand Down Expand Up @@ -201,8 +201,9 @@ impl Flags {
HelpVerboseOnly::try_parse_from(it.clone())
{
println!("NOTE: updating submodules before printing available paths");
let config = Config::parse(&[String::from("build")]);
let build = Build::new(config);
let ctx = Context {};
let config = Config::parse(&ctx, &[String::from("build")]);
let build = Build::new(ctx, config);
let paths = Builder::get_help(&build, subcommand);
if let Some(s) = paths {
println!("{s}");
Expand Down
30 changes: 19 additions & 11 deletions src/bootstrap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use std::fs::{self, File};
use std::io;
use std::path::{Path, PathBuf};
use std::process::{Command, Output, Stdio};
use std::rc::Rc;
use std::str;
use std::sync::OnceLock;

Expand Down Expand Up @@ -118,6 +119,9 @@ const EXTRA_CHECK_CFGS: &[(Option<Mode>, &str, Option<&[&'static str]>)] = &[
(Some(Mode::ToolRustc), "windows_raw_dylib", None),
];

/// A global bootstrap context that tracks various things.
pub struct Context {}

/// A structure representing a Rust compiler.
///
/// Each compiler has a `stage` that it is associated with and a `host` that
Expand Down Expand Up @@ -159,6 +163,8 @@ pub struct Build {
/// User-specified configuration from `config.toml`.
config: Config,

ctx: Rc<Context>,

// Version information
version: String,

Expand Down Expand Up @@ -310,7 +316,7 @@ impl Build {
/// line and the filesystem `config`.
///
/// By default all build output will be placed in the current directory.
pub fn new(mut config: Config) -> Build {
pub fn new(ctx: Context, mut config: Config) -> Build {
let src = config.src.clone();
let out = config.out.clone();

Expand All @@ -332,15 +338,16 @@ impl Build {
let is_sudo = false;

let omit_git_hash = config.omit_git_hash;
let rust_info = GitInfo::new(omit_git_hash, &src);
let cargo_info = GitInfo::new(omit_git_hash, &src.join("src/tools/cargo"));
let rust_analyzer_info = GitInfo::new(omit_git_hash, &src.join("src/tools/rust-analyzer"));
let clippy_info = GitInfo::new(omit_git_hash, &src.join("src/tools/clippy"));
let miri_info = GitInfo::new(omit_git_hash, &src.join("src/tools/miri"));
let rustfmt_info = GitInfo::new(omit_git_hash, &src.join("src/tools/rustfmt"));
let rust_info = GitInfo::new(&ctx, omit_git_hash, &src);
let cargo_info = GitInfo::new(&ctx, omit_git_hash, &src.join("src/tools/cargo"));
let rust_analyzer_info =
GitInfo::new(&ctx, omit_git_hash, &src.join("src/tools/rust-analyzer"));
let clippy_info = GitInfo::new(&ctx, omit_git_hash, &src.join("src/tools/clippy"));
let miri_info = GitInfo::new(&ctx, omit_git_hash, &src.join("src/tools/miri"));
let rustfmt_info = GitInfo::new(&ctx, omit_git_hash, &src.join("src/tools/rustfmt"));

// we always try to use git for LLVM builds
let in_tree_llvm_info = GitInfo::new(false, &src.join("src/llvm-project"));
let in_tree_llvm_info = GitInfo::new(&ctx, false, &src.join("src/llvm-project"));

let initial_target_libdir_str = if config.dry_run() {
"/dummy/lib/path/to/lib/".to_string()
Expand Down Expand Up @@ -400,6 +407,7 @@ impl Build {
}

let mut build = Build {
ctx: Rc::new(ctx),
initial_rustc: config.initial_rustc.clone(),
initial_cargo: config.initial_cargo.clone(),
initial_lld,
Expand Down Expand Up @@ -514,7 +522,7 @@ impl Build {

// NOTE: The check for the empty directory is here because when running x.py the first time,
// the submodule won't be checked out. Check it out now so we can build it.
if !GitInfo::new(false, &absolute_path).is_managed_git_subrepository()
if !GitInfo::new(&self.ctx, false, &absolute_path).is_managed_git_subrepository()
&& !dir_is_empty(&absolute_path)
{
return;
Expand Down Expand Up @@ -622,7 +630,7 @@ impl Build {
// Sample output: `submodule.src/rust-installer.path src/tools/rust-installer`
let submodule = Path::new(line.split_once(' ').unwrap().1);
// Don't update the submodule unless it's already been cloned.
if GitInfo::new(false, submodule).is_managed_git_subrepository() {
if GitInfo::new(&self.ctx, false, submodule).is_managed_git_subrepository() {
self.update_submodule(submodule);
}
}
Expand All @@ -635,7 +643,7 @@ impl Build {
return;
}

if GitInfo::new(false, submodule).is_managed_git_subrepository() {
if GitInfo::new(&self.ctx, false, submodule).is_managed_git_subrepository() {
self.update_submodule(submodule);
}
}
Expand Down
30 changes: 14 additions & 16 deletions src/bootstrap/src/utils/channel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@
use std::fs;
use std::path::Path;

use crate::utils::helpers::{output, t};
use crate::Build;
use crate::utils::helpers::t;
use crate::{Build, Context};

use super::helpers;
use super::git;

#[derive(Clone, Default)]
pub enum GitInfo {
Expand All @@ -35,7 +35,7 @@ pub struct Info {
}

impl GitInfo {
pub fn new(omit_git_hash: bool, dir: &Path) -> GitInfo {
pub fn new(ctx: &Context, omit_git_hash: bool, dir: &Path) -> GitInfo {
// See if this even begins to look like a git dir
if !dir.join(".git").exists() {
match read_commit_info_file(dir) {
Expand All @@ -45,9 +45,8 @@ impl GitInfo {
}

// Make sure git commands work
match helpers::git(Some(dir)).arg("rev-parse").output() {
Ok(ref out) if out.status.success() => {}
_ => return GitInfo::Absent,
if git::cmd_works(dir).run_maybe(ctx).is_failure() {
return GitInfo::Absent;
}

// If we're ignoring the git info, we don't actually need to collect it, just make sure this
Expand All @@ -57,16 +56,15 @@ impl GitInfo {
}

// Ok, let's scrape some info
let ver_date = output(
helpers::git(Some(dir))
.arg("log")
.arg("-1")
.arg("--date=short")
.arg("--pretty=format:%cd"),
);
let ver_hash = output(helpers::git(Some(dir)).arg("rev-parse").arg("HEAD"));
let ver_date = git::cmd_git(Some(dir))
.arg("log")
.arg("-1")
.arg("--date=short")
.arg("--pretty=format:%cd")
.run_output(ctx);
let ver_hash = git::cmd_get_current_sha(dir).run_output(ctx);
let short_ver_hash =
output(helpers::git(Some(dir)).arg("rev-parse").arg("--short=9").arg("HEAD"));
git::cmd_git(Some(dir)).arg("rev-parse").arg("--short=9").arg("HEAD").run_output(ctx);
GitInfo::Present(Some(Info {
commit_date: ver_date.trim().to_string(),
sha: ver_hash.trim().to_string(),
Expand Down
94 changes: 94 additions & 0 deletions src/bootstrap/src/utils/cmd.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
use std::ffi::OsStr;
use std::path::Path;
use std::process::{Command, ExitStatus, Output};

use crate::Context;

/// Wrapper around `std::process::Command` whose execution will be eventually
/// tracked.
#[derive(Debug)]
pub struct TrackedCommand {
cmd: Command,
}

impl TrackedCommand {
pub fn new<P: AsRef<OsStr>>(program: P) -> Self {
Self { cmd: Command::new(program) }
}

pub fn current_dir<P: AsRef<Path>>(mut self, path: P) -> Self {
self.cmd.current_dir(path);
self
}

pub fn arg<A: AsRef<OsStr>>(mut self, arg: A) -> Self {
self.cmd.arg(arg);
self
}

/// Runs the command, ensuring that it has succeeded.
#[track_caller]
pub fn run(&mut self, ctx: &Context) -> CommandOutput {
let output = self.run_maybe(ctx);
if !output.is_success() {
panic!(
"`{:?}`\nwas supposed to succeed, but it failed with {}\nStdout: {}\nStderr: {}",
self.cmd,
output.status,
output.stdout(),
output.stderr()
)
}
output
}

/// Runs the command.
#[track_caller]
pub fn run_maybe(&mut self, _ctx: &Context) -> CommandOutput {
let output = self.cmd.output().expect("Cannot execute process");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: it might be nice to report the complete command invocation that failed

output.into()
}

/// Runs the command, ensuring that it has succeeded, and returns its stdout.
#[track_caller]
pub fn run_output(&mut self, ctx: &Context) -> String {
self.run(ctx).stdout()
}
}

/// Creates a new tracked command.
pub fn cmd<P: AsRef<OsStr>>(program: P) -> TrackedCommand {
TrackedCommand::new(program)
}

/// Represents the output of an executed process.
#[allow(unused)]
pub struct CommandOutput {
status: ExitStatus,
stdout: Vec<u8>,
stderr: Vec<u8>,
}

impl CommandOutput {
pub fn is_success(&self) -> bool {
self.status.success()
}

pub fn is_failure(&self) -> bool {
!self.is_success()
}

pub fn stdout(&self) -> String {
String::from_utf8(self.stdout.clone()).expect("Cannot parse process stdout as UTF-8")
}

pub fn stderr(&self) -> String {
String::from_utf8(self.stderr.clone()).expect("Cannot parse process stderr as UTF-8")
}
}

impl From<Output> for CommandOutput {
fn from(output: Output) -> Self {
Self { status: output.status, stdout: output.stdout, stderr: output.stderr }
}
}
20 changes: 20 additions & 0 deletions src/bootstrap/src/utils/git.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
use crate::utils::cmd::{cmd, TrackedCommand};
use std::path::Path;

/// Command that checks whether git works at the given `directory`.
pub fn cmd_works(directory: &Path) -> TrackedCommand {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_in_repo maybe instead?

cmd_git(Some(directory)).arg("rev-parse")
}

/// Command that finds the currently checked out SHA at the given `directory`.
pub fn cmd_get_current_sha(directory: &Path) -> TrackedCommand {
cmd_git(Some(directory)).arg("rev-parse").arg("HEAD")
}

pub fn cmd_git(directory: Option<&Path>) -> TrackedCommand {
let mut cmd = cmd("git");
if let Some(directory) = directory {
cmd = cmd.current_dir(directory);
}
cmd
}
2 changes: 2 additions & 0 deletions src/bootstrap/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub(crate) mod cache;
pub(crate) mod cc_detect;
pub(crate) mod change_tracker;
pub(crate) mod channel;
pub(crate) mod cmd;
pub(crate) mod dylib;
pub(crate) mod exec;
pub(crate) mod helpers;
Expand All @@ -14,3 +15,4 @@ pub(crate) mod job;
pub(crate) mod metrics;
pub(crate) mod render_tests;
pub(crate) mod tarball;
mod git;
Loading