Skip to content

chore: module-renaming workflow inverts between libevm and geth #152

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

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Feb 20, 2025

Why this should be merged

Originally I'd planned on doing an upstream sync by running the rename workflow on the incoming commit and then merging it to main however this resulted in hundreds of merge commits that were solely due to Go imports. Renaming the module from ava-labs/libevm to ethereum/go-ethereum removed 90% of conflicts (H/T @darioush). The module will then need to be named ava-labs/libevm again, so the commit history will probably1 look like this after an update:

---
config:
  gitGraph:
    mainBranchName: main
    parallelCommits: true
---
gitGraph TB:
    branch geth order:2
    commit id:"[email protected]"
    checkout main
    commit id:"[email protected]"
    branch sync/v1.15.2
    commit id:"[AUTO] rename to ethereum/go-ethereum"
    merge geth
    commit id:"[AUTO] rename to ava-labs/libevm"
    checkout main
    merge sync/v1.15.2 id:"[email protected]"
Loading

How this works

The current module name is determined with go list -m and the rename from/to patterns are no longer hard-coded.

How this was tested

Inspection of runs and resulting branch. Although these were the bc8e501 workflow, the only other commit in this PR is cosmetic (as seen in this run to create f2cecaf).

  1. Run on main @ d32c7e0 changes from libevm to go-ethereum:
    a. Run
    b. Commit
    c. Created branch arr4n/auto/test-invertible-rename
  2. Re-run on arr4n/auto/test-invertible-rename changes back to libevm:
    a. Run
    b. Commit
    c. Auto-generated branch is identical to main at the time of running

Footnotes

  1. Specifics of and rationale behind the merge strategy are beyond the scope of this PR.

@ARR4N ARR4N marked this pull request as ready for review February 20, 2025 11:12
@ARR4N ARR4N requested review from a team, darioush, michaelkaplan13 and cwend-ava and removed request for a team and cwend-ava February 20, 2025 11:12
@ARR4N ARR4N mentioned this pull request Feb 20, 2025
5 tasks
Copy link

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Whoa that is quite awesome, good work 👍 💯 !!

Comment on lines +32 to +33
git branch --points-at HEAD;
git tag --points-at HEAD;
Copy link

Choose a reason for hiding this comment

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

supernit I don't think semi colons are necessary

Suggested change
git branch --points-at HEAD;
git tag --points-at HEAD;
git branch --points-at HEAD
git tag --points-at HEAD

Comment on lines +35 to +36
- name: Set up Go
uses: actions/setup-go@v5
Copy link

Choose a reason for hiding this comment

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

nit the action name is self explanatory I think

Suggested change
- name: Set up Go
uses: actions/setup-go@v5
- uses: actions/setup-go@v5


- name: Validate Go module
if: ${{ steps.go.outputs.MODULE != 'github.com/ava-labs/libevm' && steps.go.outputs.MODULE != 'github.com/ethereum/go-ethereum' }}
run: echo "Unexpected Go module ${{ steps.go.outputs.MODULE }}" && exit 1;
Copy link

Choose a reason for hiding this comment

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

supernit let's exit even if echo fails, no point having &&

Suggested change
run: echo "Unexpected Go module ${{ steps.go.outputs.MODULE }}" && exit 1;
run: echo "Unexpected Go module ${{ steps.go.outputs.MODULE }}"; exit 1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If echo were to fail then the step would fail anyway.

env:
WORKFLOW_HASH: ${{ hashFiles('.github/workflows/rename-module.yml') }}
RENAME_TO: ${{ steps.go.outputs.MODULE_SUFFIX == 'ava-labs/libevm' && 'ethereum/go-ethereum' || 'ava-labs/libevm' }}
Copy link

Choose a reason for hiding this comment

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

What does this expression mean, I am rather confused 😄

steps.go.outputs.MODULE_SUFFIX == 'ava-labs/libevm' && 'ethereum/go-ethereum' || 'ava-labs/libevm'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment in the code.

@ARR4N ARR4N enabled auto-merge (squash) March 7, 2025 13:17
@ARR4N ARR4N merged commit 4c6e50e into main Mar 7, 2025
11 checks passed
@ARR4N ARR4N deleted the arr4n/invertible-module-rename branch March 7, 2025 13:23
Copy link

@alarso16 alarso16 left a comment

Choose a reason for hiding this comment

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

This is pretty cool

github-merge-queue bot pushed a commit that referenced this pull request Mar 17, 2025
## Why this should be merged

Safer release process by enforcing invariants of `release/*` branches as
automated in #137 to fulfil #25.

## How this works

New test for `go_tooling` CI job.

If the PR target branch is `main` then only the `params.ReleaseType` is
checked. If the target is neither `main` nor a release branch then the
test is skipped. The checks performed on `release/*` branches are
described in the test.

## How this was tested

Locally against a dummy release branch with deliberate problems created
by (a) including this PR's changes in the final commit and (b) not
updating the libevm version.

```
$ go test -v ./... --target_branch="release/v1.13.14-0.1.0.rc.3"
=== RUN   TestCherryPicksFormat
--- PASS: TestCherryPicksFormat (0.39s)
=== RUN   TestBranchProperties
=== RUN   TestBranchProperties/branch_name
    release_test.go:172: 
                Error Trace:    .../ava-labs/libevm/libevm/tooling/release/release_test.go:172
                Error:          Not equal: 
                                expected: "release/v1.13.14-0.1.0.beta"
                                actual  : "release/v1.13.14-0.1.0.rc.3"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -release/v1.13.14-0.1.0.beta
                                +release/v1.13.14-0.1.0.rc.3
                Test:           TestBranchProperties/branch_name
    release_test.go:175: On release branch; params.LibEVMReleaseType = "beta", which is unsuitable for release branches
=== RUN   TestBranchProperties/commit_history
    release_test.go:192: Forked from "main" at commit 4c6e50e (chore: module-renaming workflow inverts between `libevm` and `geth` (#152))
    release_test.go:314: ### History since fork from default branch (8 commits):
    release_test.go:316: internal/build, rpc: add missing HTTP response body Close() calls (ethereum#29223) by Shiming Zhang <[email protected]>
    release_test.go:316: core/state: fix bug in statedb.Copy and remove unnecessary preallocation (ethereum#29563) by Aaron Chen <[email protected]>
    release_test.go:316: params: print time value instead of pointer in ConfigCompatError (ethereum#29514) by Nathan <[email protected]>
    release_test.go:316: eth/gasprice: add query limit for FeeHistory to defend DDOS attack (ethereum#29644) by Nathan <[email protected]>
    release_test.go:316: core/state/snapshot: add a missing lock (ethereum#30001) by maskpp <[email protected]>
    release_test.go:316: crypto: add IsOnCurve check (ethereum#31100) by Felix Lange <[email protected]>
    release_test.go:316: internal/ethapi: fix panic in debug methods (ethereum#31157) by Sina M <[email protected]>
    release_test.go:316: x by Arran Schlosberg <[email protected]>
=== RUN   TestBranchProperties/commit_history/cherry_picked_commits
    release_test.go:314: ### Expected cherry-picks (7 commits):
    release_test.go:316: internal/build, rpc: add missing HTTP response body Close() calls (ethereum#29223) by Shiming Zhang <[email protected]>
    release_test.go:316: core/state: fix bug in statedb.Copy and remove unnecessary preallocation (ethereum#29563) by Aaron Chen <[email protected]>
    release_test.go:316: params: print time value instead of pointer in ConfigCompatError (ethereum#29514) by Nathan <[email protected]>
    release_test.go:316: eth/gasprice: add query limit for FeeHistory to defend DDOS attack (ethereum#29644) by Nathan <[email protected]>
    release_test.go:316: core/state/snapshot: add a missing lock (ethereum#30001) by maskpp <[email protected]>
    release_test.go:316: crypto: add IsOnCurve check (ethereum#31100) by Felix Lange <[email protected]>
    release_test.go:316: internal/ethapi: fix panic in debug methods (ethereum#31157) by Sina M <[email protected]>
=== RUN   TestBranchProperties/commit_history/final_commit
    release_test.go:365: Modified disallowed file "go.yml"
    release_test.go:365: Modified disallowed file "go.mod"
    release_test.go:365: Modified disallowed file "go.sum"
    release_test.go:365: Modified disallowed file "release_test.go"
--- FAIL: TestBranchProperties (2.07s)
    --- FAIL: TestBranchProperties/branch_name (0.00s)
    --- FAIL: TestBranchProperties/commit_history (2.07s)
        --- PASS: TestBranchProperties/commit_history/cherry_picked_commits (1.81s)
        --- FAIL: TestBranchProperties/commit_history/final_commit (0.01s)
FAIL
FAIL    github.com/ava-labs/libevm/libevm/tooling/release       2.712s
FAIL
```
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.

3 participants