Iteratively construct target cfg #11114

matklad · 2022-09-20T10:00:09Z

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 rustc up to two times to collect the cfg values, updating which flags to use on the second call based on the cfg discovered in the first call.

Closes #6858.

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).

rust-highfive · 2022-09-20T10:00:12Z

r? @ehuss

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

matklad · 2022-09-20T10:51:10Z

Ok, I think I've debugged what is happening here. The core of the issue is that we get an inconsistent TargetInfo at some point, which looks like this:

TargetInfo {
  rustflags: ["-Ctarget-feature=+sse4.1,+sse4.2"]
  cfg: debug_assertions,panic = "unwind",target_arch = "x86_64",target_endian = "little",target_env = "gnu",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 = "unknown",unix
}

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

    
           // recalculate `rustflags` from above now that we have `cfg` 
        
           // information 
        
           rustflags: env_args( 
        
               config, 
        
               requested_kinds, 
        
               &rustc.host, 
        
               Some(&cfg), 
        
               kind, 
        
               Flags::Rust, 
        
           )?,

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.

matklad · 2022-09-20T11:03:58Z

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!

ehuss · 2022-09-25T17:51:05Z

There is some more background on this general problem in #6858. I've been reluctant to run rustc in a loop because it can take hundreds of milliseconds to run each time. I would be a little more comfortable doing that if it could bypass rustup (#10986, rust-lang/rustup#3031, rust-lang/rustup#3035). However, I believe this should only be a problem if rustflags is explicitly set in a target config table, right? And that should only affect the first uncached run, right?

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.

ehuss · 2022-09-25T17:52:19Z

src/cargo/core/compiler/build_context/target_info.rs

+                supports_split_debuginfo,
+            });
+        }
+        anyhow::bail!("diverging rustflags")


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 · 2022-09-26T12:37:50Z

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:

I restricted us to do only 2 iterations
I warn, rather than error, if that's not enough

This, at least, is guaranteed to not outright bork any existing builds. I've also added a test which fixes the current behavior.

ehuss · 2022-09-26T21:05:08Z

Sounds reasonable to me. Thanks!

@bors r+

bors · 2022-09-26T21:05:10Z

📌 Commit c333b0a has been approved by ehuss

It is now in the queue for this repository.

Mark-Simulacrum · 2022-09-27T01:08:00Z

@bors r- r=ehuss

bors · 2022-09-27T01:08:04Z

📌 Commit c333b0a has been approved by ehuss

It is now in the queue for this repository.

bors · 2022-09-27T01:08:16Z

⌛ Testing commit c333b0a with merge f6de921...

bors · 2022-09-27T01:48:21Z

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing f6de921 to master...

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`

rust-highfive assigned ehuss Sep 20, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2022

mm-near mentioned this pull request Sep 20, 2022

Rollback of #7130 - due to cargo issue near/nearcore#7648

Merged

akhi3030 mentioned this pull request Sep 20, 2022

Don't enable SSE for wasm compilation near/nearcore#7650

Closed

ehuss reviewed Sep 25, 2022

View reviewed changes

when learning target information, run a little fixedpoint iteration loop

c333b0a

matklad force-pushed the build-falgs branch from 19362a7 to c333b0a Compare September 26, 2022 12:24

ehuss changed the title ~~add failing test for CARGO_CFG_TARGET_FEATURE~~ Iteratively construct target cfg Sep 26, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 27, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Sep 27, 2022

bors merged commit f6de921 into rust-lang:master Sep 27, 2022

weihanglo mentioned this pull request Sep 27, 2022

Update cargo rust-lang/rust#102365

Merged

BugenZhao mentioned this pull request Oct 1, 2022

extern library not resolved when specifying --cfg rust flag in [target.<cfg>] #11166

Closed

ehuss added this to the 1.66.0 milestone Oct 1, 2022

BugenZhao mentioned this pull request Oct 8, 2022

build(toolchain): bump to 2022-10-16 risingwavelabs/risingwave#5119

Closed

BugenZhao mentioned this pull request Oct 30, 2022

chore(build): unify rustflags for all targets risingwavelabs/risingwave#6113

Merged

3 tasks

pugachAG mentioned this pull request Jan 6, 2023

fix: only add '-Ctarget-feature=+sse4.1,+sse4.2' flags for x86_64 near/nearcore#8305

Merged

matklad deleted the build-falgs branch March 29, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iteratively construct target cfg #11114

Iteratively construct target cfg #11114

matklad commented Sep 20, 2022 •

edited by ehuss

Loading

rust-highfive commented Sep 20, 2022

matklad commented Sep 20, 2022

matklad commented Sep 20, 2022

ehuss commented Sep 25, 2022

ehuss Sep 25, 2022

matklad commented Sep 26, 2022

ehuss commented Sep 26, 2022

bors commented Sep 26, 2022

Mark-Simulacrum commented Sep 27, 2022

bors commented Sep 27, 2022

bors commented Sep 27, 2022

bors commented Sep 27, 2022

Iteratively construct target cfg #11114

Iteratively construct target cfg #11114

Conversation

matklad commented Sep 20, 2022 • edited by ehuss Loading

rust-highfive commented Sep 20, 2022

matklad commented Sep 20, 2022

matklad commented Sep 20, 2022

ehuss commented Sep 25, 2022

ehuss Sep 25, 2022

Choose a reason for hiding this comment

matklad commented Sep 26, 2022

ehuss commented Sep 26, 2022

bors commented Sep 26, 2022

Mark-Simulacrum commented Sep 27, 2022

bors commented Sep 27, 2022

bors commented Sep 27, 2022

bors commented Sep 27, 2022

matklad commented Sep 20, 2022 •

edited by ehuss

Loading