Skip to content

Commit 9e82096

Browse files
borsbors
committed
Auto merge of #127799 - Kobzol:bootstrap-cmd-refactor-7, r=onur-ozkan
Bootstrap command refactoring: make command output API more bulletproof (step 7) Continuation of #127680. This PR modifies the API of running commands to make it more explicit when a command is expected to produce programmatically handled output. Now if you call just `run`, you cannot access the stdout/stderr by accident, because it will not be returned to the caller. This API change might be seen as overkill, let me know what do you think. In any case, I'd like to land the second commit, to make it harder to accidentally read stdout/stderr of commands that did not capture output (now you'd get an empty string as a result, but you should probably get a panic instead, if you try to read uncaptured stdout/stderr). Tracking issue: #126819 r? `@onur-ozkan`
2 parents 8ded134 + 1984a46 commit 9e82096

19 files changed

+152
-165
lines changed

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -1481,7 +1481,7 @@ pub fn compiler_file(
14811481
let mut cmd = command(compiler);
14821482
cmd.args(builder.cflags(target, GitRepo::Rustc, c));
14831483
cmd.arg(format!("-print-file-name={file}"));
1484-
let out = cmd.capture_stdout().run(builder).stdout();
1484+
let out = cmd.run_capture_stdout(builder).stdout();
14851485
PathBuf::from(out.trim())
14861486
}
14871487

@@ -1844,7 +1844,7 @@ impl Step for Assemble {
18441844
builder.ensure(llvm::Llvm { target: target_compiler.host });
18451845
if !builder.config.dry_run() && builder.config.llvm_tools_enabled {
18461846
let llvm_bin_dir =
1847-
command(llvm_config).capture_stdout().arg("--bindir").run(builder).stdout();
1847+
command(llvm_config).arg("--bindir").run_capture_stdout(builder).stdout();
18481848
let llvm_bin_dir = Path::new(llvm_bin_dir.trim());
18491849

18501850
// Since we've already built the LLVM tools, install them to the sysroot.
@@ -2170,7 +2170,7 @@ pub fn strip_debug(builder: &Builder<'_>, target: TargetSelection, path: &Path)
21702170
}
21712171

21722172
let previous_mtime = t!(t!(path.metadata()).modified());
2173-
command("strip").capture().arg("--strip-debug").arg(path).run(builder);
2173+
command("strip").arg("--strip-debug").arg(path).run_capture(builder);
21742174

21752175
let file = t!(fs::File::open(path));
21762176

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ fn make_win_dist(
182182
//Ask gcc where it keeps its stuff
183183
let mut cmd = command(builder.cc(target));
184184
cmd.arg("-print-search-dirs");
185-
let gcc_out = cmd.capture_stdout().run(builder).stdout();
185+
let gcc_out = cmd.run_capture_stdout(builder).stdout();
186186

187187
let mut bin_path: Vec<_> = env::split_paths(&env::var_os("PATH").unwrap_or_default()).collect();
188188
let mut lib_path = Vec::new();
@@ -1062,7 +1062,7 @@ impl Step for PlainSourceTarball {
10621062
cmd.arg("--sync").arg(manifest_path);
10631063
}
10641064

1065-
let config = cmd.capture().run(builder).stdout();
1065+
let config = cmd.run_capture(builder).stdout();
10661066

10671067
let cargo_config_dir = plain_dst_src.join(".cargo");
10681068
builder.create_dir(&cargo_config_dir);
@@ -2070,7 +2070,7 @@ fn maybe_install_llvm(
20702070
let mut cmd = command(llvm_config);
20712071
cmd.arg("--libfiles");
20722072
builder.verbose(|| println!("running {cmd:?}"));
2073-
let files = cmd.capture_stdout().run(builder).stdout();
2073+
let files = cmd.run_capture_stdout(builder).stdout();
20742074
let build_llvm_out = &builder.llvm_out(builder.config.build);
20752075
let target_llvm_out = &builder.llvm_out(target);
20762076
for file in files.trim_end().split(' ') {

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

+4-6
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ fn get_rustfmt_version(build: &Builder<'_>) -> Option<(String, PathBuf)> {
6060
});
6161
cmd.arg("--version");
6262

63-
let output = cmd.capture().allow_failure().run(build);
63+
let output = cmd.allow_failure().run_capture(build);
6464
if output.is_failure() {
6565
return None;
6666
}
@@ -160,25 +160,23 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
160160
}
161161
}
162162
let git_available =
163-
helpers::git(None).capture().allow_failure().arg("--version").run(build).is_success();
163+
helpers::git(None).allow_failure().arg("--version").run_capture(build).is_success();
164164

165165
let mut adjective = None;
166166
if git_available {
167167
let in_working_tree = helpers::git(Some(&build.src))
168-
.capture()
169168
.allow_failure()
170169
.arg("rev-parse")
171170
.arg("--is-inside-work-tree")
172-
.run(build)
171+
.run_capture(build)
173172
.is_success();
174173
if in_working_tree {
175174
let untracked_paths_output = helpers::git(Some(&build.src))
176-
.capture_stdout()
177175
.arg("status")
178176
.arg("--porcelain")
179177
.arg("-z")
180178
.arg("--untracked-files=normal")
181-
.run(build)
179+
.run_capture_stdout(build)
182180
.stdout();
183181
let untracked_paths: Vec<_> = untracked_paths_output
184182
.split_terminator('\0')

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -471,7 +471,7 @@ impl Step for Llvm {
471471
builder.ensure(Llvm { target: builder.config.build });
472472
if !builder.config.dry_run() {
473473
let llvm_bindir =
474-
command(&llvm_config).capture_stdout().arg("--bindir").run(builder).stdout();
474+
command(&llvm_config).arg("--bindir").run_capture_stdout(builder).stdout();
475475
let host_bin = Path::new(llvm_bindir.trim());
476476
cfg.define(
477477
"LLVM_TABLEGEN",
@@ -522,7 +522,7 @@ impl Step for Llvm {
522522
// Helper to find the name of LLVM's shared library on darwin and linux.
523523
let find_llvm_lib_name = |extension| {
524524
let version =
525-
command(&res.llvm_config).capture_stdout().arg("--version").run(builder).stdout();
525+
command(&res.llvm_config).arg("--version").run_capture_stdout(builder).stdout();
526526
let major = version.split('.').next().unwrap();
527527

528528
match &llvm_version_suffix {
@@ -578,7 +578,7 @@ fn check_llvm_version(builder: &Builder<'_>, llvm_config: &Path) {
578578
return;
579579
}
580580

581-
let version = command(llvm_config).capture_stdout().arg("--version").run(builder).stdout();
581+
let version = command(llvm_config).arg("--version").run_capture_stdout(builder).stdout();
582582
let mut parts = version.split('.').take(2).filter_map(|s| s.parse::<u32>().ok());
583583
if let (Some(major), Some(_minor)) = (parts.next(), parts.next()) {
584584
if major >= 17 {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ impl Step for BuildManifest {
4040
panic!("\n\nfailed to specify `dist.upload-addr` in `config.toml`\n\n")
4141
});
4242

43-
let today = command("date").capture_stdout().arg("+%Y-%m-%d").run(builder).stdout();
43+
let today = command("date").arg("+%Y-%m-%d").run_capture_stdout(builder).stdout();
4444

4545
cmd.arg(sign);
4646
cmd.arg(distdir(builder));

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

+4-7
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ impl Step for Link {
275275
}
276276

277277
fn rustup_installed(builder: &Builder<'_>) -> bool {
278-
command("rustup").capture_stdout().arg("--version").run(builder).is_success()
278+
command("rustup").arg("--version").run_capture_stdout(builder).is_success()
279279
}
280280

281281
fn stage_dir_exists(stage_path: &str) -> bool {
@@ -313,10 +313,9 @@ fn attempt_toolchain_link(builder: &Builder<'_>, stage_path: &str) {
313313

314314
fn toolchain_is_linked(builder: &Builder<'_>) -> bool {
315315
match command("rustup")
316-
.capture_stdout()
317316
.allow_failure()
318317
.args(["toolchain", "list"])
319-
.run(builder)
318+
.run_capture_stdout(builder)
320319
.stdout_if_ok()
321320
{
322321
Some(toolchain_list) => {
@@ -341,9 +340,8 @@ fn toolchain_is_linked(builder: &Builder<'_>) -> bool {
341340

342341
fn try_link_toolchain(builder: &Builder<'_>, stage_path: &str) -> bool {
343342
command("rustup")
344-
.capture_stdout()
345343
.args(["toolchain", "link", "stage1", stage_path])
346-
.run(builder)
344+
.run_capture_stdout(builder)
347345
.is_success()
348346
}
349347

@@ -481,9 +479,8 @@ impl Step for Hook {
481479
// install a git hook to automatically run tidy, if they want
482480
fn install_git_hook_maybe(builder: &Builder<'_>, config: &Config) -> io::Result<()> {
483481
let git = helpers::git(Some(&config.src))
484-
.capture()
485482
.args(["rev-parse", "--git-common-dir"])
486-
.run(builder)
483+
.run_capture(builder)
487484
.stdout();
488485
let git = PathBuf::from(git.trim());
489486
let hooks_dir = git.join("hooks");

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,9 @@ pub fn suggest(builder: &Builder<'_>, run: bool) {
1414
let git_config = builder.config.git_config();
1515
let suggestions = builder
1616
.tool_cmd(Tool::SuggestTests)
17-
.capture_stdout()
1817
.env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository)
1918
.env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch)
20-
.run(builder)
19+
.run_capture_stdout(builder)
2120
.stdout();
2221

2322
let suggestions = suggestions

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ fn create_synthetic_target(
6464
// we cannot use nightly features. So `RUSTC_BOOTSTRAP` is needed here.
6565
cmd.env("RUSTC_BOOTSTRAP", "1");
6666

67-
let output = cmd.capture().run(builder).stdout();
67+
let output = cmd.run_capture(builder).stdout();
6868
let mut spec: serde_json::Value = serde_json::from_slice(output.as_bytes()).unwrap();
6969
let spec_map = spec.as_object_mut().unwrap();
7070

0 commit comments

Comments
 (0)