Skip to content

ci: Generate protocol_autogen only once in CI - #7918

Open
mathbunnyru wants to merge 1 commit into
XRPLF:developfrom
mathbunnyru:generate_once
Open

ci: Generate protocol_autogen only once in CI#7918
mathbunnyru wants to merge 1 commit into
XRPLF:developfrom
mathbunnyru:generate_once

Conversation

@mathbunnyru

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Instead of generating multiple times, let's do it once.
Also, made it so it doesn't require all the tooling, so it's done much faster, and doesn't require using heavy runners / nor our image.

And fixed a bug, where newly added files will trigger an error, if their generated things weren't added to a repo.

Context of Change

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

@mathbunnyru
mathbunnyru requested a review from a team as a code owner July 30, 2026 18:28
@bthomee
bthomee requested a review from Copilot July 30, 2026 18:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR streamlines CI verification of the generated protocol wrapper classes by introducing a lightweight, standalone CMake “codegen-only” project and a dedicated reusable GitHub Actions workflow, so protocol autogen is generated/checked once per CI run instead of per build variant.

Changes:

  • Add a standalone cmake/codegen CMake project that exposes setup_code_gen and code_gen without configuring the full build/dependencies.
  • Introduce a reusable CI workflow to run code generation and fail with a helpful diff/message when generated files are out of date (including newly added generated files).
  • Update CI orchestration to run the new check once, and update docs to describe the standalone codegen flow.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
include/xrpl/protocol_autogen/README.md Documents the new standalone CMake codegen invocation for regenerating protocol autogen files.
cmake/XrplProtocolAutogen.cmake Makes codegen targets relocatable by deriving repo root from the module location and using it for paths/working directory.
cmake/codegen/CMakeLists.txt Adds a minimal standalone CMake project to expose setup_code_gen / code_gen without the full build.
BUILD.md Documents the standalone codegen project and clarifies CI verifies generated files are up-to-date.
.github/workflows/reusable-check-autogen.yml New reusable workflow that configures/runs standalone codegen and checks for diffs (including new untracked generated files).
.github/workflows/reusable-build-test-config.yml Removes the per-build autogen verification step now handled by the dedicated workflow.
.github/workflows/on-trigger.yml Adds the new reusable autogen check job to scheduled/push/manual trigger workflow.
.github/workflows/on-pr.yml Adds the new reusable autogen check job to PR workflow and includes it in the final dependency aggregation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mathbunnyru
mathbunnyru requested a review from mvadari July 30, 2026 18:36
# so that classes generated for a newly added transaction or ledger
# entry type show up in the diff below rather than silently as an
# empty one.
git add --intent-to-add .

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.

Since checking for new differences is done in multiple workflows, should their logic be identical to each as well?

I'm seeing a new git add --intent-to-add ., which I actually have never seen before. Either way, if this is better than what's done in the other workflows, I'd suggest to make that same change to the other workflows as well.

@mathbunnyru mathbunnyru Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Other workflows don't possibly create new files though, so I don't think we should add it anywhere else

Comment thread BUILD.md

The regenerated files should be committed alongside your changes.
The same targets are also available as a standalone project, which does not
need the dependencies to be configured first:

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.

In terms of configuring dependencies, when a proxy is configured for accessing the Python libraries (as some of us have who work from an office) & connecting to a VPN is required, does that change when using the standalone project approach? Here it says that dependencies do not need to be configured first, so I'm wondering what is really meant by this sentence.

@mathbunnyru mathbunnyru Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works the same way as before, because essentually setup_code_gen implementation hasn't changed.

By dependencies it means C++ dependencies - the standalone project requires 0 knowledge of C++/Conan/compiler and stuff like that

@mathbunnyru
mathbunnyru requested a review from bthomee July 30, 2026 18:50
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@mathbunnyru
mathbunnyru requested a review from a1q123456 August 4, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants