Skip to content
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

Merged
merged 11 commits into from
Jun 9, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 34 additions & 73 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,31 +108,43 @@ jobs:

# run tests on all workspace members with default feature list
cargo test

# Switch to arrow crate
cd arrow
# re-run tests on arrow crate to ensure
# all arrays are created correctly
cargo test --features=force_validate
cargo test --features=prettyprint
# run test on arrow crate with minimal set of features
cargo test --no-default-features

# re-run tests on arrow crate with all supported features
cargo test -p arrow --features=force_validate,prettyprint
Copy link
Contributor Author

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

Copy link
Contributor

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)


# Test arrow examples
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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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

# Test compilation of arrow library crate with different feature combinations
cargo check -p arrow
cargo check -p arrow --no-default-features

# Test compilation of arrow targets with different feature combinations
Copy link
Contributor Author

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

cargo check -p arrow --all-targets
cargo check -p arrow --no-default-features --all-targets
cargo check -p arrow --no-default-features --all-targets --features test_utils

# Switch to parquet crate
cd ../parquet
# re-run tests on parquet crate with async feature enabled
cargo test --features=async
cargo check --no-default-features
# re-run tests on arrow-flight with all features
cargo test -p arrow-flight --all-features

# Switch to arrow-flight
cd ../arrow-flight
cargo test --features=flight-sql-experimental
cargo check --no-default-features
# re-run tests on parquet crate with all features
cargo test -p parquet --all-features

# Test compilation of parquet library crate with different feature combinations
cargo check -p parquet
cargo check -p parquet --no-default-features
cargo check -p parquet --no-default-features --features arrow

# Test compilation of parquet targets with different feature combinations
cargo check -p parquet --all-targets
cargo check -p parquet --no-default-features --all-targets
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
Copy link
Contributor Author

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


# test the --features "simd" of the arrow crate. This requires nightly.
linux-test-simd:
Expand Down Expand Up @@ -173,12 +185,12 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
cd arrow
cargo test --features "simd"
- name: Check new project build with simd features
- name: Check compilation with simd features
run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd arrow/test/dependency/simd
cargo check
cargo check -p arrow --features simd
cargo check -p arrow --features simd --all-targets

windows-and-macos:
name: Test on ${{ matrix.os }} Rust ${{ matrix.rust }}
Expand Down Expand Up @@ -435,54 +447,3 @@ jobs:
export CARGO_TARGET_DIR="/github/home/target"
export RUSTDOCFLAGS="-Dwarnings"
cargo doc --document-private-items --no-deps --workspace --all-features


# test builds with various feature flag combinations outside the main workspace
default-build:
name: Feature Flag Builds ${{ matrix.rust }}
runs-on: ubuntu-latest
strategy:
matrix:
arch: [amd64]
rust: [stable]
container:
image: ${{ matrix.arch }}/rust
env:
# Disable debug symbol generation to speed up CI build and keep memory down
RUSTFLAGS: "-C debuginfo=0"
steps:
- uses: actions/checkout@v2
- name: Cache Cargo
uses: actions/cache@v3
with:
path: /github/home/.cargo
# this key equals the ones on `linux-build-lib` for re-use
key: cargo-cache3-
- name: Cache Rust dependencies
uses: actions/cache@v3
with:
path: /github/home/target
# this key equals the ones on `linux-build-lib` for re-use
key: ${{ runner.os }}-${{ matrix.arch }}-target-cache3-${{ matrix.rust }}
- name: Setup Rust toolchain
uses: ./.github/actions/setup-builder
with:
rust-version: ${{ matrix.rust }}
- name: Arrow Build with default features
run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd arrow/test/dependency/default-features
cargo check
- name: Arrow Build with default-features=false
run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd arrow/test/dependency/no-default-features
cargo check
- name: Parquet Derive build with default-features
run: |
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd parquet_derive/test/dependency/default-features
cargo check
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ members = [
"arrow-flight",
"integration-testing",
]
resolver = "2"
alamb marked this conversation as resolved.
Show resolved Hide resolved

# this package is excluded because it requires different compilation flags, thereby significantly changing
# how it is compiled within the workspace, causing the whole workspace to be compiled from scratch
Expand Down
15 changes: 15 additions & 0 deletions arrow/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ tempfile = "3"
[[bench]]
name = "aggregate_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "array_from_vec"
Expand All @@ -96,6 +97,7 @@ harness = false
[[bench]]
name = "builder"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "buffer_bit_ops"
Expand All @@ -104,6 +106,7 @@ harness = false
[[bench]]
name = "boolean_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "boolean_append_packed"
Expand All @@ -112,22 +115,27 @@ harness = false
[[bench]]
name = "arithmetic_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "cast_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "comparison_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "filter_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "take_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "length_kernel"
Expand All @@ -140,10 +148,12 @@ harness = false
[[bench]]
name = "sort_kernel"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "partition_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "csv_writer"
Expand All @@ -156,6 +166,7 @@ harness = false
[[bench]]
name = "equal"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "array_slice"
Expand All @@ -164,18 +175,22 @@ harness = false
[[bench]]
name = "concatenate_kernel"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "mutable_array"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "buffer_create"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "substring_kernels"
harness = false
required-features = ["test_utils"]

[[bench]]
name = "array_data_validate"
Expand Down
3 changes: 2 additions & 1 deletion arrow/examples/read_csv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ fn main() {
Field::new("lng", DataType::Float64, false),
]);

let file = File::open("test/data/uk_cities.csv").unwrap();
let path = format!("{}/test/data/uk_cities.csv", env!("CARGO_MANIFEST_DIR"));
let file = File::open(path).unwrap();

let mut csv =
csv::Reader::new(file, Arc::new(schema), false, None, 1024, None, None, None);
Expand Down
6 changes: 5 additions & 1 deletion arrow/examples/read_csv_infer_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ use std::fs::File;
fn main() {
#[cfg(feature = "csv")]
{
let file = File::open("test/data/uk_cities_with_headers.csv").unwrap();
let path = format!(
"{}/test/data/uk_cities_with_headers.csv",
env!("CARGO_MANIFEST_DIR")
);
let file = File::open(path).unwrap();
let builder = csv::ReaderBuilder::new()
.has_header(true)
.infer_schema(Some(100));
Expand Down
21 changes: 0 additions & 21 deletions arrow/test/dependency/README.md

This file was deleted.

30 changes: 0 additions & 30 deletions arrow/test/dependency/default-features/Cargo.toml

This file was deleted.

3 changes: 0 additions & 3 deletions arrow/test/dependency/default-features/src/main.rs

This file was deleted.

30 changes: 0 additions & 30 deletions arrow/test/dependency/no-default-features/Cargo.toml

This file was deleted.

3 changes: 0 additions & 3 deletions arrow/test/dependency/no-default-features/src/main.rs

This file was deleted.

30 changes: 0 additions & 30 deletions arrow/test/dependency/simd/Cargo.toml

This file was deleted.

3 changes: 0 additions & 3 deletions arrow/test/dependency/simd/src/main.rs

This file was deleted.

Loading