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

Support overriding initial rustc and cargo paths #102266

Merged
merged 1 commit into from
Sep 25, 2022
Merged

Support overriding initial rustc and cargo paths #102266

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 8 additions & 6 deletions src/bootstrap/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,9 +901,7 @@ impl Config {
config.config = toml_path;

let build = toml.build.unwrap_or_default();
let has_custom_rustc = build.rustc.is_some();

set(&mut config.initial_rustc, build.rustc.map(PathBuf::from));
set(&mut config.out, flags.build_dir.or_else(|| build.build_dir.map(PathBuf::from)));
// NOTE: Bootstrap spawns various commands with different working directories.
// To avoid writing to random places on the file system, `config.out` needs to be an absolute path.
Expand All @@ -912,10 +910,14 @@ impl Config {
config.out = crate::util::absolute(&config.out);
}

if !has_custom_rustc && !config.initial_rustc.starts_with(&config.out) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really understand what this check was trying to accomplish -- it seems pretty odd to me that config.out matters here. The new logic just trusts build.rustc or build.cargo, and falls back to the stage0 path as before.

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand why this behavior is different than before; don't we set initial_rustc on line 906 above? I think the bug is that I forgot the same check for initial_cargo.

That said, I agree the new logic is simpler and easier to understand, so 👍 for using it for initial_rustc as well.

I don't really understand what this check was trying to accomplish -- it seems pretty odd to me that config.out matters

I was trying to be conservative and only change the behavior when bootstrap was downloaded from CI (i.e. config.out wasn't a parent of the RUSTC env variable bootstrap was built with). But I think your approach is fine too and easier to maintain - ideally we wouldn't set RUSTC in the build script at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the build-script set RUSTC matters at all at this point? It's not getting read by config.rs anyway, so I would hope it doesn't :)

Copy link
Member

@jyn514 jyn514 Sep 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh you're right, I'd forgotten I'd completely removed that: https://github.com/rust-lang/rust/pull/98483/files#diff-7eddc76f1be9eca2599a9ae58c65ffe247fbdff9b02ef687439894cab9afe749L781

Yes, this is absolutely the right fix then if there's no default for initial_rustc. I think we can go ahead and remove bootstrap's build script too, I'll make a follow-up PR for that :)

config.initial_rustc = config.out.join(config.build.triple).join("stage0/bin/rustc");
config.initial_cargo = config.out.join(config.build.triple).join("stage0/bin/cargo");
}
config.initial_rustc = build
.rustc
.map(PathBuf::from)
.unwrap_or_else(|| config.out.join(config.build.triple).join("stage0/bin/rustc"));
config.initial_cargo = build
.cargo
.map(PathBuf::from)
.unwrap_or_else(|| config.out.join(config.build.triple).join("stage0/bin/cargo"));

// NOTE: it's important this comes *after* we set `initial_rustc` just above.
if config.dry_run {
Expand Down