Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bootstrap command refactoring: port more Command usages to BootstrapCmd (step 2) #126822

Merged
merged 11 commits into from
Jun 29, 2024
Merged

Bootstrap command refactoring: port more Command usages to BootstrapCmd (step 2) #126822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
903d6a9
Remove `run_cmd`
Kobzol Jun 22, 2024
cf5bbb3
Remove `run` and rename `run_tracked` to `run`
Kobzol Jun 22, 2024
31911e5
Improve documentation of `BootstrapCommand`
Kobzol Jun 22, 2024
3722fb5
Store `Command` directly inside `BootstrapCommand`
Kobzol Jun 22, 2024
2a9d5ab
Make it easier to migrate `Command` to `BootstrapCmd`
Kobzol Jun 22, 2024
86b2191
Migrate some usage of `Command` to `BootstrapCmd`
Kobzol Jun 22, 2024
f7d9543
Migrate `cargo_clippy_cmd` and `cargo_miri_cmd` to `BootstrapCommand`
Kobzol Jun 22, 2024
8a890cb
Migrate a few command usages to `BootstrapCommand`
Kobzol Jun 22, 2024
83d33c2
Migrate a few command usages to `BootstrapCommand`
Kobzol Jun 22, 2024
bed2cbd
Get rid of `Deref/DerefMut` impl for `BootstrapCmd`
Kobzol Jun 22, 2024
2ebfcce
Migrate more `Command` usages to `BootstrapCmd`
Kobzol Jun 22, 2024
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
2 changes: 1 addition & 1 deletion src/bootstrap/src/core/build_steps/clean.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ macro_rules! clean_crate_tree {

// NOTE: doesn't use `run_cargo` because we don't want to save a stamp file,
// and doesn't use `stream_cargo` to avoid passing `--message-format` which `clean` doesn't accept.
builder.run(&mut cargo);
builder.run(cargo);
}
}
)+ }
Expand Down
5 changes: 3 additions & 2 deletions src/bootstrap/src/core/build_steps/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use crate::core::builder::crate_description;
use crate::core::builder::Cargo;
use crate::core::builder::{Builder, Kind, PathSet, RunConfig, ShouldRun, Step, TaskPath};
use crate::core::config::{DebuginfoLevel, LlvmLibunwind, RustcLto, TargetSelection};
use crate::utils::exec::BootstrapCommand;
use crate::utils::helpers::{
exe, get_clang_cl_resource_dir, is_debug_info, is_dylib, output, symlink_dir, t, up_to_date,
};
Expand Down Expand Up @@ -771,7 +772,7 @@ impl Step for StartupObjects {
let src_file = &src_dir.join(file.to_string() + ".rs");
let dst_file = &dst_dir.join(file.to_string() + ".o");
if !up_to_date(src_file, dst_file) {
let mut cmd = Command::new(&builder.initial_rustc);
let mut cmd = BootstrapCommand::new(&builder.initial_rustc);
cmd.env("RUSTC_BOOTSTRAP", "1");
if !builder.local_rebuild {
// a local_rebuild compiler already has stage1 features
Expand Down Expand Up @@ -2076,7 +2077,7 @@ pub fn stream_cargo(
tail_args: Vec<String>,
cb: &mut dyn FnMut(CargoMessage<'_>),
) -> bool {
let mut cargo = Command::from(cargo);
let mut cargo = BootstrapCommand::from(cargo).command;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this usage makes it like building commands from std Command, so it will not be possible to profile/debug/log (pretty much any reason that this implementation was based on) the execution. The inner command should never be exposed to avoid this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I plan to address this later. I'm doing the refactoring step by step, by introducing compatibility layers in the meantime, otherwise, I'd have to change all usages at once and that would be very hard to review and implement for me. One of those compatibility layers is making the Command pub, so that I can access it when needed.

As one of the next N steps, I will make the command private, which will force me to deal with the more complex cases, like output streaming. But I don't want to do that yet, first I want to port all/most of the simple use-cases and iterate on the API.

// Instruct Cargo to give us json messages on stdout, critically leaving
// stderr as piped so we can get those pretty colors.
let mut message_format = if builder.config.json_output {
Expand Down
37 changes: 19 additions & 18 deletions src/bootstrap/src/core/build_steps/dist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use crate::core::build_steps::tool::{self, Tool};
use crate::core::builder::{Builder, Kind, RunConfig, ShouldRun, Step};
use crate::core::config::TargetSelection;
use crate::utils::channel::{self, Info};
use crate::utils::exec::BootstrapCommand;
use crate::utils::helpers::{
exe, is_dylib, move_file, output, t, target_supports_cranelift_backend, timeit,
};
Expand Down Expand Up @@ -1599,14 +1600,14 @@ impl Step for Extended {
let _ = fs::remove_dir_all(&pkg);

let pkgbuild = |component: &str| {
let mut cmd = Command::new("pkgbuild");
let mut cmd = BootstrapCommand::new("pkgbuild");
cmd.arg("--identifier")
.arg(format!("org.rust-lang.{}", component))
.arg("--scripts")
.arg(pkg.join(component))
.arg("--nopayload")
.arg(pkg.join(component).with_extension("pkg"));
builder.run(&mut cmd);
builder.run(cmd);
};

let prepare = |name: &str| {
Expand Down Expand Up @@ -1636,7 +1637,7 @@ impl Step for Extended {
builder.create_dir(&pkg.join("res"));
builder.create(&pkg.join("res/LICENSE.txt"), &license);
builder.install(&etc.join("gfx/rust-logo.png"), &pkg.join("res"), 0o644);
let mut cmd = Command::new("productbuild");
let mut cmd = BootstrapCommand::new("productbuild");
cmd.arg("--distribution")
.arg(xform(&etc.join("pkg/Distribution.xml")))
.arg("--resources")
Expand All @@ -1649,7 +1650,7 @@ impl Step for Extended {
.arg("--package-path")
.arg(&pkg);
let _time = timeit(builder);
builder.run(&mut cmd);
builder.run(cmd);
}

if target.is_windows() {
Expand Down Expand Up @@ -1704,7 +1705,7 @@ impl Step for Extended {

let heat_flags = ["-nologo", "-gg", "-sfrag", "-srd", "-sreg"];
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("rustc")
Expand All @@ -1720,7 +1721,7 @@ impl Step for Extended {
);
if built_tools.contains("rust-docs") {
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("rust-docs")
Expand All @@ -1738,7 +1739,7 @@ impl Step for Extended {
);
}
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("cargo")
Expand All @@ -1755,7 +1756,7 @@ impl Step for Extended {
.arg(etc.join("msi/remove-duplicates.xsl")),
);
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("rust-std")
Expand All @@ -1771,7 +1772,7 @@ impl Step for Extended {
);
if built_tools.contains("rust-analyzer") {
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("rust-analyzer")
Expand All @@ -1790,7 +1791,7 @@ impl Step for Extended {
}
if built_tools.contains("clippy") {
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("clippy")
Expand All @@ -1809,7 +1810,7 @@ impl Step for Extended {
}
if built_tools.contains("miri") {
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("miri")
Expand All @@ -1827,7 +1828,7 @@ impl Step for Extended {
);
}
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("rust-analysis")
Expand All @@ -1845,7 +1846,7 @@ impl Step for Extended {
);
if target.ends_with("windows-gnu") {
builder.run(
Command::new(&heat)
BootstrapCommand::new(&heat)
.current_dir(&exe)
.arg("dir")
.arg("rust-mingw")
Expand All @@ -1864,7 +1865,7 @@ impl Step for Extended {
let candle = |input: &Path| {
let output = exe.join(input.file_stem().unwrap()).with_extension("wixobj");
let arch = if target.contains("x86_64") { "x64" } else { "x86" };
let mut cmd = Command::new(&candle);
let mut cmd = BootstrapCommand::new(&candle);
cmd.current_dir(&exe)
.arg("-nologo")
.arg("-dRustcDir=rustc")
Expand Down Expand Up @@ -1893,7 +1894,7 @@ impl Step for Extended {
if target.ends_with("windows-gnu") {
cmd.arg("-dGccDir=rust-mingw");
}
builder.run(&mut cmd);
builder.run(cmd);
};
candle(&xform(&etc.join("msi/rust.wxs")));
candle(&etc.join("msi/ui.wxs"));
Expand Down Expand Up @@ -1925,7 +1926,7 @@ impl Step for Extended {

builder.info(&format!("building `msi` installer with {light:?}"));
let filename = format!("{}-{}.msi", pkgname(builder, "rust"), target.triple);
let mut cmd = Command::new(&light);
let mut cmd = BootstrapCommand::new(&light);
cmd.arg("-nologo")
.arg("-ext")
.arg("WixUIExtension")
Expand Down Expand Up @@ -1962,7 +1963,7 @@ impl Step for Extended {
cmd.arg("-sice:ICE57");

let _time = timeit(builder);
builder.run(&mut cmd);
builder.run(cmd);

if !builder.config.dry_run() {
t!(move_file(exe.join(&filename), distdir(builder).join(&filename)));
Expand All @@ -1971,7 +1972,7 @@ impl Step for Extended {
}
}

fn add_env(builder: &Builder<'_>, cmd: &mut Command, target: TargetSelection) {
fn add_env(builder: &Builder<'_>, cmd: &mut BootstrapCommand, target: TargetSelection) {
let mut parts = builder.version.split('.');
cmd.env("CFG_RELEASE_INFO", builder.rust_version())
.env("CFG_RELEASE_NUM", &builder.version)
Expand Down
19 changes: 10 additions & 9 deletions src/bootstrap/src/core/build_steps/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ impl Step for TheBook {
let shared_assets = builder.ensure(SharedAssets { target });

// build the command first so we don't nest GHA groups
// FIXME: this doesn't do anything!
builder.rustdoc_cmd(compiler);

// build the redirect pages
Expand Down Expand Up @@ -300,7 +301,7 @@ fn invoke_rustdoc(
cmd.arg("-Z").arg("unstable-options").arg("--disable-minification");
}

builder.run(&mut cmd);
builder.run(cmd);
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
Expand Down Expand Up @@ -394,7 +395,7 @@ impl Step for Standalone {
} else {
cmd.arg("--markdown-css").arg("rust.css");
}
builder.run(&mut cmd);
builder.run(cmd);
}

// We open doc/index.html as the default if invoked as `x.py doc --open`
Expand Down Expand Up @@ -493,7 +494,7 @@ impl Step for Releases {
cmd.arg("--disable-minification");
}

builder.run(&mut cmd);
builder.run(cmd);
}

// We open doc/RELEASES.html as the default if invoked as `x.py doc --open RELEASES.md`
Expand Down Expand Up @@ -737,7 +738,7 @@ fn doc_std(
format!("library{} in {} format", crate_description(requested_crates), format.as_str());
let _guard = builder.msg_doc(compiler, description, target);

builder.run(&mut cargo.into());
builder.run(cargo);
builder.cp_link_r(&out_dir, out);
}

Expand Down Expand Up @@ -862,7 +863,7 @@ impl Step for Rustc {
let proc_macro_out_dir = builder.stage_out(compiler, Mode::Rustc).join("doc");
symlink_dir_force(&builder.config, &out, &proc_macro_out_dir);

builder.run(&mut cargo.into());
builder.run(cargo);

if !builder.config.dry_run() {
// Sanity check on linked compiler crates
Expand Down Expand Up @@ -995,7 +996,7 @@ macro_rules! tool_doc {
symlink_dir_force(&builder.config, &out, &proc_macro_out_dir);

let _guard = builder.msg_doc(compiler, stringify!($tool).to_lowercase(), target);
builder.run(&mut cargo.into());
builder.run(cargo);

if !builder.config.dry_run() {
// Sanity check on linked doc directories
Expand Down Expand Up @@ -1079,7 +1080,7 @@ impl Step for ErrorIndex {
index.arg(out);
index.arg(&builder.version);

builder.run(&mut index);
builder.run(index);
}
}

Expand Down Expand Up @@ -1115,7 +1116,7 @@ impl Step for UnstableBookGen {
cmd.arg(builder.src.join("src"));
cmd.arg(out);

builder.run(&mut cmd);
builder.run(cmd);
}
}

Expand Down Expand Up @@ -1210,7 +1211,7 @@ impl Step for RustcBook {
self.compiler.host,
self.target,
);
builder.run(&mut cmd);
builder.run(cmd);
drop(doc_generator_guard);

// Run rustbook/mdbook to generate the HTML pages.
Expand Down
6 changes: 3 additions & 3 deletions src/bootstrap/src/core/build_steps/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
use std::env;
use std::fs;
use std::path::{Component, Path, PathBuf};
use std::process::Command;

use crate::core::build_steps::dist;
use crate::core::builder::{Builder, RunConfig, ShouldRun, Step};
use crate::core::config::{Config, TargetSelection};
use crate::utils::exec::BootstrapCommand;
use crate::utils::helpers::t;
use crate::utils::tarball::GeneratedTarball;
use crate::{Compiler, Kind};
Expand Down Expand Up @@ -102,7 +102,7 @@ fn install_sh(
let empty_dir = builder.out.join("tmp/empty_dir");
t!(fs::create_dir_all(&empty_dir));

let mut cmd = Command::new(SHELL);
let mut cmd = BootstrapCommand::new(SHELL);
cmd.current_dir(&empty_dir)
.arg(sanitize_sh(&tarball.decompressed_output().join("install.sh")))
.arg(format!("--prefix={}", prepare_dir(&destdir_env, prefix)))
Expand All @@ -113,7 +113,7 @@ fn install_sh(
.arg(format!("--libdir={}", prepare_dir(&destdir_env, libdir)))
.arg(format!("--mandir={}", prepare_dir(&destdir_env, mandir)))
.arg("--disable-ldconfig");
builder.run(&mut cmd);
builder.run(cmd);
t!(fs::remove_dir_all(&empty_dir));
}

Expand Down
15 changes: 7 additions & 8 deletions src/bootstrap/src/core/build_steps/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl Step for BuildManifest {
cmd.arg(&builder.config.channel);

builder.create_dir(&distdir(builder));
builder.run(&mut cmd);
builder.run(cmd);
}
}

Expand All @@ -72,7 +72,7 @@ impl Step for BumpStage0 {
fn run(self, builder: &Builder<'_>) -> Self::Output {
let mut cmd = builder.tool_cmd(Tool::BumpStage0);
cmd.args(builder.config.args());
builder.run(&mut cmd);
builder.run(cmd);
}
}

Expand All @@ -94,7 +94,7 @@ impl Step for ReplaceVersionPlaceholder {
fn run(self, builder: &Builder<'_>) -> Self::Output {
let mut cmd = builder.tool_cmd(Tool::ReplaceVersionPlaceholder);
cmd.arg(&builder.src);
builder.run(&mut cmd);
builder.run(cmd);
}
}

Expand Down Expand Up @@ -158,8 +158,7 @@ impl Step for Miri {
// after another --, so this must be at the end.
miri.args(builder.config.args());

let mut miri = Command::from(miri);
builder.run(&mut miri);
builder.run(miri);
}
}

Expand Down Expand Up @@ -189,7 +188,7 @@ impl Step for CollectLicenseMetadata {
let mut cmd = builder.tool_cmd(Tool::CollectLicenseMetadata);
cmd.env("REUSE_EXE", reuse);
cmd.env("DEST", &dest);
builder.run(&mut cmd);
builder.run(cmd);

dest
}
Expand Down Expand Up @@ -219,7 +218,7 @@ impl Step for GenerateCopyright {
let mut cmd = builder.tool_cmd(Tool::GenerateCopyright);
cmd.env("LICENSE_METADATA", &license_metadata);
cmd.env("DEST", &dest);
builder.run(&mut cmd);
builder.run(cmd);

dest
}
Expand All @@ -243,7 +242,7 @@ impl Step for GenerateWindowsSys {
fn run(self, builder: &Builder<'_>) {
let mut cmd = builder.tool_cmd(Tool::GenerateWindowsSys);
cmd.arg(&builder.src);
builder.run(&mut cmd);
builder.run(cmd);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/src/core/build_steps/suggest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub fn suggest(builder: &Builder<'_>, run: bool) {
.tool_cmd(Tool::SuggestTests)
.env("SUGGEST_TESTS_GIT_REPOSITORY", git_config.git_repository)
.env("SUGGEST_TESTS_NIGHTLY_BRANCH", git_config.nightly_branch)
.command
.output()
.expect("failed to run `suggest-tests` tool");

Expand Down
Loading
Loading