[quality of life] Fix "--all-features" - #1338
[quality of life] Fix "--all-features"#1338Mark Hildebrand (hildebrandmw) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes workspace builds under cargo ... --all-features by removing the diskann-quantization build-time dependency on flatc (previously via the flatbuffers-build feature + build.rs) and shifting FlatBuffers generation to a checked-in helper script. CI is updated to exercise --all-features directly instead of maintaining an explicit features list.
Changes:
- Remove
diskann-quantization’sflatbuffers-buildfeature and deletebuild.rsto eliminateflatcas a build requirement. - Add
diskann-quantization/tools/generate-flatbuffers.shfor regenerating checked-in FlatBuffers bindings, and update crate docs accordingly. - Update CI/nightly workflows to run clippy/tests/docs with
--all-features.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| diskann-quantization/tools/generate-flatbuffers.sh | New script to regenerate checked-in FlatBuffers Rust bindings. |
| diskann-quantization/src/flatbuffers/minmax/quantizer_generated.rs | Regenerated/rewrapped generated FlatBuffers output to match repo formatting conventions for generated files. |
| diskann-quantization/src/flatbuffers.rs | Update module doc to point to the new generation script and clarify why a custom import tree is maintained. |
| diskann-quantization/README.md | Document how to regenerate FlatBuffers bindings and which flatc version to use. |
| diskann-quantization/Cargo.toml | Remove flatbuffers-build feature entry. |
| diskann-quantization/build.rs | Delete build-time flatc compilation/copy step. |
| .github/workflows/nightly.yml | Switch clippy/tests/docs to --all-features. |
| .github/workflows/ci.yml | Switch clippy/tests/docs to --all-features. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| -o "$output" \ | ||
| "$root"/schemas/*.fbs | ||
|
|
||
| rm "$output/mod.rs" |
| cat <<'EOF' | ||
| /* | ||
| * Copyright (c) Microsoft Corporation. | ||
| * Licensed under the MIT license. | ||
| */ | ||
|
|
||
| EOF |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (64.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1338 +/- ##
==========================================
+ Coverage 92.54% 92.55% +0.01%
==========================================
Files 522 521 -1
Lines 99531 99506 -25
==========================================
- Hits 92107 92099 -8
+ Misses 7424 7407 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Building the workspace with
--all-featureshas been broken since open-sourcing due to theflatbuffers-buildfeature indiskann-quantizationrequiring theflatccompiler. This kind of made sense before open-sourcing, but now is problematic since the feature does not actually enable any functionality in the crate.This PR moves the flatbuffer compilation step into a very simple script and removes the build step from
diskann-quantization. The script is intentionally kept pretty dumb (no directory or stale file removal) because (1) I'm afraid of wiping someone's hard drive and (2) updates do not happen frequently).The min/max quantizer was updated to have a formatting similar to the other generated files.
As a consequence, CI has been updated to use
--all-featuresinstead of the (long) explicit list. This actually increases coverage because a few features were missed and means that we no longer need to maintain this list manually.AI disclosure: I still cannot type - an agent generated the new script, updated the README, removed the old
build.rsand updated CI. This is another reason I wanted to keep the script simple.