-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add panic=immediate-abort support to Cargo #16041
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
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
(unstable, multiple_build_scripts, "", "reference/unstable.html#multiple-build-scripts"), | ||
|
||
/// Allows use of panic="immediate-abort". | ||
(unstable, panic_immediate_abort, "", "reference/unstable.html#panic-immediate-abort"), |
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.
As the [profile]
table is also supported in Cargo configuration, you might want to add both feature gates:
- Unstable feature in Cargo.toml manifest
- Unstable flag in CLI/config.toml
See the first few commits in #12625 for reference
src/doc/src/reference/unstable.md
Outdated
|
||
* Tracking Issue: [#16042](https://github.com/rust-lang/cargo/issues/16042) | ||
|
||
Allow use of the ImmediateAbort panic strategy in a Cargo profile |
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.
Could we link to rustc book and tracking issue (if any)?
src/cargo/core/profiles.rs
Outdated
|
||
/// The `panic` setting. | ||
#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash, PartialOrd, Ord, serde::Serialize)] | ||
#[serde(rename_all = "lowercase")] |
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.
Hmm. Traced that serialization is used by unit-graph. Before it is too late, i think we should change this to kebab-case.
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.
And strip
seems wrong as well
cargo/src/cargo/core/profiles.rs
Lines 921 to 922 in 94f6379
#[serde(rename_all = "lowercase")] | |
pub enum Strip { |
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.
cargo/src/cargo/core/profiles.rs
Lines 607 to 608 in 56bdf49
#[derive(Clone, Eq, PartialOrd, Ord, serde::Serialize)] | |
pub struct Profile { |
Hey we don't even specify anything for the entire struct! Probably worth a separate PR/issue to address it.
We're holding 2 things behind: - The toolchain because of rust-lang/rust#146974. We'll wait until rust-lang/cargo#16041 is fixed to update. - The WebAssembly spec because non-trivial changes might be needed to update the interpreter (or at least the test-suite).
"immediate-abort" => PanicStrategy::ImmediateAbort, | ||
// This should be validated in TomlProfile::validate | ||
_ => panic!("Unexpected panic setting `{}`", panic), |
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 validation in there needs to be updated
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.
I'm afraid I'm lost. What validation? I can't find any validation of the panic setting or a TomlProfile::validate
.
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.
cargo/src/cargo/util/toml/mod.rs
Lines 2552 to 2560 in 56bdf49
if let Some(panic) = &root.panic { | |
if panic != "unwind" && panic != "abort" { | |
bail!( | |
"`panic` setting of `{}` is not a valid setting, \ | |
must be `unwind` or `abort`", | |
panic | |
); | |
} | |
} |
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.
Outdated comment always 🙇🏾♂️
Generally, we have end-to-end tests for new features (which would have caught #16041 (comment)) and feature gate tests for unstable features. |
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.
Looks good. Thank you!
Would you mind cleaning the commit history a bit. We usually follow atomic commit pattern and every commit shall pass all tests (not MUST we don't enforce that but encourage).
|
||
if let Some(panic) = &root.panic { | ||
if panic != "unwind" && panic != "abort" { | ||
if panic != "unwind" && panic != "abort" && panic != "immediate-abort" { |
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.
Not sure if we want to make it a warning or configurable lint in the future. It seems too restrictive. Anyway, it is worth its own separate topic.
efd675c
to
50697eb
Compare
I don't think a PR that's this conceptually simple needs multiple commits, but I'll split it up if you prefer. |
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.
I personally don't mind for this kind of addition. For bugfix I'll be a bit more nitpicky.
Thanks!
Head branch was pushed to by a user without write access
50697eb
to
4cb6fe4
Compare
The test |
Oh I see this test has been rather flaky recently. Ouch. |
yeah, I should create an issue and temporarily ignore that |
Update cargo submodule 3 commits in 2394ea6cea8b26d717aa67eb1663a2dbf2d26078..801d9b4981dd07e3aecdca1ab86834c13615737e 2025-10-03 14:13:01 +0000 to 2025-10-04 13:30:15 +0000 - chore: Disabled `reserved_windows_name` test temporaily (rust-lang/cargo#16048) - Add panic=immediate-abort support to Cargo (rust-lang/cargo#16041) - Accessing each build script's `OUT_DIR` (rust-lang/cargo#15891)
|
||
This can be enabled like so: | ||
```toml | ||
cargo-features = ["immediate-abort"] |
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.
Should be
cargo-features = ["immediate-abort"] | |
cargo-features = ["panic-immediate-abort"] |
I recently turned
-Zbuild-std-features=panic_immediate_abort
into-Cpanic=immediate-abort
in rust-lang/rust#146317. There was some discussion of the feature on Zulip here: #t-compiler/major changes > Unstably add -Cpanic=immediate-abort compiler-team#909One of the outcomes of this shipping in nightly is that a few use patterns were broken, see rust-lang/rust#146974 and rust-lang/rust#146317 for examples. I think most of the users commenting on these issues are having trouble with how
RUSTFLAGS
is propagated through Cargo, and most likely they would have just gotten what they wanted if they could have set a profile option instead. So I'm trying to implement that.