Embed code generators into stellar-xdr CLI#541
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c93e7922b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| xdr-generator-rust = { version = "0.1.0", path = "xdr-generator-rust/generator", optional = true } | ||
| generator-definitions-json = { version = "0.1.0", path = "xdr-generator-rust/generator-definitions-json", optional = true } |
There was a problem hiding this comment.
Publish the generator crates before enabling them
When this crate is packaged for crates.io, Cargo resolves path dependencies that also specify a version from the registry rather than from the packaged subdirectory. These new optional dependencies are enabled by cli, and the existing release workflow packages the crate and then builds the packaged copy with --features cli, while the README advertises cargo install stellar-xdr --features cli; both paths will fail unless xdr-generator-rust and generator-definitions-json are published to crates.io in the right order. Please either publish/include these crates as real registry dependencies or keep the CLI generators inside the main package without registry-only path deps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR embeds the existing XDR code generators into the stellar-xdr CLI (under xfile) by converting the generator crates from standalone binaries into libraries and wiring them as optional path dependencies behind the cli feature, so downstream users can generate Rust code and a JSON AST using the single stellar-xdr binary.
Changes:
- Convert
xdr-generator-rust/generatorandxdr-generator-rust/generator-definitions-jsonfrom bin crates into lib crates exposinggenerate(...). - Add
stellar-xdr xfile generate-rustandstellar-xdr xfile astCLI subcommands to invoke those library entry points. - Update the
Makefileto drive generation viacargo run --features cli -- xfile ...instead ofcargo run --manifest-path ....
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| xdr-generator-rust/generator/src/main.rs | Removes the old standalone generator CLI entry point. |
| xdr-generator-rust/generator/src/lib.rs | Adds library API (generate) for Rust code generation. |
| xdr-generator-rust/generator/Cargo.toml | Switches the generator crate from [[bin]] to [lib] and drops clap. |
| xdr-generator-rust/generator-definitions-json/src/lib.rs | Refactors JSON AST emitter into a library generate entry point. |
| xdr-generator-rust/generator-definitions-json/Cargo.toml | Switches the JSON generator crate from [[bin]] to [lib] and drops clap. |
| xdr-generator-rust/Cargo.lock | Updates inner workspace lockfile to reflect removal of clap and bin targets. |
| src/cli/xfile/mod.rs | Registers new xfile subcommands and error variants. |
| src/cli/xfile/generate_rust.rs | Adds CLI wrapper for Rust generator library. |
| src/cli/xfile/ast.rs | Adds CLI wrapper for JSON AST generator library. |
| Makefile | Routes generation targets through the stellar-xdr CLI subcommands. |
| Cargo.toml | Adds optional path deps on generator libraries and includes them in the cli feature. |
| Cargo.lock | Pulls generator-library dependency graph into the main lockfile under cli. |
Comments suppressed due to low confidence (1)
Makefile:45
- The Makefile now uses
cargo run --features clito perform generation, which requires compiling thestellar-xdrcrate first. Sincestellar-xdr's library compilation depends onsrc/generated.rs(viamod generated;), runningmake clean(which deletessrc/generated.rs/src/generated/) will preventmake generatefrom bootstrapping generation from a clean state.
Consider either (a) keeping a standalone generator binary for bootstrapping, (b) making clean stop removing src/generated.rs/src/generated/, or (c) adding a dedicated small generation-only crate/binary that doesn’t depend on the generated module but still drives generation through the same library APIs.
cargo run --features cli -- xfile generate-rust \
$(addprefix --input ,$(sort $(wildcard xdr/*.x))) \
--output $@ \
--custom-default $(CUSTOM_DEFAULT_IMPL) \
--custom-str $(CUSTOM_STR_IMPL)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
587fc36 to
372f4ba
Compare
What
Embed the two XDR code generators into the
stellar-xdrCLI as new subcommands under the existingxfilegroup:stellar-xdr xfile generate-rustproducessrc/generated.rs(and its per-type directory) andstellar-xdr xfile astproduces the JSON definitions AST. The standalonexdr-generator-rustandgenerator-definitions-jsoncrates are converted from binaries into libraries exposing agenerate(...)entry point, wired into the main crate as optional path dependencies enabled only by theclifeature, and the Makefile now drives generation through the CLI instead ofcargo run --manifest-path.Why
Code generation from
.xfiles was only reachable through two binaries that live in an inner workspace, so a downstream consumer had to know about that workspace andcargo install --git … --bin …the specific generator they wanted rather than reaching for thestellar-xdrCLI they already use for the repository's other XDR tooling. Surfacing both generators asxfilesubcommands puts code generation alongside the existingtypes,decode,encode,compare, andxfile preprocesscommands under the single binary, and removes the need to expose the inner workspace to anyone who just wants to generate code.Known limitations
Behavior is unchanged: the new subcommands and library functions reproduce the previous binaries' semantics exactly (input reading and sort-by-path, parsing, options/feature construction, and output paths), with regeneration verified to produce byte-identical
src/generated.rs,src/generated/, andxdr-definitions-json/xdr.json. The only dropped behavior is the binaries' cosmetic stderr progress lines. The generators still live underxdr-generator-rust/as libraries.Close #527
Generated by Claude Code