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

[target.'cfg(...)'] doesn't expose all keys (like test and features) #8170

Open
indygreg opened this issue Apr 27, 2020 · 7 comments
Open

[target.'cfg(...)'] doesn't expose all keys (like test and features) #8170

indygreg opened this issue Apr 27, 2020 · 7 comments
Labels
A-cfg-expr Area: Platform cfg expressions A-configuration Area: cargo config files and env vars A-target-dependencies Area: [target.'cfg(foo)'.dependencies] C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed. S-propose-close Status: A team member has nominated this for closing, pending further input from the team

Comments

@indygreg
Copy link

Problem

I am trying to override rustflags for my test binaries. But I don't want to do this for the normal, non-test binaries.

In theory, you can use the [target.'cfg(...)'] syntax to influence rustflags on a very granular basis. However, it appears that cfg() is missing a few keys when compiling test binaries, notably test and feature.

# Works
[target.'cfg(target_os = "linux")']
rustflags = ["-C", "link-args=-Wl,-export-dynamic"]

# Does not work - feature test fails.
[target.'cfg(feature = "build-mode-standalone")']
rustflags = ["-C", "link-args=-Wl,-export-dynamic"]

# Does not work - test not defined.
[target.'cfg(test)']
rustflags = ["-C", "link-args=-Wl,-export-dynamic"]

FWIW, --test and --cfg 'feature="build-mode-standalone"' is getting passed into rustc when building the test binaries. So something in Cargo knows about them. It appears that these values aren't getting plumbed through to cfg() evaluation.

Possible Solution(s)

I think cfg() should have access to test, feature, and perhaps other conditional keys when building test binaries.

Output of cargo version: cargo 1.43.0 (3532cf738 2020-03-17)

@indygreg indygreg added the C-bug Category: bug label Apr 27, 2020
@indygreg
Copy link
Author

According to https://doc.rust-lang.org/cargo/reference/config.html#target:

cfg values come from those built-in to the compiler (run rustc --print=cfg to view), values set by build scripts, and extra --cfg flags passed to rustc (such as those defined in RUSTFLAGS). Do not try to match on debug_assertions or Cargo features like feature="foo".

This clearly says that feature keys aren't supported. Sorry for missing that in the docs!

However, it does seem to imply that cargo:rustc-cfg=... lines from build scripts will get included. However, I tested this and they don't appear to work for test binaries. I do see the --cfg getting passed to rustc when building the test binary. But the value set by the build script does not seem to be in the evaluation context for [target.'cfg(...)'], seemingly contrary to what the docs say.

@alexcrichton
Copy link
Member

Currently rustflags is not determined on a per-compilation basis, but rather an extremely coarse per-cargo-execution basis. This means it doesn't have access to any per-unit flags like features/test/etc. This has also been the source of much confusion in the past, however, so it's something that might be worthwhile trying to fix.

@ehuss ehuss added A-cfg-expr Area: Platform cfg expressions A-configuration Area: cargo config files and env vars labels Apr 29, 2020
@tones111
Copy link

I discovered this while trying to enable source-based code coverage (rust-lang/rust#79121) for my test binaries. It would be convenient to be able to add "-Zinstrument-coverage" to RUSTFLAGS when running "cargo test" but not impact debug or release builds.

I'd initially thought this was a small oversight, but it sounds like a fix may require some architectural changes. Is this something that could be worked with some guidance, or is it a significant enough change to require an RFC?

@eholk
Copy link
Contributor

eholk commented Sep 30, 2021

I ran into this issue today. I had some code where we want to change rustflags based on what features were passed on the cargo command line. I'm already using [target.'cfg(...)'] to control rustflags for a couple of other options, since this seems to give the most flexibility in combining different sets of flags.

@golddranks
Copy link
Contributor

I also ran into this issue today. Expected a cfg expression like [target.'cfg(all(target_os = "macos", target_arch = "x86_64", not(test)))'] to work, but the not(test) part gets ignored. I'm building a project for bare-metal x86 with no_std, but unit tests require std. I set some linking flags with rustflags, but I'd like to disable some of them for tests as they are incompatible with the std-requiring test runner.

jul-sh added a commit to jul-sh/oak that referenced this issue Apr 14, 2022
This config does not behave as intended, as (somewhat counterintuitively) cfg in the config does not have access to all the usual values, including features or in this case test.

See the tracking cargo rust-lang/cargo#8170
jul-sh added a commit to project-oak/oak that referenced this issue Apr 14, 2022
This config does not behave as intended, as (somewhat counterintuitively) cfg in the config does not have access to all the usual values, including features or in this case test.

See the tracking cargo rust-lang/cargo#8170
@jirutka
Copy link
Contributor

jirutka commented Jan 15, 2023

For me, cfg in target doesn’t work at all, neither in Cargo.toml or config.toml.

# Doesn't work, it's ignored
[target.'cfg(target_os = "linux")']
z-ng = { rustc-link-lib = ["z-ng"], rustc-cfg = ["zng"] }

# Works
[target.x86_64-alpine-linux-musl]
z-ng = { rustc-link-lib = ["z-ng"], rustc-cfg = ["zng"] }
$ rustc --print=cfg
debug_assertions
panic="unwind"
target_arch="x86_64"
target_endian="little"
target_env="musl"
target_family="unix"
target_feature="fxsr"
target_feature="sse"
target_feature="sse2"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
target_has_atomic="ptr"
target_os="linux"
target_pointer_width="64"
target_vendor="alpine"
unix

@epage epage added S-needs-team-input Status: Needs input from team on whether/how to proceed. A-target-dependencies Area: [target.'cfg(foo)'.dependencies] labels Nov 4, 2023
@epage epage added the S-propose-close Status: A team member has nominated this for closing, pending further input from the team label Feb 14, 2024
@epage
Copy link
Contributor

epage commented Feb 14, 2024

I'm proposing to the cargo team that we close this based on a discussion in a previous cargo team meeting which is covered in the This Development-cycle in Cargo: 1.77. I have the impression there isn't a sufficiently viable way of implementing support for this. Instead, we likely should focus on things like global, mutually exclusive features.

ntn-x2 added a commit to KILTprotocol/kilt-node that referenced this issue Oct 25, 2024
Add more lints to our codebase. I have taken all lints that are allowed
by default, and made either either warnings or strong denies. **I have
not added any lints from the
[pedantic](https://rust-lang.github.io/rust-clippy/master/index.html?groups=pedantic)
group, as those tend to be opinionated and I don't think should be
enforced on a shared codebase.**

Unfortunately, there is an [open cargo
issue](rust-lang/cargo#8170) which makes it
impossible to enable/disable lints based on the `#[cfg(test)]` or
`#[cfg(feature = "...")]` features, hence I have had to enable lints for
either the whole codebase (`[target.'cfg(all())']`), or for runtime-only
code `[target.'cfg(target_arch = "wasm32")']`.

The changeset is huge but just because I tackled all the new lints
generated, so reviewing the new lints (in case some are unwanted or in
case I missed some) would be sufficient, since the CI has been updated
to verify all lints are properly addressed.

Last but not least, I had to enable the `-Dwarnings` flag for ALL clippy
invocations, so also on local machines, because it's not possible to
pass custom `--cfg` either to rustc via cargo, e.g., `cargo clippy --
--cfg ci` and then have a `[target.'cfg(ci)']` entry in the config file.
This might go away in future versions of cargo (we are currently on
1.74), but until then we either never fail on the CI, or we fail for
everything also locally, and I think the latter is a better choice. This
is also because rustc takes additional flags either from the `RUSTFLAGS`
env variable or the `.cargo/config.toml` file. Hence specifying
`RUSTFLAGS="-Dwarnings"` in the CI would not enable any additional lints
as the env variable would take precedence over the config file,
rendering them useless.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cfg-expr Area: Platform cfg expressions A-configuration Area: cargo config files and env vars A-target-dependencies Area: [target.'cfg(foo)'.dependencies] C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed. S-propose-close Status: A team member has nominated this for closing, pending further input from the team
Projects
None yet
Development

No branches or pull requests

8 participants