-
Notifications
You must be signed in to change notification settings - Fork 82
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
GEN-115: Don't compile backtrace support if feature is disabled #4685
GEN-115: Don't compile backtrace support if feature is disabled #4685
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4685 +/- ##
=======================================
Coverage 20.19% 20.19%
=======================================
Files 457 457
Lines 15714 15750 +36
Branches 2380 2380
=======================================
+ Hits 3173 3181 +8
- Misses 12500 12528 +28
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
minor questions, otherwise LGTM
libs/error-stack/Cargo.toml
Outdated
@@ -41,8 +41,9 @@ thiserror = "1.0.59" | |||
rustc_version = "0.4" | |||
|
|||
[features] | |||
default = ["std"] | |||
default = ["backtrace"] |
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 know that std
is implied here, but can we make it explicit, as in: std
, backtrace
?
libs/error-stack/build.rs
Outdated
@@ -10,14 +10,9 @@ fn main() { | |||
} | |||
|
|||
let rustc_version = version_meta.semver; | |||
let trimmed_rustc_version = Version::new( | |||
let _trimmed_rustc_version = Version::new( |
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 commented out, if we don't use it? shouldn't it be removed in that case?
…support-if-rust-feature-is-disabled # Conflicts: # libs/error-stack/CHANGELOG.md
May we release a 0.5.0 for this change? |
I need to check for breaking changes and check if the MSRV did not change. Then we can file a new release, yes! 🙂 |
🌟 What is the purpose of this PR?
In some contexts, such as with
panic_immediate_abort
set, backtraces should not be compiled into the binary. Currently, thestd
feature determines if backtraces are enabled or not. Making this a dedicated feature flag helps reduce the binary size.🔗 Related links
🔍 What does this change?
Backtrace
behind thebacktrace
feature