Skip to content

Commit c1e3f03

Browse files
borsbors
committed
Auto merge of #127450 - Kobzol:bootstrap-cmd-refactor-5, r=onur-ozkan
Bootstrap command refactoring: improve debuggability (step 5) Continuation of #127321. This PR improves the debuggability of command execution, by improving the output logged when a command fails (it now includes the exact location where the command was created and where it was executed), and also by adding a "drop bomb", which will panic if a command was created, but not executed (which is probably a bug). Here is how the output of a failed command looks like: ``` Command "git" "foo" "[bar]" (failure_mode=Exit, stdout_mode=Capture, stderr_mode=Capture) did not execute successfully. Expected success, got exit status: 1 Created at: src/core/build_steps/compile.rs:1699:9 Executed at: src/core/build_steps/compile.rs:1699:58 STDOUT ---- STDERR ---- git: 'foo' is not a git command. See 'git --help'. ``` And this is what is printed if you forget to execute a command: ``` thread 'main' panicked at /projects/personal/rust/rust/src/tools/build_helper/src/drop_bomb/mod.rs:42:13: command constructed at `src/core/build_steps/compile.rs:1699:9` was dropped without being executed: `git` ``` Best reviewed commit by commit. Tracking issue: #126819 r? `@onur-ozkan`
2 parents 44fb857 + 0cce0bb commit c1e3f03

File tree

24 files changed

+193
-84
lines changed

24 files changed

+193
-84
lines changed

‎Cargo.lock

+1
Original file line numberDiff line numberDiff line change
@@ -3420,6 +3420,7 @@ version = "0.2.0"
34203420
dependencies = [
34213421
"ar",
34223422
"bstr",
3423+
"build_helper",
34233424
"gimli 0.28.1",
34243425
"object 0.34.0",
34253426
"regex",

‎src/bootstrap/src/core/build_steps/compile.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -2080,7 +2080,8 @@ pub fn stream_cargo(
20802080
tail_args: Vec<String>,
20812081
cb: &mut dyn FnMut(CargoMessage<'_>),
20822082
) -> bool {
2083-
let mut cargo = cargo.into_cmd().command;
2083+
let mut cmd = cargo.into_cmd();
2084+
let cargo = cmd.as_command_mut();
20842085
// Instruct Cargo to give us json messages on stdout, critically leaving
20852086
// stderr as piped so we can get those pretty colors.
20862087
let mut message_format = if builder.config.json_output {

‎src/bootstrap/src/core/build_steps/dist.rs

+2-10
Original file line numberDiff line numberDiff line change
@@ -1060,11 +1060,7 @@ impl Step for PlainSourceTarball {
10601060
cmd.arg("--sync").arg(manifest_path);
10611061
}
10621062

1063-
let config = if !builder.config.dry_run() {
1064-
cmd.capture().run(builder).stdout()
1065-
} else {
1066-
String::new()
1067-
};
1063+
let config = cmd.capture().run(builder).stdout();
10681064

10691065
let cargo_config_dir = plain_dst_src.join(".cargo");
10701066
builder.create_dir(&cargo_config_dir);
@@ -2072,11 +2068,7 @@ fn maybe_install_llvm(
20722068
let mut cmd = command(llvm_config);
20732069
cmd.arg("--libfiles");
20742070
builder.verbose(|| println!("running {cmd:?}"));
2075-
let files = if builder.config.dry_run() {
2076-
"".into()
2077-
} else {
2078-
cmd.capture_stdout().run(builder).stdout()
2079-
};
2071+
let files = cmd.capture_stdout().run(builder).stdout();
20802072
let build_llvm_out = &builder.llvm_out(builder.config.build);
20812073
let target_llvm_out = &builder.llvm_out(target);
20822074
for file in files.trim_end().split(' ') {

‎src/bootstrap/src/core/build_steps/doc.rs

+7-6
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,20 @@ impl<P: Step> Step for RustbookSrc<P> {
146146
let out = out.join(&name);
147147
let index = out.join("index.html");
148148
let rustbook = builder.tool_exe(Tool::Rustbook);
149-
let mut rustbook_cmd = builder.tool_cmd(Tool::Rustbook);
150149

151150
if !builder.config.dry_run()
152151
&& (!up_to_date(&src, &index) || !up_to_date(&rustbook, &index))
153152
{
154153
builder.info(&format!("Rustbook ({target}) - {name}"));
155154
let _ = fs::remove_dir_all(&out);
156155

157-
rustbook_cmd.arg("build").arg(&src).arg("-d").arg(&out).run(builder);
156+
builder
157+
.tool_cmd(Tool::Rustbook)
158+
.arg("build")
159+
.arg(&src)
160+
.arg("-d")
161+
.arg(&out)
162+
.run(builder);
158163

159164
for lang in &self.languages {
160165
let out = out.join(lang);
@@ -253,10 +258,6 @@ impl Step for TheBook {
253258
// build the version info page and CSS
254259
let shared_assets = builder.ensure(SharedAssets { target });
255260

256-
// build the command first so we don't nest GHA groups
257-
// FIXME: this doesn't do anything!
258-
builder.rustdoc_cmd(compiler);
259-
260261
// build the redirect pages
261262
let _guard = builder.msg_doc(compiler, "book redirect pages", target);
262263
for file in t!(fs::read_dir(redirect_path)) {

‎src/bootstrap/src/core/build_steps/llvm.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ pub(crate) fn detect_llvm_sha(config: &Config, is_git: bool) -> String {
172172
// the LLVM shared object file is named `LLVM-12-rust-{version}-nightly`
173173
config.src.join("src/version"),
174174
]);
175-
output(&mut rev_list.command).trim().to_owned()
175+
output(rev_list.as_command_mut()).trim().to_owned()
176176
} else if let Some(info) = channel::read_commit_info_file(&config.src) {
177177
info.sha.trim().to_owned()
178178
} else {
@@ -254,7 +254,7 @@ pub(crate) fn is_ci_llvm_modified(config: &Config) -> bool {
254254
// `true` here.
255255
let llvm_sha = detect_llvm_sha(config, true);
256256
let head_sha =
257-
output(&mut helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").command);
257+
output(helpers::git(Some(&config.src)).arg("rev-parse").arg("HEAD").as_command_mut());
258258
let head_sha = head_sha.trim();
259259
llvm_sha == head_sha
260260
}

‎src/bootstrap/src/core/build_steps/setup.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ impl Step for Hook {
484484
fn install_git_hook_maybe(config: &Config) -> io::Result<()> {
485485
let git = helpers::git(Some(&config.src))
486486
.args(["rev-parse", "--git-common-dir"])
487-
.command
487+
.as_command_mut()
488488
.output()
489489
.map(|output| {
490490
assert!(output.status.success(), "failed to run `git`");

‎src/bootstrap/src/core/build_steps/test.rs

+53-11
Original file line numberDiff line numberDiff line change
@@ -471,16 +471,12 @@ impl Miri {
471471
// We re-use the `cargo` from above.
472472
cargo.arg("--print-sysroot");
473473

474-
if builder.config.dry_run() {
475-
String::new()
476-
} else {
477-
builder.verbose(|| println!("running: {cargo:?}"));
478-
let stdout = cargo.capture_stdout().run(builder).stdout();
479-
// Output is "<sysroot>\n".
480-
let sysroot = stdout.trim_end();
481-
builder.verbose(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}"));
482-
sysroot.to_owned()
483-
}
474+
builder.verbose(|| println!("running: {cargo:?}"));
475+
let stdout = cargo.capture_stdout().run(builder).stdout();
476+
// Output is "<sysroot>\n".
477+
let sysroot = stdout.trim_end();
478+
builder.verbose(|| println!("`cargo miri setup --print-sysroot` said: {sysroot:?}"));
479+
sysroot.to_owned()
484480
}
485481
}
486482

@@ -1352,6 +1348,52 @@ impl Step for CrateRunMakeSupport {
13521348
}
13531349
}
13541350

1351+
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
1352+
pub struct CrateBuildHelper {
1353+
host: TargetSelection,
1354+
}
1355+
1356+
impl Step for CrateBuildHelper {
1357+
type Output = ();
1358+
const ONLY_HOSTS: bool = true;
1359+
1360+
fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
1361+
run.path("src/tools/build_helper")
1362+
}
1363+
1364+
fn make_run(run: RunConfig<'_>) {
1365+
run.builder.ensure(CrateBuildHelper { host: run.target });
1366+
}
1367+
1368+
/// Runs `cargo test` for build_helper.
1369+
fn run(self, builder: &Builder<'_>) {
1370+
let host = self.host;
1371+
let compiler = builder.compiler(0, host);
1372+
1373+
let mut cargo = tool::prepare_tool_cargo(
1374+
builder,
1375+
compiler,
1376+
Mode::ToolBootstrap,
1377+
host,
1378+
"test",
1379+
"src/tools/build_helper",
1380+
SourceType::InTree,
1381+
&[],
1382+
);
1383+
cargo.allow_features("test");
1384+
run_cargo_test(
1385+
cargo,
1386+
&[],
1387+
&[],
1388+
"build_helper",
1389+
"build_helper self test",
1390+
compiler,
1391+
host,
1392+
builder,
1393+
);
1394+
}
1395+
}
1396+
13551397
default_test!(Ui { path: "tests/ui", mode: "ui", suite: "ui" });
13561398

13571399
default_test!(Crashes { path: "tests/crashes", mode: "crashes", suite: "crashes" });
@@ -2058,7 +2100,7 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
20582100
cmd.arg("--nightly-branch").arg(git_config.nightly_branch);
20592101

20602102
// FIXME: Move CiEnv back to bootstrap, it is only used here anyway
2061-
builder.ci_env.force_coloring_in_ci(&mut cmd.command);
2103+
builder.ci_env.force_coloring_in_ci(cmd.as_command_mut());
20622104

20632105
#[cfg(feature = "build-metrics")]
20642106
builder.metrics.begin_test_suite(

‎src/bootstrap/src/core/build_steps/toolstate.rs

+14-9
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ fn check_changed_files(toolstates: &HashMap<Box<str>, ToolState>) {
106106
.arg("--name-status")
107107
.arg("HEAD")
108108
.arg("HEAD^")
109-
.command
109+
.as_command_mut()
110110
.output();
111111
let output = match output {
112112
Ok(o) => o,
@@ -329,7 +329,7 @@ fn checkout_toolstate_repo() {
329329
.arg("--depth=1")
330330
.arg(toolstate_repo())
331331
.arg(TOOLSTATE_DIR)
332-
.command
332+
.as_command_mut()
333333
.status();
334334
let success = match status {
335335
Ok(s) => s.success(),
@@ -343,8 +343,13 @@ fn checkout_toolstate_repo() {
343343
/// Sets up config and authentication for modifying the toolstate repo.
344344
fn prepare_toolstate_config(token: &str) {
345345
fn git_config(key: &str, value: &str) {
346-
let status =
347-
helpers::git(None).arg("config").arg("--global").arg(key).arg(value).command.status();
346+
let status = helpers::git(None)
347+
.arg("config")
348+
.arg("--global")
349+
.arg(key)
350+
.arg(value)
351+
.as_command_mut()
352+
.status();
348353
let success = match status {
349354
Ok(s) => s.success(),
350355
Err(_) => false,
@@ -413,7 +418,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
413418
.arg("-a")
414419
.arg("-m")
415420
.arg(&message)
416-
.command
421+
.as_command_mut()
417422
.status());
418423
if !status.success() {
419424
success = true;
@@ -424,7 +429,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
424429
.arg("push")
425430
.arg("origin")
426431
.arg("master")
427-
.command
432+
.as_command_mut()
428433
.status());
429434
// If we successfully push, exit.
430435
if status.success() {
@@ -437,14 +442,14 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
437442
.arg("fetch")
438443
.arg("origin")
439444
.arg("master")
440-
.command
445+
.as_command_mut()
441446
.status());
442447
assert!(status.success());
443448
let status = t!(helpers::git(Some(Path::new(TOOLSTATE_DIR)))
444449
.arg("reset")
445450
.arg("--hard")
446451
.arg("origin/master")
447-
.command
452+
.as_command_mut()
448453
.status());
449454
assert!(status.success());
450455
}
@@ -460,7 +465,7 @@ fn commit_toolstate_change(current_toolstate: &ToolstateData) {
460465
/// `publish_toolstate.py` script if the PR passes all tests and is merged to
461466
/// master.
462467
fn publish_test_results(current_toolstate: &ToolstateData) {
463-
let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").command.output());
468+
let commit = t!(helpers::git(None).arg("rev-parse").arg("HEAD").as_command_mut().output());
464469
let commit = t!(String::from_utf8(commit.stdout));
465470

466471
let toolstate_serialized = t!(serde_json::to_string(&current_toolstate));

‎src/bootstrap/src/core/builder.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -860,6 +860,7 @@ impl<'a> Builder<'a> {
860860
test::Clippy,
861861
test::CompiletestTest,
862862
test::CrateRunMakeSupport,
863+
test::CrateBuildHelper,
863864
test::RustdocJSStd,
864865
test::RustdocJSNotStd,
865866
test::RustdocGUI,
@@ -2104,7 +2105,7 @@ impl<'a> Builder<'a> {
21042105
// Try to use a sysroot-relative bindir, in case it was configured absolutely.
21052106
cargo.env("RUSTC_INSTALL_BINDIR", self.config.bindir_relative());
21062107

2107-
self.ci_env.force_coloring_in_ci(&mut cargo.command);
2108+
self.ci_env.force_coloring_in_ci(cargo.as_command_mut());
21082109

21092110
// When we build Rust dylibs they're all intended for intermediate
21102111
// usage, so make sure we pass the -Cprefer-dynamic flag instead of

‎src/bootstrap/src/core/config/config.rs

+8-8
Original file line numberDiff line numberDiff line change
@@ -1259,7 +1259,7 @@ impl Config {
12591259
cmd.arg("rev-parse").arg("--show-cdup");
12601260
// Discard stderr because we expect this to fail when building from a tarball.
12611261
let output = cmd
1262-
.command
1262+
.as_command_mut()
12631263
.stderr(std::process::Stdio::null())
12641264
.output()
12651265
.ok()
@@ -2163,7 +2163,7 @@ impl Config {
21632163

21642164
let mut git = helpers::git(Some(&self.src));
21652165
git.arg("show").arg(format!("{commit}:{}", file.to_str().unwrap()));
2166-
output(&mut git.command)
2166+
output(git.as_command_mut())
21672167
}
21682168

21692169
/// Bootstrap embeds a version number into the name of shared libraries it uploads in CI.
@@ -2469,11 +2469,11 @@ impl Config {
24692469
// Look for a version to compare to based on the current commit.
24702470
// Only commits merged by bors will have CI artifacts.
24712471
let merge_base = output(
2472-
&mut helpers::git(Some(&self.src))
2472+
helpers::git(Some(&self.src))
24732473
.arg("rev-list")
24742474
.arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
24752475
.args(["-n1", "--first-parent", "HEAD"])
2476-
.command,
2476+
.as_command_mut(),
24772477
);
24782478
let commit = merge_base.trim_end();
24792479
if commit.is_empty() {
@@ -2489,7 +2489,7 @@ impl Config {
24892489
.args(["diff-index", "--quiet", commit])
24902490
.arg("--")
24912491
.args([self.src.join("compiler"), self.src.join("library")])
2492-
.command
2492+
.as_command_mut()
24932493
.status())
24942494
.success();
24952495
if has_changes {
@@ -2563,11 +2563,11 @@ impl Config {
25632563
// Look for a version to compare to based on the current commit.
25642564
// Only commits merged by bors will have CI artifacts.
25652565
let merge_base = output(
2566-
&mut helpers::git(Some(&self.src))
2566+
helpers::git(Some(&self.src))
25672567
.arg("rev-list")
25682568
.arg(format!("--author={}", self.stage0_metadata.config.git_merge_commit_email))
25692569
.args(["-n1", "--first-parent", "HEAD"])
2570-
.command,
2570+
.as_command_mut(),
25712571
);
25722572
let commit = merge_base.trim_end();
25732573
if commit.is_empty() {
@@ -2589,7 +2589,7 @@ impl Config {
25892589
git.arg(top_level.join(path));
25902590
}
25912591

2592-
let has_changes = !t!(git.command.status()).success();
2592+
let has_changes = !t!(git.as_command_mut().status()).success();
25932593
if has_changes {
25942594
if if_unchanged {
25952595
if self.verbose > 0 {

‎src/bootstrap/src/core/sanity.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ than building it.
209209

210210
#[cfg(not(feature = "bootstrap-self-test"))]
211211
let stage0_supported_target_list: HashSet<String> = crate::utils::helpers::output(
212-
&mut command(&build.config.initial_rustc).args(["--print", "target-list"]).command,
212+
command(&build.config.initial_rustc).args(["--print", "target-list"]).as_command_mut(),
213213
)
214214
.lines()
215215
.map(|s| s.to_string())

0 commit comments

Comments
 (0)