Move protobuf and flatbuffer definitions into their owning crates - #9550
Conversation
AdamGS
left a comment
AdamGS
left a comment
There was a problem hiding this comment.
I remember some release issue with flatbuffers and build.rs based setup, are we good now?
|
the problem with releases was that we tried to discover package dependencies during build but |
66222b7 to
bea206a
Compare
Merging this PR will regress 3 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress[u64, (4000, 1024)] |
71.8 µs | 86 µs | -16.47% |
| ❌ | WallTime | filtered_owned_i64_avx512[OneNullInEight] |
22.3 µs | 26.1 µs | -14.51% |
| ❌ | WallTime | mul_u32_nonnull_avx512 |
5.6 µs | 6.3 µs | -11.54% |
| ⚡ | WallTime | dict_canonicalize_gt_u8_avx2[16000000] |
11.1 ms | 7.1 ms | +57.38% |
| ⚡ | WallTime | mul_u64_nonnull_neon |
20.7 µs | 15.5 µs | +33.5% |
| ⚡ | WallTime | filtered_owned_i64_avx2[OneNullInEight] |
26 µs | 22.4 µs | +15.68% |
| ⚡ | WallTime | multiply_shapes_neon[(16384, PerRowPerRow)] |
20.2 µs | 17.5 µs | +15.57% |
| ⚡ | WallTime | mul_i64_nonnull_neon |
20.1 µs | 17.5 µs | +14.78% |
| ⚡ | WallTime | words_gather_scalar_avx2[65536] |
9.4 µs | 8.2 µs | +13.79% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/protobuf-codegen-build-rs-s897y8 (f1ebf01) with develop (f9093df)
Footnotes
-
176 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
1 benchmark was run, but is now archived. If it was deleted in another branch, consider rebasing to remove it from the report. Instead if it was added back, click here to restore it. ↩
04ff00b to
514b12f
Compare
AdamGS
left a comment
AdamGS
left a comment
There was a problem hiding this comment.
LGTM, love the line count, just one small questions.
|
Will merge this after release |
514b12f to
b06447c
Compare
Closes #1906. The `.fbs` and `.proto` schemas all lived in `vortex-flatbuffers` and `vortex-proto`, away from the types they describe, with their generated Rust checked in. Build-time generation existed before #557 but was removed because `vortex-build` discovered include paths by walking workspace metadata with `cargo_metadata` and reaching outside the package directory, neither of which survives packaging, so publishing broke. Declaring the dependencies explicitly avoids that. Each schema moves to the crate that owns the types it describes, and is compiled into `OUT_DIR` by that crate's `build.rs`. Nothing generated is checked in: | Schema | Crate | Module | | -------------------------- | --------------- | ---------------------------- | | `array.fbs`, `dtype.fbs` | `vortex-array` | `vortex_array::flatbuffers` | | `dtype/scalar/expr.proto` | `vortex-array` | `vortex_array::proto` | | `layout.fbs` | `vortex-layout` | `vortex_layout::flatbuffers` | | `footer.fbs` | `vortex-file` | `vortex_file::flatbuffers` | | `message.fbs` | `vortex-ipc` | `vortex_ipc::flatbuffers` | `vortex-proto` and `vortex-flatbuffers` are both removed. The FlatBuffers read/write traits move into `vortex_array::flatbuffers` alongside the generated array and dtype bindings, which every crate that used them already depended on. The new `vortex-build` crate holds the shared build script helpers, and `xtask` is deleted since code generation was its only job. A crate whose schemas include another crate's names it explicitly: vortex_build::flatbuffers() .depends_on("vortex-array") .compile(&["vortex-serde/message.fbs"]); `depends_on` resolves the dependency's schema directory through Cargo's `links` metadata, so a path dependency in the workspace and a package unpacked from a registry behave identically. `flatc`'s `--include-prefix` points cross-crate includes at a small `deps` module each consuming crate provides by hand, so no schema is compiled twice. The issue suggests generating into `src/flatbuffers/` and git-ignoring it. Generating into `OUT_DIR` instead keeps the same property — no generated code in git — without a build script writing into its own package, which would invalidate the registry checksum for anyone building a published crate. `.proto` compilation uses `protox` rather than `protoc`, so protobuf codegen needs no external tooling; the generated output is byte-identical to what was checked in. `.fbs` compilation shells out to `flatc`, which must be on `PATH` or named by `FLATC`, so CI installs it as part of the shared Rust setup and the musl job pulls it from Alpine. The flatbuffer back-compat check now flattens each revision's per-crate schema directories into one tree before running `flatc --conform`. `vortex_proto::{dtype, scalar, expr}` becomes `vortex_array::proto::*`, and both the flatbuffer traits and the generated modules move from `vortex_flatbuffers::*` to `vortex_array::flatbuffers` and the crates listed above. The `vortex` facade keeps `vortex::proto` and `vortex::flatbuffers` pointing at the same items, and `vortex_array:: dtype`'s `proto` and `flatbuffers` re-exports are unchanged. Building any Vortex crate from source now requires `flatc`, including for downstream consumers and docs.rs. Signed-off-by: Robert Kruszewski <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zTmZ6ANxESomTnvkvk85t
b06447c to
f1ebf01
Compare
2 participants
Instead of checking in the generated code in centralised crates and reexporting it we generate flatbuffer and protobuffer objects during build.
We remove the xtask module as it no longer has any tasks.
Fix #1906