Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve git commit hash lookup #5225

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Improve git commit hash lookup #5225

wants to merge 4 commits into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Dec 17, 2024

High Level Overview of Change

Changes the lookup of the git hash included in rippled's version number (for debug builds) to use rev-parse instead of describe, plus a few more improvements.

  • Also get the branch name.
  • Return the git hash and branch name in server_info for admin connections.
  • Include git hash and branch name on separate lines in --version.

Context of Change

Commit bf013c0 back in 2021 used git describe to find the hash of the commit being built when building rippled.

From the description of git describe:

The command finds the most recent tag that is reachable from a commit. If the tag points to the commit, then only the tag is shown. Otherwise, it suffixes the tag name with the number of additional commits on top of the tagged object and the abbreviated object name of the most recent commit. The result is a "human-readable" object name which can also be used to identify the commit to other git commands.

This sometimes results in rippled version numbers that look like:

rippled version 2.4.0-b1+ppe-build-175-g49e0d54c76be6323f7932386973e38b490e18eda.DEBUG

We want version numbers that look like:

rippled version 2.4.0-b1+49e0d54c76be6323f7932386973e38b490e18eda.DEBUG

In other words, without ppe-build-175-g, indicating that the commit is 175 commits after the ppe-build tag, particularly since that tag is on version 2.1.0-rc1 from February.

This changes the lookup to use git rev-parse instead, which always just returns a hash.

While I was in there, I also added code to find the branch name, and included the hash and branch name in the admin-only result of server_info, and the output of --version, so this info can be found on release builds as well.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)

API Impact

  • Public API: New feature (new methods and/or new fields)

Before / After

rippled --version

Before:

rippled --version on a debug build may return something like

rippled version 2.4.0-b1+ppe-build-175-g49e0d54c76be6323f7932386973e38b490e18eda.DEBUG

After:

rippled --version on a debug build will return something like

rippled version 2.4.0-b1+c14acfc32b3375427405372d742b5b72f3c430dc.DEBUG
Git commit hash: c14acfc32b3375427405372d742b5b72f3c430dc
Git build branch: pr/gch

And on a release build will return something like

rippled version 2.4.0-b1
Git commit hash: c14acfc32b3375427405372d742b5b72f3c430dc
Git build branch: pr/gch

server_info

Before:

server_info does not include any git information.

After:

server_info on an admin connection will include git info:

  [...]
  "complete_ledgers": "92826571-92837212",
  "git": {
    "branch": "pr/gch",
    "hash": "c14acfc32b3375427405372d742b5b72f3c430dc"
  },
  "hostid": "HOST",
  [...]

* Also get the branch name.
* Use rev-parse instead of describe to get a clean hash.
* Return the git hash and branch name in server_info for admin
  connections.
* Include git hash and branch name on separate lines in --version.
@ximinez ximinez requested review from gregtatcam and vlntb December 17, 2024 19:46
@ximinez ximinez added API Change Build System Bug Tech Debt Non-urgent improvements Documentation README changes, code comments, etc. Perf impact not expected Change is not expected to improve nor harm performance. labels Dec 17, 2024
@ximinez ximinez added this to the 2.4.0 (2025) milestone Dec 17, 2024
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (ed4870c) to head (a64a40b).

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   #5225     +/-   ##
=========================================
- Coverage     78.0%   78.0%   -0.0%     
=========================================
  Files          789     789             
  Lines        67007   67012      +5     
  Branches      8110    8112      +2     
=========================================
+ Hits         52274   52275      +1     
- Misses       14733   14737      +4     
Files with missing lines Coverage Δ
src/xrpld/app/misc/NetworkOPs.cpp 70.2% <100.0%> (+<0.1%) ⬆️
src/xrpld/app/main/Main.cpp 79.0% <0.0%> (-0.8%) ⬇️

... and 5 files with indirect coverage changes

Impacted file tree graph

* upstream/develop:
  chore: add macos dependency installation (5233)
  prefix Uint384 and Uint512 with Hash in server_definitions (5231)
  refactor: add `rpcName` to `LEDGER_ENTRY` macro (5202)
* upstream/develop:
  chore: update deprecated Github Actions (5241)
  XLS-46: DynamicNFT (5048)
Copy link
Collaborator

@legleux legleux left a comment

Choose a reason for hiding this comment

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

I love it

* upstream/develop:
  chore: Update Visual Studio CI to VS 2022, and add VS Debug builds (5240)
  Add [validator_list_threshold] to validators.txt to improve UNL security (5112)
  Switch from assert to XRPL_ASSERT (5245)
  Add missing space character to a log message (5251)
  Cleanup API-CHANGELOG.md (5207)
  test: Unit tests to recreate invalid index logic error (5242)
  Update branch management and merge / release processes  (5215)
  fix: Error consistency in LedgerEntry::parsePermissionedDomains() (5252)
  Set version to 2.4.0-b2
  fix: Use consistent CMake settings for all modules (5228)
  Fix levelization script to ignore commented includes (5194)
  Fix the flag processing of NFTokenModify (5246)
  Fix failing assert in `connect` RPC (5235)
  Permissioned Domains (XLS-80d) (5161)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change Bug Build System Documentation README changes, code comments, etc. Perf impact not expected Change is not expected to improve nor harm performance. Tech Debt Non-urgent improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants