add rust.rustflags and per target rustflags options to bootstrap.toml

[karolzwolak]

Part of #148782; see also #148708

Add new options rust.rustflags for all targets and rustflags par target that will pass specified flags to rustc for all stages. Target specific flags override (are passed after) global rust.rustflags ones.

This makes easy to persistently pass any flag to the compiler when building rustc. For example you can use a different linker by putting the following in bootstrap.toml:

[rust]
rustflags = ["-Clinker=clang", "-Clink-arg=--ld-path=wild"]

r? bootstrap

[rustbot]

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies bootstrap.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[Kobzol]

Just to clarify, does this provide any additional benefit over just configuring .cargo/config.toml and putting the rustflags there?

[karolzwolak]

Yes it does!
Putting rustflags in .cargo/config.toml in the rust project root won't really have an effect on building rustc. It may have an effect on the dependencies of rustc — but not rustc itself. I may be wrong here but putting these flags in .cargo/config.toml doesn't actually link rustc using wild — whereas putting them in bootstrap.toml does.

[Kobzol]

Right. .cargo/config.toml is actually used, but since bootstrap also unconditionally sets RUSTFLAGS, and that has priority over the config file, rustflags specifically won't be applied.

It would be great if you could generalize this to multiple targets, so that it would be possible to set e.g. [target.x86_64-unknown-linux-gnu] rustflags = [...]. Similar to other bootstrap options, the target options should have priority, and if they are unset, the top-level rust.rustflags option should be used instead.

I would also suggest that RUSTFLAGS from the environment should have priority over the rustflags in the TOML file. I checked what happens now, and RUSTFLAGS is currently passed before bootstrap's own rustflags. I think that having the env. var. override what is in the config file makes more sense, as it is easier to change the env. var. than to modify the config.

[karolzwolak]

Hey, does this PR allow for target specific rustflags?
or only one rustflag option globally?
for ex
[linux-arm64]
rustflags="sysroot-arm"
[linux-x64]
rustflags="sysroot-x64"

It's only global ones — but I will add target specific ones (see the following reply).

It would be great if you could generalize this to multiple targets, so that it would be possible to set e.g. [target.x86_64-unknown-linux-gnu] rustflags = [...]

Sure, will do. I wasn't sure it was needed so I only added global ones.

I would also suggest that RUSTFLAGS from the environment should have priority over the rustflags in the TOML file. I checked what happens now, and RUSTFLAGS is currently passed before bootstrap's own rustflags. I think that having the env. var. override what is in the config file makes more sense, as it is easier to change the env. var. than to modify the config.

Yeah I know it's a bit weird. However all options from bootstrop.toml override (come after) RUSTFLAGS variables.
Take a look at how it's currently implemented:

rust/src/bootstrap/src/core/builder/cargo.rs

Lines 26 to 31 in c8f22ca

	implRustflags{
	fnnew(target:TargetSelection) -> Rustflags{
	letmut ret = Rustflags(String::new(), target);
	ret.propagate_cargo_env("RUSTFLAGS");
	ret
	}

rust/src/bootstrap/src/core/builder/cargo.rs

Lines 603 to 615 in c8f22ca

	letmut rustflags = Rustflags::new(target);
	if build_compiler_stage != 0{
	ifletOk(s) = env::var("CARGOFLAGS_NOT_BOOTSTRAP"){
	cargo.args(s.split_whitespace());
	}
	rustflags.env("RUSTFLAGS_NOT_BOOTSTRAP");
	}else{
	ifletOk(s) = env::var("CARGOFLAGS_BOOTSTRAP"){
	cargo.args(s.split_whitespace());
	}
	rustflags.env("RUSTFLAGS_BOOTSTRAP");
	rustflags.arg("--cfg=bootstrap");
	}

RUSTFLAGS are first, followed by more specific {RUST, CARGO}FLAGS_{NOT_, _}BOOTSTRAP flags. Relative order of the all the variables is okay here — but they all should be applied after all the bootstrap.toml options — not before.
I think we should match behavior of cargo:

There are four mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

CARGO_ENCODED_RUSTFLAGS environment variable.
RUSTFLAGS environment variable.
All matching target..rustflags and target..rustflags config entries joined together.
build.rustflags config value.

This could be a breaking change breaking peoples bootstrap.toml. What should we do?
I personally think we should make the RUSTFLAG variables put their contents at the end of flags in a separate PR first — then change rustflags here so they work like in cargo.

[Urgau]

If the main goal is to allow the use of custom linkers, why not have a more targeted approch to it? Allowing anyone to set custom RUSTFLAGS imo problematic as it opens the door to many incompatibilities.

Some could set -Cdebug-assertions=no while in bootstrap.toml it's set activated, making (at least) the tests suite unhappy.

[Kobzol]

but they all should be applied after all the bootstrap.toml options — not before.

Yeah, that's what I was proposing. RUSTFLAGS from the env. should come after stuff from bootstrap.toml so that it has higher precedence.

If the main goal is to allow the use of custom linkers, why not have a more targeted approch to it? Allowing anyone to set custom RUSTFLAGS imo problematic as it opens the door to many incompatibilities.

We already have a lot of custom configs, including linkers. But I don't doubt that there are a bunch of people that just want to pass whatever flags they need when building rustc, and it sounds reasonable to me to make this easier. Of course we should also document that using this flags is "at your own risk" and it can break pretty much anything.

[karolzwolak]

If the main goal is to allow the use of custom linkers, why not have a more targeted approch to it? Allowing anyone to set custom RUSTFLAGS imo problematic as it opens the door to many incompatibilities.

We already have a lot of custom configs, including linkers. But I don't doubt that there are a bunch of people that just want to pass whatever flags they need when building rustc, and it sounds reasonable to me to make this easier. Of course we should also document that using this flags is "at your own risk" and it can break pretty much anything.

Yes exactly — I didn't want to add yet another option that will be limited.

Yeah, that's what I was proposing. RUSTFLAGS from the env. should come after stuff from bootstrap.toml so that it has higher precedence.

Okay — then I'm putting this PR on hold till I change the precedence in another PR.

[Kobzol]

Okay — then I'm putting this PR on hold till I change the precedence in another PR.

Oh, sorry, I was unclear 🤦 I didn't mean to change the existing behavior w.r.t. the old rustflags handling. I just thought that we might specifically put the new rustflags from bootstrap.toml before (not after) RUSTFLAGS. That shouldn't be a breaking change, because before there were no bootstrap.toml flags being passed.

So to summarize the order:

1. Flags from bootstrap.toml
2. Flags from RUSTFLAGS
3. Flags that bootstrap itself sets

Today, we have the order 2) -> 3), after this PR we'd have 1) -> 2) -> 3). There is no need to land a separate PR, I think.

[karolzwolak]

Sure. The new options are going to be a bit awkward to use because they might not have high enough precedence but I get that 'fixing' this would be a breaking change.

[Kobzol]

I mean, there are probably good reasons for that. If you override rustflags set by bootstrap, there's a high chance that the build itself will fail. But yeah, we could still allow it, in theory. But that would have to be a separate change - I can bring this to t-boostrap.

[karolzwolak]

That'd be nice if you could bring it up to t-bootstrap. These flags should behave as they do in cargo. If it turned out we wanted to change the precedence we can do it in the future after merging this — it's gonna be a breaking change anyway.

[Kobzol]

Opened https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Precedence.20of.20RUSTFLAGS.20in.20bootstrap/with/554903280.

[Kobzol]

Please move this discussion to #148816, I will answer there.

[karolzwolak]

Update:

• add per target rustflags
• the precedence is now: rust.rustflags, target.rustflags, RUSTFLAGS (later flags override prior ones)

[karolzwolak]

Changing the precedence of RUSTFLAG env vars is merged. Therefore I've rebased these changes and made some other tweaks.
Since you @Kobzol reviewed my last change and you're familiar with the context I think it makes sense to change the reviewer if nobody minds.

r? @Kobzol

[rustbot]

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

[bors]

☔ The latest upstream changes (presumably #148803) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Kobzol]

Two docs nits, otherwise LGTM. Sorry for the delay.

View changes since this review

[karolzwolak]

Sorry for the delay.

No worries! I addressed the comments.

[Kobzol]

Thanks!

@bors r+

[bors]

📌 Commit 6e63c39 has been approved by Kobzol

It is now in the queue for this repository.