-
Notifications
You must be signed in to change notification settings - Fork 721
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
Change to use resolver v2
, test more feature flag combinations in CI, fix errors (#1630)
#1822
Conversation
376f38e
to
75f9b49
Compare
cargo test --no-default-features | ||
|
||
# re-run tests on arrow crate with all supported features | ||
cargo test -p arrow --features=force_validate,prettyprint |
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 merged these two test runs together as I couldn't see an obvious reason to separate them, and this will make CI slightly faster
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 think the original rationale was to try and test without the default features (mostly I think to try and catch build errors, which this PR does in another way)
.github/workflows/rust.yml
Outdated
cargo run --example builders | ||
cargo run --example dynamic_types | ||
cargo run --example read_csv | ||
cargo run --example read_csv_infer_schema | ||
cargo check --no-default-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.
I moved the cargo check
logic into here as opposed to a separate job so it can benefit from the compilation already performed above.
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.
Maybe it would be worth making a separate named run step (as is done https://github.com/apache/arrow-rs/pull/1822/files#diff-73e17259d77e5fbef83b2bdbbe4dc40a912f807472287f7f45b77e0cbf78792dR188)
- name: Check compilation with simd features
So when/if this check fails it will be easier to figure out what is wrong
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.
Working on this, running into nonsense with environment variables and caching
@@ -135,6 +135,7 @@ macro_rules! eof_err { | |||
($fmt:expr, $($args:expr),*) => (ParquetError::EOF(format!($fmt, $($args),*))); | |||
} | |||
|
|||
#[cfg(any(feature = "arrow", test))] |
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.
This removes a warning
@@ -27,7 +27,7 @@ use crate::data_type::{ByteArray, Decimal, Int96}; | |||
use crate::errors::{ParquetError, Result}; | |||
use crate::schema::types::ColumnDescPtr; | |||
|
|||
#[cfg(feature = "cli")] | |||
#[cfg(any(feature = "cli", test))] |
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.
Drive by refactor to make consistent with rest of crate which tests everything
cargo check -p parquet --no-default-features --features arrow --all-targets | ||
|
||
# Test compilation of parquet_derive macro with different feature combinations | ||
cargo check -p parquet_derive |
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.
To be completely honest I'm not sure why we were testing this, parquet_derive doesn't have any dev-dependencies or feature flags, but I guess it can't hurt to be thorough
.github/workflows/rust.yml
Outdated
cargo check -p arrow | ||
cargo check -p arrow --no-default-features | ||
|
||
# Test compilation of arrow targets with different feature combinations |
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.
It would theoretically be more correct to test each target individually, so that feature resolution is not impacted by other targets, in practice this is probably good enough
@@ -39,38 +39,44 @@ brotli = { version = "3.3", optional = true } | |||
flate2 = { version = "1.0", optional = true } | |||
lz4 = { version = "1.23", optional = true } | |||
zstd = { version = "0.11.1", optional = true, default-features = false } | |||
chrono = { version = "0.4", default-features = false } | |||
chrono = { version = "0.4", default-features = false, features = ["alloc"] } |
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.
This is the actual fix for #1630
Unfortunately this is required by the record API which is tightly coupled with the file APIs, and so there isn't an easy way to hide this behind a feature flag. Perhaps something for another day, I'm a bit feature flagged out 😆
Codecov Report
@@ Coverage Diff @@
## master #1822 +/- ##
==========================================
+ Coverage 83.44% 83.54% +0.10%
==========================================
Files 199 200 +1
Lines 56651 56798 +147
==========================================
+ Hits 47272 47452 +180
+ Misses 9379 9346 -33
Continue to review full report at Codecov.
|
resolver v2
, test more feature flag combinations in CI, fix errors (#1630)
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 idea looks great - thanks @tustvold
I want to review the actual test output before I approve this PR and it still appears to be running
cargo test --no-default-features | ||
|
||
# re-run tests on arrow crate with all supported features | ||
cargo test -p arrow --features=force_validate,prettyprint |
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 think the original rationale was to try and test without the default features (mostly I think to try and catch build errors, which this PR does in another way)
.github/workflows/rust.yml
Outdated
cargo run --example builders | ||
cargo run --example dynamic_types | ||
cargo run --example read_csv | ||
cargo run --example read_csv_infer_schema | ||
cargo check --no-default-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.
Maybe it would be worth making a separate named run step (as is done https://github.com/apache/arrow-rs/pull/1822/files#diff-73e17259d77e5fbef83b2bdbbe4dc40a912f807472287f7f45b77e0cbf78792dR188)
- name: Check compilation with simd features
So when/if this check fails it will be easier to figure out what is wrong
Setting as draft whilst I try to get CI happy |
9b0324a
to
b701d2b
Compare
b701d2b
to
ed413af
Compare
I had to rework the caching to allow splitting up the stages into smaller steps, without having to duplicate "export CARGO_HOME". into every step. Setting these variable at the top-level caused rustup to fail to install correctly, and so rather than using |
68d1441
to
7142dff
Compare
Don't install unused components
7142dff
to
89c0429
Compare
Here is an example of it restoring from a cache key (without lockfile suffix), and then publishing the lockfile specific cache key 🎉 This then gets picked up by https://github.com/apache/arrow-rs/runs/6817593817?check_suite_focus=true |
MIRI appears to be unhappy... 👀 |
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.
Beautiful
Appears MIRI version is too old to supposed namespaced deps - rust-lang/cargo#5565 Attempting to update it in - #1828 |
Thank you @tustvold |
Which issue does this PR close?
Closes #1630
Rationale for this change
We continue to have issues with various feature flag combinations resulting in compile errors. A particularly pernicious variant of this occurs you need to enable a feature of an optional dependency for tests, for example, arrow prettyprint within parquet. To do this you add the dependency as a dev-dependency, with the feature enabled.
However, the way feature resolution traditionally works, is that even when building the library crate on its own, it would take into account features enabled by the dev-dependencies. This would mask issues with feature flags. The previous solution to this has been to have
test/dependency
crates that simply depend on the library, without the dev-dependencies.What changes are included in this PR?
rust-lang/cargo#7916 added an option to skip using dev-dependencies when resolving features, and this was stabilised in feature resolver v2 which was released in Rust 1.51. In particular with the new feature resolver
This PR therefore:
test/dependency
workaroundThanks to @carols10cents for the pointer ❤️
Are there any user-facing changes?
No