Include lints.rust.unexpected_cfgs.check-cfg in the fingerprint
#13958
Conversation
rustbot
added
A-rebuild-detection
| // Include all the args from `[lints.rust.unexpected_cfgs.check-cfg]` | ||
| let mut lint_check_cfg = Vec::new(); | ||
| if let Ok(Some(lints)) = unit.pkg.manifest().resolved_toml().resolved_lints() { | ||
| if let Some(rust_lints) = lints.get("rust") { | ||
| if let Some(unexpected_cfgs) = rust_lints.get("unexpected_cfgs") { | ||
| if let Some(config) = unexpected_cfgs.config() { | ||
| if let Some(check_cfg) = config.get("check-cfg") { | ||
| if let Ok(check_cfgs) = | ||
| toml::Value::try_into::<Vec<String>>(check_cfg.clone()) | ||
| { | ||
| lint_check_cfg = check_cfgs; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Rather than re-looking up this entry, we should see if there is a good way to make this reuseable
Example areas to explore
- I assume we fingerprint the build script check-cfg. Can we mix the two
- We already check
lint_rustflags. Should we instead move the lookup to be a part of that?
There was a problem hiding this comment.
I assume we fingerprint the build script check-cfg. Can we mix the two
I don't know, I'm not sure we do anything special for build-script. I think they are just seen as deps in fingerprint.
We already check lint_rustflags. Should we instead move the lookup to be a part of that?
I had the same though since lint_rustflags creates the different --warn, --allow, ... flags. We could add the --check-cfg args at some point, but not right now since we need to gate all --check-cfg args behind the compiler supporting check-cfg stably, which can only be done when we have access to the BuildRunner1 and Unit (I think) which we don't have access to when lint_rustflags is called
Footnotes
There was a problem hiding this comment.
I looked a bit more and I think the best way forward would be to add the --check-cfg args in toml::lints_to_rustflags so that we don't need to lookup them here and in check_cfg_args. I've added a fixme for that.
There was a problem hiding this comment.
Created #13975 (so the hack can be tracked) and updated the code with your suggested wording (as well as changing it to a HACK).
Urgau
force-pushed
the
check-cfg-config-fingerprint
branch
from
bfe62ed to
bb3af49
Compare
Urgau
force-pushed
the
check-cfg-config-fingerprint
branch
from
bb3af49 to
dfb69e6
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 5 commits in a8d72c675ee52dd57f0d8f2bae6655913c15b2fb..431db31d0dbeda320caf8ef8535ea48eb3093407 2024-05-24 03:34:17 +0000 to 2024-05-28 18:17:31 +0000 - Include `lints.rust.unexpected_cfgs.check-cfg` in the fingerprint (rust-lang/cargo#13958) - feat(test): Auto-redact elapsed time (rust-lang/cargo#13973) - chore: Update to snapbox 0.6 (rust-lang/cargo#13963) - fix: check if rev is full commit sha for github fast path (rust-lang/cargo#13969) - test: switch from `drop` to `let _` due to nightly rustc change (rust-lang/cargo#13964) r? ghost
Cleanup duplicated check-cfg lint logic ### What does this PR try to resolve? This PR clean-ups some duplication left by #13958, because of Cargo MSRV. Fixes #13975 ### How should we test and review this PR? The tests in `tests/testsuite/check_cfg.rs` show no change in behaviours (except for the better error messages). I suggest maybe reviewing commit by commit.
5 participants
What does this PR try to resolve?
When changing the
--check-cfgargs in thelints.rust.unexpected_cfgs.check-cfglint config, the build should be restarted as the arguments we pass to the compiler change, and they can change the diagnostics output by generating new or removing someunexpected_cfgswarning(s).How should we test and review this PR?
Look at the before and after test (separate commit).
Additional information
Similar to #13012 which did that for the declared features.
Contrary to that PR, I didn't add a new
DirtyReasonvariant, since even the[lints]table didn't get one.r? @epage