Skip to content

chore: Move codegen venv setup into build stage#6617

Merged
ximinez merged 13 commits intoXRPLF:developfrom
kuznetsss:Speed_up_compilation
Apr 15, 2026
Merged

chore: Move codegen venv setup into build stage#6617
ximinez merged 13 commits intoXRPLF:developfrom
kuznetsss:Speed_up_compilation

Conversation

@kuznetsss
Copy link
Copy Markdown
Contributor

@kuznetsss kuznetsss commented Mar 23, 2026

High Level Overview of Change

This PR moves python venv setup into build stage. Also move venv setup into python code so venv is created only when generation is needed.

Context of Change

Python venv setup is relatively slow so moving it into build stage helps build system to do something else in parallel while python dependencies are being downloaded.

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)

@kuznetsss kuznetsss requested a review from a1q123456 March 23, 2026 14:37
Comment thread scripts/list_outputs.py Outdated
@kuznetsss kuznetsss requested a review from mathbunnyru March 23, 2026 15:34
@kuznetsss kuznetsss force-pushed the Speed_up_compilation branch from 943f911 to bb3c7f0 Compare March 23, 2026 15:40
@kuznetsss kuznetsss changed the title chore: Move venv setup into build stage chore: Move codegen venv setup into build stage Mar 23, 2026
@kuznetsss kuznetsss marked this pull request as ready for review March 23, 2026 17:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.4%. Comparing base (6e24522) to head (c5aeb62).
⚠️ Report is 24 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6617     +/-   ##
=========================================
- Coverage     81.4%   81.4%   -0.0%     
=========================================
  Files          999     999             
  Lines        74423   74423             
  Branches      7557    7553      -4     
=========================================
- Hits         60613   60606      -7     
- Misses       13810   13817      +7     

see 5 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

A few comments, nothing major

Comment thread scripts/list_outputs.py Outdated
Comment thread scripts/list_outputs.py Outdated
Comment thread scripts/list_outputs.py Outdated
Comment thread scripts/list_outputs.py Outdated
@a1q123456
Copy link
Copy Markdown
Contributor

a1q123456 commented Mar 24, 2026

I think there're a couple of things we can do

  1. remove the macro files from the reconfigure dependency list so that we don't reconfigure automatically after the macro files get changed
  2. generate a stamp file on the first time when we configure (when the file does not exist) to say we don't need to regenerate
  3. when we build the project, we check if the stamp file is up to date, call the scripts and update the stamp file if not

Because currently, cmake still regenerates even if we touch the macro files and we want to avoid regeneration as much as possible.

@a1q123456
Copy link
Copy Markdown
Contributor

a1q123456 commented Mar 25, 2026

I'm just thinking, does it make sense to call list_output each time when we run cmake?

I think what we expect is:

  1. if the macro files haven't been updated - we don't regenerate
  2. if the macro files have been updated by git - we don't regenerate
  3. if the macro files have been modified by the current develop - we generate

but what we're doing currently is:

  1. if the macro files haven't been updated - we don't regenerate
  2. if the macro files have been updated - we reconfigure, call list_output.py, and regenerate

Would it be easier to have a stamp file containing the hash of the script file, the template files, and the macro files, and then, we check the stamp file into git, and we only generate when the hash is different?

Doing so means that:

  1. when git pulls a commit that modifies the macro files, the stamp file must be in the commit as well - we don't regenerate
  2. when the developer modifies a macro file, cmake checks the hash and decides to regenerate

I think this is the safest way

@github-actions
Copy link
Copy Markdown

This PR has conflicts, please resolve them in order for the PR to be reviewed.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

@a1q123456
Copy link
Copy Markdown
Contributor

following up the discussion, we'll move the code generation process outside of cmake.

Comment thread .github/workflows/reusable-build-test-config.yml
Comment thread include/xrpl/protocol_autogen/README.md Outdated
a1q123456 and others added 2 commits April 1, 2026 19:07
Co-authored-by: Bart <bthomee@users.noreply.github.com>
@a1q123456 a1q123456 requested a review from bthomee April 1, 2026 18:15
Copy link
Copy Markdown
Collaborator

@bthomee bthomee left a comment

Choose a reason for hiding this comment

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

I'm somewhat concerned it will be too confusing for developers to keep track of which cmake commands they need to run and when.

I also wonder if it will work fine in IDEs - after I run the conan install command, my IDE for instance will automatically pick up the CMake profile and configure everything; all I need to do then is build the code. Will my workflow have to change? Do I need to remember to run setup_code_gen and/or code_gen each time I switch branches, for instance?

@a1q123456
Copy link
Copy Markdown
Contributor

a1q123456 commented Apr 1, 2026

I'm somewhat concerned it will be too confusing for developers to keep track of which cmake commands they need to run and when.

I also wonder if it will work fine in IDEs - after I run the conan install command, my IDE for instance will automatically pick up the CMake profile and configure everything; all I need to do then is build the code. Will my workflow have to change? Do I need to remember to run setup_code_gen and/or code_gen each time I switch branches, for instance?

I'm somewhat concerned it will be too confusing for developers to keep track of which cmake commands they need to run and when.

I also wonder if it will work fine in IDEs - after I run the conan install command, my IDE for instance will automatically pick up the CMake profile and configure everything; all I need to do then is build the code. Will my workflow have to change? Do I need to remember to run setup_code_gen and/or code_gen each time I switch branches, for instance?

No, you only have to run cmake --build . --target code_gen when you update the macro files or the template files, and the ci pipeline will ask you to do it if you update those files but forget to run the scripts

@a1q123456 a1q123456 added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Apr 1, 2026
@a1q123456 a1q123456 added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Apr 15, 2026
@ximinez ximinez added this pull request to the merge queue Apr 15, 2026
Merged via the queue into XRPLF:develop with commit d52d735 Apr 15, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants