Iteratively construct target cfg #11114
Conversation
When setting target features via rustflags via `[build]` config key, cargo correctly propagates them to rustc (via -C flag) and to build.rs (via CARGO_CFG_TARGET_FEATURE env var). However, if `[target.cfg]` is used instead, build.rs doesn't get the flags (rustc still gets them though).
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Ok, I think I've debugged what is happening here. The core of the issue is that we get an inconsistent that is, rusflags says that we have sse4.2, but cfg say we don't. When we run build.rs, we look at the cfg. When we run the actual compiler, we use RUSTFLAGS, hence the discrepancy. I think the issue lies in this logic: cargo/src/cargo/core/compiler/build_context/target_info.rs Lines 236 to 245 in 8dea819 What happens here is that rustflags and cfgs are mutually dependent: rustflags determine rustc's cfgs, and cfgs determine which config sections apply, which might affect rustflags. The code currently does one-and-a-half iterations of fixed point algorithm. |
|
The last commit adds a fixed-point iteration loop, as a crude hack. That looks somewhat amusing, in not a good way, so I think I'll pause here and wait for feedback from Cargo team on this one! |
|
There is some more background on this general problem in #6858. I've been reluctant to run It might be good to also give some careful consideration on the backwards compatibility hazards with this change. Will it change the behavior for existing projects in a meaningful way? The overall approach seems reasonable to me. |
| supports_split_debuginfo, | ||
| }); | ||
| } | ||
| anyhow::bail!("diverging rustflags") |
There was a problem hiding this comment.
Before merging, I think it would be good to make this error a little more descriptive.
I also wouldn't say it is necessarily diverging. It could just be that it requires more steps to finish.
matklad
force-pushed
the
build-falgs
branch
from
19362a7
to
c333b0a
Compare
|
Yeah, this all sounds reasonable. We indeed should only run this when needed, and it indeed should be cached. Given that the use-case here is essentially "set cfgs based on other cfgs", I don't see a better fundamental solution. For my specific use-case (setting target_features), I would imagine a more specialized solution (like a dedicated key in the profile or what not) would work, but, even if we had that, it still is very surprising that today's code observes inconsistent information between build.rs and rustc. Regarding backwards compatibility, yeah.... This definitely would affect some builds, probably not too many, and probably by making them more correct, but "fixing builds" tends annoy people also :) I guess my value judgement is that this is a rather obscure corner case, so few folks would be affected, and it feels like its' worth fixing (in our case, we get a significant but suble perf regression due to sse flags not propagating to our C deps). Thats why, after sleeping on it, I figured that outright erroring-out is probably not the right call here, so:
This, at least, is guaranteed to not outright bork any existing builds. I've also added a test which fixes the current behavior. |
ehuss
changed the title
|
Sounds reasonable to me. Thanks! @bors r+ |
bors
added
S-waiting-on-bors
|
@bors r- r=ehuss |
bors
added
S-waiting-on-author
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14 2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000 - build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150) - Indicate how Cargo locates the manifest (rust-lang/cargo#10770) - Reduce references to `[project]` within cargo (rust-lang/cargo#11135) - Iteratively construct target cfg (rust-lang/cargo#11114) - update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146) - Call out that not all config values can be set via env vars (rust-lang/cargo#11139) - Bump to 0.67.0, update changelog (rust-lang/cargo#11137) - ci: update toolchain for building api doc (rust-lang/cargo#11134) - Http publish not noop (rust-lang/cargo#11111) - Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113) - switch to `std::task::ready!()` where possible (rust-lang/cargo#11130) - Report cmd aliasing failure with more contexts (rust-lang/cargo#11087) - minor: remove unused mut (rust-lang/cargo#11127) - fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118) - This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938) - Add support for relative git submodule paths (rust-lang/cargo#11106) - make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098) - Unlink old final artifacts before compilation (rust-lang/cargo#11122) - refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116) - fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119) - Add a minor clarification (rust-lang/cargo#11093) - doc(changelog): mention CVE fixes (rust-lang/cargo#11104)
Update cargo 22 commits in 73ba3f35e0205844418260722c11602113179c4a..f5fed93ba24607980647962c59863bbabb03ce14 2022-09-18 06:38:16 +0000 to 2022-09-27 12:03:57 +0000 - build-scripts.md: Use em dash consistently. (rust-lang/cargo#11150) - Indicate how Cargo locates the manifest (rust-lang/cargo#10770) - Reduce references to `[project]` within cargo (rust-lang/cargo#11135) - Iteratively construct target cfg (rust-lang/cargo#11114) - update comment about `CARGO_BIN_EXE_` (rust-lang/cargo#11146) - Call out that not all config values can be set via env vars (rust-lang/cargo#11139) - Bump to 0.67.0, update changelog (rust-lang/cargo#11137) - ci: update toolchain for building api doc (rust-lang/cargo#11134) - Http publish not noop (rust-lang/cargo#11111) - Improve errors for TOML fields that support workspace inheritance (rust-lang/cargo#11113) - switch to `std::task::ready!()` where possible (rust-lang/cargo#11130) - Report cmd aliasing failure with more contexts (rust-lang/cargo#11087) - minor: remove unused mut (rust-lang/cargo#11127) - fix(cli): Forward non-UTF8 arguments to external subcommands (rust-lang/cargo#11118) - This change adds an example to the authors attribute in the manifest. (rust-lang/cargo#10938) - Add support for relative git submodule paths (rust-lang/cargo#11106) - make unknown features on `cargo add` more discoverable (rust-lang/cargo#11098) - Unlink old final artifacts before compilation (rust-lang/cargo#11122) - refactor(cli): Prepare for clap v4 (rust-lang/cargo#11116) - fix(cli): Error trailing args rather than ignore (rust-lang/cargo#11119) - Add a minor clarification (rust-lang/cargo#11093) - doc(changelog): mention CVE fixes (rust-lang/cargo#11104)
) With #8284 we upgraded cargo version to `1.66`, so rust-lang/cargo#11114 is available. This PR is the same as #7130, but we can safely merge it now. See #7650 for more context. In order to test it I've used the code from [`librocksdb-sys/build.rs`](https://github.com/rust-rocksdb/rust-rocksdb/blob/master/librocksdb-sys/build.rs#L111) as part of `neard/build.rs`: ``` fn main() { - if let Err(err) = try_main() { - eprintln!("{}", err); - std::process::exit(1); + let target_feature = std::env::var("CARGO_CFG_TARGET_FEATURE").unwrap(); + let target_features: Vec<_> = target_feature.split(',').collect(); + if target_features.contains(&"neon") { + panic!("FAIL: {target_feature}"); + } else { + panic!("OK: {target_feature}"); } } ``` It was was tested on M1 macbook, so I've changed the feature to `neon` (which is enabled by default), my `.cargo/config.toml` is as follows: ``` -[target.'cfg(target_arch = "x86_64")'] -rustflags = ["-Ctarget-feature=+sse4.1,+sse4.2", "-Cforce-unwind-tables=y"] +[target.'cfg(target_arch = "aarch64")'] +rustflags = ["-Ctarget-feature=-neon", "-Cforce-unwind-tables=y"] ``` `neon` was successfully removed when using `1.66` toolchain: ``` OK: crc,...,vh ``` while it was still present with `1.65`: ``` FAIL: aes,..,neon,...,vh ``` This proves that rustflags are passed as env variable to `build.rs` in `1.66`, which was not the case in `1.65`
…ar#8305) With near#8284 we upgraded cargo version to `1.66`, so rust-lang/cargo#11114 is available. This PR is the same as near#7130, but we can safely merge it now. See near#7650 for more context. In order to test it I've used the code from [`librocksdb-sys/build.rs`](https://github.com/rust-rocksdb/rust-rocksdb/blob/master/librocksdb-sys/build.rs#L111) as part of `neard/build.rs`: ``` fn main() { - if let Err(err) = try_main() { - eprintln!("{}", err); - std::process::exit(1); + let target_feature = std::env::var("CARGO_CFG_TARGET_FEATURE").unwrap(); + let target_features: Vec<_> = target_feature.split(',').collect(); + if target_features.contains(&"neon") { + panic!("FAIL: {target_feature}"); + } else { + panic!("OK: {target_feature}"); } } ``` It was was tested on M1 macbook, so I've changed the feature to `neon` (which is enabled by default), my `.cargo/config.toml` is as follows: ``` -[target.'cfg(target_arch = "x86_64")'] -rustflags = ["-Ctarget-feature=+sse4.1,+sse4.2", "-Cforce-unwind-tables=y"] +[target.'cfg(target_arch = "aarch64")'] +rustflags = ["-Ctarget-feature=-neon", "-Cforce-unwind-tables=y"] ``` `neon` was successfully removed when using `1.66` toolchain: ``` OK: crc,...,vh ``` while it was still present with `1.65`: ``` FAIL: aes,..,neon,...,vh ``` This proves that rustflags are passed as env variable to `build.rs` in `1.66`, which was not the case in `1.65`
When setting target features via rustflags via
[build]config key, cargo correctly propagates them to rustc (via -C flag) and to build.rs (via CARGO_CFG_TARGET_FEATURE env var).However, if
[target.cfg]is used instead, build.rs doesn't get the flags (rustc still gets them though).This changes it so that cargo will call
rustcup to two times to collect thecfgvalues, updating which flags to use on the second call based on the cfg discovered in the first call.Closes #6858.