Conversation
crepererum
left a comment
crepererum
left a comment
There was a problem hiding this comment.
This adds a build config, not a cargo feature. A cargo feature (which I think should be preferred because build configs are an even bigger mess) works like this:
- add feature to
Cargo.toml - change the cfg guard to
#[cfg(feature = "backtrace")]
On top of that, I think this needs some docs, at least for get_back_trace (you probably want to use a single method so you can easily reuse the docstring and either use the cfg-guards within that method or create a new private fn called get_back_trace_impl).
| } | ||
|
|
||
| /// To enable optional rust backtrace in DataFusion: | ||
| /// - [`Setup Env Variables`]<https://doc.rust-lang.org/std/backtrace/index.html#environment-variables> |
| @@ -34,6 +34,7 @@ path = "src/lib.rs" | |||
|
|
|||
| [features] | |||
| avro = ["apache-avro"] | |||
There was a problem hiding this comment.
As written, to be enabled, a user would have to use datafusion_common directly (they couldn't activate it through datafusion)
To enable it via datafusion we also need to add it to core/Cargo.toml, like this:
There was a problem hiding this comment.
Thanks I was little bit struggling on how it make on the datafusion level
| #[cfg(feature = "backtrace")] | ||
| #[test] | ||
| #[allow(clippy::unnecessary_literal_unwrap)] | ||
| fn test_enabled_backtrace() { |
There was a problem hiding this comment.
I wonder if we should also add a CI test, perhaps like https://github.com/apache/arrow-datafusion/blob/cde74016e930ffd9c55eed403b84bcd026f38d0f/.github/workflows/rust.yml#L99
|
2 tests failed because the configuration is different between architectures for mac,win Lack of BACKTRACE=1 on mac,win arch expectedly lead the test to fail. @alamb would you mind to help where this set up? |
3 participants
Which issue does this PR close?
Closes #7522.
Rationale for this change
In #7434 the backtrace was introduced, however its expensive to build and should be optional. Introducing a cargo feature for backtrace
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?