Conversation
- Introduced BUILD_TIME argument in Dockerfiles to capture build time. - Updated installation commands to pass git commit, version, and build time as arguments. - Enhanced version command in Geth to display build time and commit information. - Modified Makefile to include build metadata in Docker image builds. - Updated CI configuration to output build time for Docker releases. - Refactored build environment to support new build metadata fields.
…nd commands - Replaced individual build flags with a unified BUILD_FLAGS variable in Makefiles. - Updated installation commands in Makefiles and MakefileEc2.mk to utilize the new BUILD_FLAGS. - Removed deprecated version handling from params/version.go and integrated versioning into the build process. - Enhanced version output across various commands to include build metadata. - Streamlined CI configuration to support the new versioning scheme and build flags.
Contributor
📝 WalkthroughWalkthroughThis PR refactors version and build metadata handling by replacing the ChangesVersion system refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/rlpdump/main.go (1)
138-138: 💤 Low valueConsider reverting to
fmt.Fprintffor consistency.This change from
fmt.Fprintf(out, "%s[]", ws(depth))tofmt.Fprint(out, ws(depth)+"[]")appears unrelated to the version system refactor. While it aligns the bracket output style with lines 140 and 151, it introduces inconsistency with the data output lines (130, 132) that usefmt.Fprintfwith format strings. The originalFprintfapproach is more idiomatic, avoids string concatenation overhead, and maintains consistency with the surrounding string formatting.♻️ Suggested revert
- fmt.Fprint(out, ws(depth)+"[]") + fmt.Fprintf(out, "%s[]", ws(depth))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/rlpdump/main.go` at line 138, Replace the direct string concatenation fmt.Fprint(out, ws(depth)+"[]") with the idiomatic formatted call fmt.Fprintf(out, "%s[]", ws(depth)) to match surrounding output lines; locate the call that writes the empty-bracket representation (uses the ws(depth) helper and the out writer) and change it to use Fprintf with a format string to avoid concatenation overhead and preserve consistency..github/workflows/docker_release.yml (1)
21-24: ⚡ Quick winDisable persisted checkout credentials.
Line 21-24 can avoid leaving credentials in git config by explicitly setting
persist-credentials: false; this reduces token exposure risk in later steps/actions.Suggested patch
- uses: actions/checkout@v4 with: ref: ${{ inputs.tag || github.ref }} fetch-depth: 0 + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker_release.yml around lines 21 - 24, The checkout step currently uses actions/checkout@v4 with inputs ref and fetch-depth but does not disable persisted credentials; update the checkout action (uses: actions/checkout@v4) to include the input persist-credentials: false so git credentials are not stored in the repo config after checkout, keeping the existing ref and fetch-depth inputs intact.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 10-13: The BUILD_FLAGS variable is defined with recursive
expansion which can re-evaluate timestamp-backed values; change its assignment
to use simple expansion so BUILD_FLAGS is evaluated once at parse time (freeze
the values of GITCOMMIT, VERSION and BUILD_TIME). Update the BUILD_FLAGS
assignment (the BUILD_FLAGS variable) to use a simple-expansion operator so it
captures the current $(GITCOMMIT), $(VERSION) and $(BUILD_TIME) values
immediately instead of re-evaluating them later.
In `@MakefileEc2.mk`:
- Around line 6-9: The BUILD_FLAGS variable is using recursive expansion which
can cause timestamp drift; change its definition to use simple (immediate)
expansion so it is frozen at assignment: define BUILD_FLAGS with := and compose
it from the already-defined GITCOMMIT, VERSION, and BUILD_TIME variables (so
BUILD_FLAGS := -git-commit=$(GITCOMMIT) -version=$(VERSION)
-build-time=$(BUILD_TIME)), ensuring BUILD_FLAGS is evaluated once rather than
on each reference.
---
Nitpick comments:
In @.github/workflows/docker_release.yml:
- Around line 21-24: The checkout step currently uses actions/checkout@v4 with
inputs ref and fetch-depth but does not disable persisted credentials; update
the checkout action (uses: actions/checkout@v4) to include the input
persist-credentials: false so git credentials are not stored in the repo config
after checkout, keeping the existing ref and fetch-depth inputs intact.
In `@cmd/rlpdump/main.go`:
- Line 138: Replace the direct string concatenation fmt.Fprint(out,
ws(depth)+"[]") with the idiomatic formatted call fmt.Fprintf(out, "%s[]",
ws(depth)) to match surrounding output lines; locate the call that writes the
empty-bracket representation (uses the ws(depth) helper and the out writer) and
change it to use Fprintf with a format string to avoid concatenation overhead
and preserve consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49327af8-58b9-4239-bde0-f1ef74260ef8
📒 Files selected for processing (32)
.github/scripts/bump_version_dot_go.mjs.github/workflows/docker_release.ymlDockerfileDockerfile.alltoolsMakefileMakefileEc2.mkbuild/ci.gobuild/deb/ethereum/deb.rulescmd/abidump/main.gocmd/abigen/main.gocmd/bootnode/main.gocmd/checkpoint-admin/main.gocmd/clef/main.gocmd/devp2p/main.gocmd/ethkey/main.gocmd/evm/main.gocmd/faucet/faucet.gocmd/gen-preimages/main.gocmd/geth/config.gocmd/geth/consolecmd_test.gocmd/geth/main.gocmd/geth/misccmd.gocmd/migration-checker/main.gocmd/p2psim/main.gocmd/puppeth/puppeth.gocmd/rlpdump/main.goeth/backend.gointernal/build/env.gointernal/flags/helpers.gointernal/version/version.gomobile/geth.goparams/version.go
💤 Files with no reviewable changes (1)
- params/version.go
Comment on lines
+10
to
+13
| GITCOMMIT ?= $(shell git rev-parse HEAD 2>/dev/null || echo "unknown") | ||
| VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") | ||
| BUILD_TIME ?= $(shell date -u +%Y-%m-%dT%H:%M:%SZ) | ||
| BUILD_FLAGS = -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME) |
Contributor
There was a problem hiding this comment.
Freeze build metadata flags with simple expansion.
Line 13 currently uses recursive expansion; this can re-evaluate timestamp-backed values and make metadata inconsistent across references in one invocation. Prefer a simply expanded BUILD_FLAGS.
Suggested fix
GITCOMMIT ?= $(shell git rev-parse HEAD 2>/dev/null || echo "unknown")
VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev")
BUILD_TIME ?= $(shell date -u +%Y-%m-%dT%H:%M:%SZ)
-BUILD_FLAGS = -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME)
+BUILD_FLAGS := -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME)🧰 Tools
🪛 checkmake (0.3.2)
[warning] 13-13: Variable "BUILD_TIME" possibly contains a timestamp and should be simply expanded.
(timestampexpanded)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 10 - 13, The BUILD_FLAGS variable is defined with
recursive expansion which can re-evaluate timestamp-backed values; change its
assignment to use simple expansion so BUILD_FLAGS is evaluated once at parse
time (freeze the values of GITCOMMIT, VERSION and BUILD_TIME). Update the
BUILD_FLAGS assignment (the BUILD_FLAGS variable) to use a simple-expansion
operator so it captures the current $(GITCOMMIT), $(VERSION) and $(BUILD_TIME)
values immediately instead of re-evaluating them later.
Source: Linters/SAST tools
Comment on lines
+6
to
+9
| GITCOMMIT ?= $(shell git rev-parse HEAD 2>/dev/null || echo "unknown") | ||
| VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") | ||
| BUILD_TIME ?= $(shell date -u +%Y-%m-%dT%H:%M:%SZ) | ||
| BUILD_FLAGS = -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME) |
Contributor
There was a problem hiding this comment.
Use simple expansion for composed build flags here too.
Line 9 has the same recursive expansion pattern; freezing BUILD_FLAGS avoids timestamp drift between references.
Suggested fix
GITCOMMIT ?= $(shell git rev-parse HEAD 2>/dev/null || echo "unknown")
VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev")
BUILD_TIME ?= $(shell date -u +%Y-%m-%dT%H:%M:%SZ)
-BUILD_FLAGS = -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME)
+BUILD_FLAGS := -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| GITCOMMIT ?= $(shell git rev-parse HEAD 2>/dev/null || echo "unknown") | |
| VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") | |
| BUILD_TIME ?= $(shell date -u +%Y-%m-%dT%H:%M:%SZ) | |
| BUILD_FLAGS = -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME) | |
| GITCOMMIT ?= $(shell git rev-parse HEAD 2>/dev/null || echo "unknown") | |
| VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev") | |
| BUILD_TIME ?= $(shell date -u +%Y-%m-%dT%H:%M:%SZ) | |
| BUILD_FLAGS := -git-commit=$(GITCOMMIT) -version=$(VERSION) -build-time=$(BUILD_TIME) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MakefileEc2.mk` around lines 6 - 9, The BUILD_FLAGS variable is using
recursive expansion which can cause timestamp drift; change its definition to
use simple (immediate) expansion so it is frozen at assignment: define
BUILD_FLAGS with := and compose it from the already-defined GITCOMMIT, VERSION,
and BUILD_TIME variables (so BUILD_FLAGS := -git-commit=$(GITCOMMIT)
-version=$(VERSION) -build-time=$(BUILD_TIME)), ensuring BUILD_FLAGS is
evaluated once rather than on each reference.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BUILD_FLAGSvariable acrossMakefile/MakefileEc2.mkand the CI/Docker build path.params/version.gointointernal/version, and inject version / git commit / build time at build time.gethbinary, and surface it in thegeth versionoutput.Changes
Makefile,MakefileEc2.mk: replace individual flags with the unifiedBUILD_FLAGS; pass git commit, version and build time into install / docker builds.Dockerfile,Dockerfile.alltools,.github/workflows/docker_release.yml: introduceBUILD_TIMEarg and propagate build metadata.internal/version/version.go,internal/build/env.go,internal/flags/helpers.go: new versioning scheme; remove deprecated handling fromparams/version.go.cmd/*,build/ci.go: surface build time and commit info in version output.Test plan
make gethbuilds successfully andgeth versionshows commit + build timeBUILD_TIME/ commit / version argsdocker_releaseworkflow runs greenMade with Cursor
Close: #337
Summary by CodeRabbit
Release Notes
New Features
-versionflag to display version information.Chores