Skip to content

Commit

Permalink
fix: properly gate things by features & test for that (#1494)
Browse files Browse the repository at this point in the history
Ideally we would test every combination of features, but at the very
least, we should verify that no features, default features, and all
features pass clippy.

The two main changes:

1. In vortex-io, we need a syntactically valid expressoin when neither
compio nor tokio are available.
2. In vortex-array, we need both flatbuffers and flexbuffers for views
to work. I removed the optional dependencies on these libraries. In
theory, someone could go through our code base and properly feature flag
array views, but that began to feel onerous as I did it.
  • Loading branch information
danking authored Nov 27, 2024
1 parent 0095a0c commit e452cbf
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 20 deletions.
14 changes: 13 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,20 @@ jobs:
- uses: ./.github/actions/setup-rust
- name: Rust Lint - Format
run: cargo fmt --all --check
- name: Rust Lint - Clippy
- name: Rust Lint - Clippy All Features
run: cargo clippy --all-features --all-targets
- name: Rust Lint - Clippy Default Features
run: cargo clippy --all-targets
- name: Rust Lint - Clippy No Default Features
run: |
set -ex
# https://spiraldb.slack.com/archives/C07BV3GKAJ2/p1732736281946729
for package in $(cargo check -p 2>&1 | grep '^ ')
do
echo ---- $package ----
cargo clippy --package $package --no-default-features
done
- name: Rust Test
run: cargo test --workspace --all-features

Expand Down
24 changes: 6 additions & 18 deletions vortex-array/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ arrow-select = { workspace = true }
bytes = { workspace = true }
enum-iterator = { workspace = true }
enum-map = { workspace = true }
flatbuffers = { workspace = true, optional = true }
flexbuffers = { workspace = true, optional = true }
flatbuffers = { workspace = true }
flexbuffers = { workspace = true }
futures-util = { workspace = true }
hashbrown = { workspace = true }
humansize = { workspace = true }
Expand All @@ -48,25 +48,13 @@ serde = { workspace = true, features = ["derive"] }
static_assertions = { workspace = true }
vortex-buffer = { workspace = true }
vortex-datetime-dtype = { workspace = true }
vortex-dtype = { workspace = true }
vortex-error = { workspace = true }
vortex-flatbuffers = { workspace = true, optional = true }
vortex-scalar = { workspace = true }
vortex-dtype = { workspace = true, features = ["flatbuffers", "serde"] }
vortex-error = { workspace = true, features = ["flatbuffers", "flexbuffers"] }
vortex-flatbuffers = { workspace = true, features = ["array"] }
vortex-scalar = { workspace = true, features = ["flatbuffers", "serde"] }

[features]
default = ["flatbuffers", "serde"]
arbitrary = ["dep:arbitrary", "vortex-dtype/arbitrary"]
flatbuffers = [
"dep:flatbuffers",
"dep:flexbuffers",
"dep:vortex-flatbuffers",
"vortex-flatbuffers/array",
"vortex-dtype/flatbuffers",
"vortex-error/flatbuffers",
"vortex-error/flexbuffers",
"vortex-scalar/flatbuffers",
]
serde = ["vortex-dtype/serde", "vortex-scalar/serde"]

[target.'cfg(target_arch = "wasm32")'.dependencies]
# Enable the JS feature of getrandom (via rand) to supprt wasm32 target
Expand Down
4 changes: 3 additions & 1 deletion vortex-io/src/dispatcher/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ mod tokio;
use std::future::Future;

use futures::channel::oneshot;
#[cfg(not(any(feature = "compio", feature = "tokio")))]
use vortex_error::vortex_panic;
use vortex_error::VortexResult;

#[cfg(feature = "compio")]
Expand Down Expand Up @@ -73,7 +75,7 @@ impl Default for IoDispatcher {
#[cfg(all(feature = "compio", not(feature = "tokio")))]
return Self(Inner::Compio(CompioDispatcher::new(1)));
#[cfg(not(any(feature = "compio", feature = "tokio")))]
return Self(Inner {});
vortex_panic!("must enable one of compio or tokio to use IoDispatcher");
}
}

Expand Down

0 comments on commit e452cbf

Please sign in to comment.