Sponsored Content
Skip to content

chore: Add Git information compile-time info to only one file - #6464

Merged
bthomee merged 18 commits into
XRPLF:developfrom
mathbunnyru:build_info
Mar 4, 2026
Merged

chore: Add Git information compile-time info to only one file#6464
bthomee merged 18 commits into
XRPLF:developfrom
mathbunnyru:build_info

Conversation

@mathbunnyru

@mathbunnyru mathbunnyru commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

High Level Overview of Change

Previous approach had one big flaw:

  • The git commit info (GIT_COMMIT_HASH and GIT_BRANCH) is added to every file which is being compiled (https://github.com/search?q=repo%3AXRPLF%2Frippled%20add_definitions&type=code)
  • It probably stopped ccache completely from working on Windows (will check this after this PR is merged)
  • And made it work worse on non-Windows (cache was taken after pre-processor, which also takes time, and not directly).

I also introduce change of behaviour: we require git to be installed, and to build with git information available. It just makes life easier, because reduces number of cases we need to support.

Instead, I added a separate C++ file, where these compile-time variables are propagated and removed all other usages by using functions.
These fixes problems mentioned above.

Note: this PR doesn't change how these variables are calculated.

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

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 review from bthomee and godexsoft March 3, 2026 17:49

@godexsoft godexsoft 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.

:rage1:

@ximinez ximinez 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.

I don't like requiring git, and requiring the repo to be checked out. It seems unnecessarily restrictive to not support building from the source in a zip file, for example. There are places on Github where one can easily do that: Release pages, the "<> Code" button on the main page, etc. There's no reason to lock those options out.

Comment thread .github/workflows/publish-docs.yml
Comment thread src/libxrpl/git/Git.cpp
@codecov

codecov Bot commented Mar 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.8%. Comparing base (595f0dd) to head (f1562e2).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/app/main/Main.cpp 0.0% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #6464     +/-   ##
=========================================
- Coverage     79.8%   79.8%   -0.0%     
=========================================
  Files          858     859      +1     
  Lines        67766   67785     +19     
  Branches      7549    7556      +7     
=========================================
+ Hits         54091   54094      +3     
- Misses       13675   13691     +16     
Files with missing lines Coverage Δ
include/xrpl/beast/core/SemanticVersion.h 100.0% <ø> (ø)
src/libxrpl/beast/core/SemanticVersion.cpp 93.1% <100.0%> (ø)
src/libxrpl/git/Git.cpp 100.0% <100.0%> (ø)
src/libxrpl/protocol/BuildInfo.cpp 98.4% <100.0%> (+0.3%) ⬆️
src/xrpld/app/misc/NetworkOPs.cpp 70.2% <100.0%> (+0.1%) ⬆️
src/xrpld/app/main/Main.cpp 77.8% <0.0%> (ø)

... and 7 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.

@mathbunnyru
mathbunnyru requested a review from ximinez March 4, 2026 15:05
Comment thread src/libxrpl/protocol/BuildInfo.cpp Outdated

@godexsoft godexsoft 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.

:rage1:

@mathbunnyru mathbunnyru 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 Mar 4, 2026

@ximinez ximinez 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.

Thanks for making the changes. This looks great! I can't wait for those faster builds...

@bthomee
bthomee enabled auto-merge (squash) March 4, 2026 19:34
@bthomee
bthomee merged commit c69091b into XRPLF:develop Mar 4, 2026
1 check passed
@mathbunnyru mathbunnyru mentioned this pull request Apr 8, 2026
@mvadari mvadari added this to the 3.2.0 milestone May 20, 2026
beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026
…6464)

The existing code added the git commit info (`GIT_COMMIT_HASH` and `GIT_BRANCH`) to every file, which was a problem for leveraging `ccache` to cache build objects. This change adds a separate C++ file from where these compile-time variables are propagated to wherever they are needed. A new CMake file is added to set the commit info if the `git` binary is available.
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