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.

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

Add target-specific RUSTFLAGS variants #10462

Closed
wants to merge 2 commits into from

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Mar 5, 2022

What does this PR try to resolve?

This is a continuation from the discussion in #10395 (comment), and is aimed at being able to solve problems like #4423 by extending the current RUSTFLAGS environment variable to also have per-target variants as in rust-lang/cc-rs#9.

To quote the unstable docs I wrote for the feature:

The -Z targeted-rustflags argument tells Cargo to also look for
target-specific variants of the RUSTFLAGS and
CARGO_ENCODED_RUSTFLAGS variables (and the same for RUSTDOCFLAGS).
The following variants are supported:

  1. <var>_<target> - for example, RUSTFLAGS_X86_64_UNKNOWN_LINUX_GNU
  2. <build-kind>_<var> - for example, HOST_RUSTFLAGS or TARGET_RUSTFLAGS

HOST_RUSTFLAGS allows setting flags to be passed to rustc for
host-artifacts (like build scripts) when cross-compiling, or all
artifacts when not cross-compiling. See also the host-config feature.
Note that Cargo considers any --target as cross-compiling, even if
the specified target is equal to the host's target triple.
TARGET_RUSTFLAGS specifically applies to all artifacts being built for
the target triple specified with --target.

This feature interacts with the target-applies-to-host setting. When
set to false, <var>_<host target> will not apply to artifacts
built for the host, like build scripts.

More specific flags always take precedence over less specific ones, with
ties broken in favor of CARGO_ENCODED_RUSTFLAGS. So, for example, if
RUSTFLAGS_$TARGET and TARGET_CARGO_ENCODED_RUSTFLAGS are both
specified, RUSTFLAGS_$TARGET would be used.

How should we test and review this PR?

The PR includes two new tests that check for the expected behavior of these flags, and how they behave when more than one is provided.

@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2022
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 5, 2022

I did, as part of this, discover a problem with target-applies-to-host and rustflags as currently implemented (even without this PR). Currently, kind.is_host() returns true for all artifacts when --target is not specified. This means that the function that tries to determine the appropriate rust flags (env_args) has no way to discern host artifacts from target artifacts in that setting. Which in turn means that it cannot correctly implement target-applies-to-host = false when --target isn't specified. The code in Cargo today ends up not taking the fall-through path here for any compilation artifacts

if kind.is_host() {
if target_applies_to_host && requested_kinds == [CompileKind::Host] {
// This is the past Cargo behavior where we fall back to the same logic as for other
// artifacts without --target.

and instead takes the path intended for host artifacts here

// In all other cases, host artifacts just get flags from [host], regardless of
// --target. Or, phrased differently, no `--target` behaves the same as `--target
// <host>`, and host artifacts are always "special" (they don't pick up `RUSTFLAGS` for
// example).
return Ok(rustflags_from_host(config, flags, host_triple)?.unwrap_or_else(Vec::new));

The net result of this is that if --target isn't specified, and target-applies-to-host = false, neither RUSTFLAGS, target.*.rustflags, nor build.rustflags is used for any artifact, and [host] (if set) applies to all artifacts.

This limitation continues to apply with this PR — if --target is not passed and target-applies-to-host = false, RUSTFLAGS_$TARGET and TARGET_RUSTFLAGS do not apply at all, and HOST_RUSTFLAGS applies to all artifacts (relevant code).

To fix this, I think we need a way to tell env_args whether the compilation unit under consideration is a host artifact like a plugin or build script separately from just kind.is_host(). Unfortunately, I'm not familiar enough with that part of the code-base to figure out how feasible that is. I suspect this is what this now-removed comment was alluding to:

// We *want* to apply RUSTFLAGS only to builds for the
// requested target architecture, and not to things like build
// scripts and plugins, which may be for an entirely different
// architecture. Cargo's present architecture makes it quite
// hard to only apply flags to things that are not build
// scripts and plugins though, so we do something more hacky
// instead to avoid applying the same RUSTFLAGS to multiple targets
// arches:

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 5, 2022

/cc @messense given #4423 (comment)

@alexcrichton
Copy link
Member

Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance.

@alexcrichton alexcrichton removed their assignment Mar 8, 2022
@bors
Copy link
Contributor

bors commented Apr 12, 2022

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

@bors
Copy link
Contributor

bors commented May 6, 2022

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

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Sorry for the extremely late review 😅

The implementation itself works for me, though I am entirely not sure whether this is a thing we are going to merge. As you know, we already have a few stable and unstable features interacting with each other in this area. It might be more confusing for users who want to tweak their compiler flags. Specifically, we have a bunch of ways to set them

  • CARGO_ENCODED_RUSTFLAGS
  • RUSTFLAGS
  • target.<cfg|triple>.rustflags
  • build.rustflags
  • profile.<name>.rustflags
  • cargo rustc -- <some flags>
  • cargo build --config <value>

And maybe more. I could have missed some. I wonder how we teach newcomers about this mess 😞

I totally understand the motivations behind your pull request and other features. Just feel a bit overwhelmed.

Comment on lines +1215 to +1216
`HOST_RUSTFLAGS` allows setting flags to be passed to rustc for
host-artifacts (like build scripts) when cross-compiling, or _all_
Copy link
Member

Choose a reason for hiding this comment

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

How does this interact with artifact dependencies (RFC 3028)?
If an artifact dependency specified with a { …, target = "<triple>" }, even without --target specified, Cargo will still try to build that dependency on that target platform.

  • Should it receive HOST_RUSTFLAGS or TARGET_RUSTFLAGS?
  • If it is a build-deps, which one it should get?

Comment on lines +1227 to +1230
More specific flags always take precedence over less specific ones, with
ties broken in favor of `CARGO_ENCODED_RUSTFLAGS`. So, for example, if
`RUSTFLAGS_$TARGET` and `TARGET_CARGO_ENCODED_RUSTFLAGS` are both
specified, `RUSTFLAGS_$TARGET` would be used.
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make an example of this precedence chain? Such as

The precedence for which value is used is done in the following order (first
match wins):
1. `[profile.dev.package.name]` — A named package.
2. `[profile.dev.package."*"]` — For any non-workspace member.
3. `[profile.dev.build-override]` — Only for build scripts, proc macros, and
their dependencies.
4. `[profile.dev]` — Settings in `Cargo.toml`.
5. Default values built-in to Cargo.

Comment on lines +668 to +670
Generic = 0,
Kind = 1,
Target = 2,
Copy link
Member

Choose a reason for hiding this comment

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

Why setting discriminant manually, supposed rustc can derive Ord with default discriminants?

Comment on lines +709 to +713
if allow_generic {
env::var(var_base).ok().map(|v| (Specificity::Generic, v))
} else {
None
}
Copy link
Member

Choose a reason for hiding this comment

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

(nit) I believe here we can use allow_generic.then(…) 🎉

Comment on lines +733 to +737
if a.is_empty() {
(s, Vec::new())
} else {
(s, a.split('\x1f').map(str::to_string).collect())
}
Copy link
Member

Choose a reason for hiding this comment

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

If str::split is a no-op for empty string, could we reduce these to one line?

Target = 2,
}

fn get_var_variants(
Copy link
Member

Choose a reason for hiding this comment

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

These new functions and enum are not immediately clear to me. Should we add some docs for them?

@@ -606,22 +606,35 @@ fn env_args(
kind: CompileKind,
Copy link
Member

Choose a reason for hiding this comment

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

Could we also update doc comment for fn env_args?

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2022
@epage
Copy link
Contributor

epage commented Jul 18, 2024

As this is waiting on the author for a couple of years for an issue without an acknowledged solution by the Cargo team, I'm going to go ahead and close this.

I'd recommend picking this back up by proposing the solution on the issue and getting consensus before moving on to a PR, per our policy at https://doc.crates.io/contrib/process/working-on-cargo.html

@epage epage closed this Jul 18, 2024
@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 20, 2024

@epage Seems reasonable to me 👍 I have since moved on from the place where I needed this, so don't have much incentive to continue pushing on it. The reason it didn't move earlier was mostly because it didn't seem like there was a clear path forward that the Cargo team would be comfortable with (which is also fair, RUSTFLAGS is a thorny thing already), and I didn't have any radically good solutions to those concerns.

I still think something like this would be valuable, so hoping this might at least serve as a useful starting point to someone else to pick up from at some later point in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants