-
Couldn't load subscription status.
- Fork 2.7k
Enable CARGO_CFG_DEBUG_ASSERTIONS in build scripts based on profile #16160
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
base: master
Are you sure you want to change the base?
Conversation
76b11fa to
72144ac
Compare
e0ee77d to
7d83b95
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
…ting fix(test): update test to reflect CARGO_CFG_DEBUG_ASSERTIONS is now passed to build scripts chore(deps): update build-rs to version 0.3.3 in Cargo.lock and Cargo.toml
…e for debug and release profiles
7d83b95 to
ee5e6e3
Compare
| #[cargo_test] | ||
| fn build_script_debug_assertions_build_override() { | ||
| let build_rs = r#" | ||
| fn main() { | ||
| let profile = std::env::var("PROFILE").unwrap(); | ||
| if profile == "debug" { | ||
| assert!(!cfg!(debug_assertions)); | ||
| } else if profile == "release" { | ||
| assert!(cfg!(debug_assertions)); | ||
| } | ||
| } | ||
| "#; | ||
|
|
||
| let p = project() | ||
| .file( | ||
| "Cargo.toml", | ||
| r#" | ||
| [package] | ||
| name = "foo" | ||
| version = "0.1.0" | ||
| edition = "2024" | ||
| [profile.dev.build-override] | ||
| debug-assertions = false | ||
| [profile.release.build-override] | ||
| debug-assertions = true | ||
| "#, | ||
| ) | ||
| .file("src/lib.rs", r#""#) | ||
| .file("build.rs", build_rs) | ||
| .build(); | ||
|
|
||
| p.cargo("check").run(); | ||
| p.cargo("check --release").run(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this checking dev and release? The test is only relevant for the profile build-override is being used on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is checking both to verify that build-override correctly affects build script compilation in both profiles. It's showing the override works bidirectionally:
You can disable debug assertions for build scripts in dev (normally they're on)
You can enable debug assertions for build scripts in release (normally they're off)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, for some reason I had missed the use of build-override in both.
We don't really need tests for each of these two profiles. Once we test things in one profile, its sufficient for all.
…emove reference to debug_assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test(add): add test for CARGO_CFG_DEBUG_ASSERTIONS in dev profile
For future reference, in Conventional commits, add is the scope, or what this applies to. In this case you are sayign you are doing tests for cargo add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While some nits, this is good enough to go in. Thanks for your work on this!
|
@rfcbot fcp merge Like with #14902, this adds an artificial |
|
Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Build scripts need to know whether debug assertions are enabled to properly configure dependencies with matching assertion behavior. Previously,
CARGO_CFG_DEBUG_ASSERTIONSwas filtered out because the cfg query to rustc doesn't include profile settings.This PR manually sets
CARGO_CFG_DEBUG_ASSERTIONSbased on the profile'sdebug-assertionssetting, allowing build scripts to configure dependencies appropriately.FCP: #16160 (comment)
Closes #15760