Skip to content

buffa-build: shared_descriptor_pool with include_bytes! sidecar - #369

Merged
iainmcgin merged 3 commits into
anthropics:mainfrom
hcrosse:feat/shared-pool-buffa-build
Aug 29, 2026
Merged

buffa-build: shared_descriptor_pool with include_bytes! sidecar#369
iainmcgin merged 3 commits into
anthropics:mainfrom
hcrosse:feat/shared-pool-buffa-build

Conversation

@hcrosse

@hcrosse hcrosse commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Part of #310. Third of the three PRs splitting that work by crate (1: the buffa-codegen mechanism, #311, merged; 2: the plugin path, in a sibling PR).

What this does

Adds buffa_build::Config::shared_descriptor_pool(bool), the build.rs front end for the mechanism #311 merged. When enabled, compile() writes the full-closure FileDescriptorSet once as a binary sidecar next to the generated tree (<include-file-stem>.descriptor_set.binpb, via the existing write_if_changed) and prepends the shared __buffa_fds root module to the include file using buffa_codegen::shared_descriptor_root_module with FdsEmbedding::Sidecar — so the descriptor bytes never expand into Rust source at all, and every package's descriptor_pool() / FILE_DESCRIPTOR_SET_BYTES delegates to the one shared copy.

The sidecar's include_bytes! form follows the same relative_includes selection the module tree already uses (include!-relative for an explicit out_dir, concat!(env!("OUT_DIR"), ...) otherwise), so the pair always resolves together. Deriving the sidecar name from the include-file stem keeps two compile() calls sharing an out_dir from clobbering each other's descriptor set.

Prerequisites are validated up front with buffa-build-shaped errors: reflection must be enabled, and include_file must be set with a usable file-name stem (the shared module needs a tree root to live in, and the sidecar is named after the stem — a stemless name like ".." would otherwise write a stray misnamed sidecar before the include-file write fails). Checks run in that order so the error names the first actually-missing prerequisite.

The include file keeps its // @generated marker on line 1: the shared root module is spliced between the header and the include! items, mirroring the packaging plugin's placement on the plugin path, so first-line generated-file detection (rustfmt's @generated window, diff-collapse heuristics) keeps working on checked-in trees.

Because the shared root module comes from the merged mechanism, it inherits #311's review hardening as-is: the bounded decode_with_options (len()*64 floored at DEFAULT_ELEMENT_MEMORY_LIMIT) rather than the untrusted-input default.

Testing

  • Happy path through compile() using the descriptor_set() source (no protoc): asserts the sidecar is written with the exact encoded bytes, the include file hosts __buffa_fds and include_bytes!s the sidecar rather than inlining a byte literal, and the package stitcher delegates instead of embedding its own copy.
  • Rejection tests for each up-front rejection: missing reflection, missing include_file, and an include_file without a file-name stem (which would otherwise write a stray misnamed sidecar before the include-file write fails).
  • The consumer-compile + Arc::ptr_eq pool-identity evidence for exactly this embedding lives in the #[ignore]d shared_pool_tree_compiles_include_bytes_sidecar fixture merged with reflect: shared descriptor pool mechanism (opt-in) #311 (passes against this branch).
  • $OUT_DIR mode end-to-end in buffa-test: its build script compiles a two-package proto set with shared_descriptor_pool(true) and no explicit out_dir (so cargo's real OUT_DIR drives both the sidecar write and the emitted concat!(env!("OUT_DIR"), ...) reference), the crate include!s the result, and tests assert Arc::ptr_eq pool identity across packages plus cross-package symbol resolution. This covers the composition a unit test can't safely exercise (setting OUT_DIR races under the multithreaded test runner).

Docs: the Config::shared_descriptor_pool rustdoc names the sidecar file and the commit-it-with-a-checked-in-out_dir guidance, compile()'s # Errors lists the new rejections, and the guide's Runtime reflection section gains a shared-pool paragraph (one pool instance across packages, sidecar semantics, the set-it-uniformly caveat).

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@hcrosse
hcrosse marked this pull request as ready for review August 25, 2026 02:34
@hcrosse
hcrosse force-pushed the feat/shared-pool-buffa-build branch 2 times, most recently from e99b93c to 8c81700 Compare August 25, 2026 02:42
Adds buffa_build::Config::shared_descriptor_pool. buffa-build emits the shared
__buffa_fds root module into the include file and writes the descriptor set once
as a binary sidecar (named from the include-file stem so parallel compile()
calls don't clobber) that the module include_bytes!s - keeping the bytes out of
the generated Rust source entirely, on top of the cross-package dedup.

- FdsEmbedding::Sidecar drives the include_bytes! path (relative or OUT_DIR)
- errors without reflection, and without include_file, checked up front
- feature overrides are threaded through so the shared bytes match generate's
  embedding (buffa-build is one process, unlike the plugin path)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@hcrosse
hcrosse force-pushed the feat/shared-pool-buffa-build branch from 8c81700 to 6d92c60 Compare August 25, 2026 02:48
…pool and thread the sidecar name through validation
@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code]

Reviewed with the code and API-ergonomics reviewers. The mechanics are right: the sidecar path and the emitted include_bytes! are driven by the same relative_includes switch so they cannot diverge in either mode, the descriptor set is the same full closure with the same feature-override transform the non-shared path applies, write_if_changed keeps mtimes stable and include_bytes! lands in rustc's dep-info so no rerun-if-changed is owed, and the 17 commits on main since your base do not touch these hunks (#361 actually makes the test's negative assertion stronger).

I pushed one fixup commit (611aaae) rather than round-tripping, all doc and comment level plus two small code items:

  • The one thing a build.rs author could get wrong silently: in shared mode the tree has to be consumed through the include file, because each package delegates to __buffa_fds by a fixed number of super:: hops. Keeping the per-package buffa::include_proto! pattern (which the guide teaches as the default) fails with an unresolved path inside generated code, and two shared-pool compile() calls included at the same scope collide on __buffa_fds. The setter doc and the guide now say both, plus the bare-file-name requirement and a .gitattributes note for the committed sidecar.
  • The splice comment cited "the packaging plugin's splice", which is codegen: wire shared_descriptor_pool through the plugins #370, not the tree; it now states the invariant that makes the splice legal (generate_include_file passes emit_inner_allow = false) with a debug_assert! guarding it.
  • CodeGenConfig::shared_descriptor_pool's rustdoc said "no effect without generate_reflection" while generate hard-errors, and named the packaging plugin as a front-end before codegen: wire shared_descriptor_pool through the plugins #370 exists; both corrected. codegen: wire shared_descriptor_pool through the plugins #370 will want to re-add the plugin wording when it lands.
  • The include-file stem is now computed once in the validation block and threaded to the write site as an Option, replacing the recomputation plus expect.
  • packages_share_one_pool_instance also asserts core::ptr::eq on the two FILE_DESCRIPTOR_SET_BYTES, which is the property the feature exists for.

Left for follow-ups rather than this PR: an include_file with a directory component passes the stem check but writes the sidecar beside the include file only in $OUT_DIR mode (the doc now requires a bare name), and the emitted __buffa_fds module carries no #[allow] unlike its sibling modules. Will approve and queue once CI is green on the new head.

@iainmcgin iainmcgin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[claude code]

Approving on Iain's instruction. Sidecar path and emitted include_bytes! are driven by the same switch in both modes, the descriptor closure matches the non-shared path including feature overrides, fingerprinting needs nothing extra, and the buffa-test $OUT_DIR run proves pool identity and now byte identity across packages. 11/11 green on b8c69b9.

@iainmcgin
iainmcgin added this pull request to the merge queue Aug 29, 2026
Merged via the queue into anthropics:main with commit 4134998 Aug 29, 2026
11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 29, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants