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

all: update golang/x/ext and fix slice sorting fallout #27909

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Aug 11, 2023

The Go authors updated golang/x/ext to change the function signature of the slices sort method. It's an entire shitshow now because x/ext is not tagged, so everyone's codebase just picked a new version that some other dep depends on, causing our code to fail building.

This PR updates the dep on our code too and does all the refactorings to follow upstream...

Fixes #27897

@karalabe karalabe added this to the 1.13.0 milestone Aug 11, 2023
// Note: sorting in descending number order.
return a.Header.Number.Uint64() >= b.Header.Number.Uint64()
return -a.Header.Number.Cmp(b.Header.Number)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return -a.Header.Number.Cmp(b.Header.Number)
return b.Header.Number.Cmp(a.Header.Number)

Alternative version is to just swap the arguments. Use with whichever you feel like

p2p/protocol.go Outdated
@@ -78,9 +79,15 @@ func (cap Cap) String() string {
}

// Less defines the canonical sorting order of capabilities.
func (cap Cap) Less(other Cap) bool {
func (cap Cap) Less(other Cap) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (cap Cap) Less(other Cap) int {
func (cap Cap) Cmp(other Cap) int {

Wouldn't that be more correct? Adds more fallout, sorry ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, indeed

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@fjl fjl merged commit be65b47 into ethereum:master Aug 11, 2023
1 check passed
@fjl fjl modified the milestones: 1.13.0, 1.12.2 Aug 11, 2023
fjl pushed a commit that referenced this pull request Aug 12, 2023
The Go authors updated golang/x/ext to change the function signature of the slices sort method. 
It's an entire shitshow now because x/ext is not tagged, so everyone's codebase just 
picked a new version that some other dep depends on, causing our code to fail building.

This PR updates the dep on our code too and does all the refactorings to follow upstream...
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
The Go authors updated golang/x/ext to change the function signature of the slices sort method. 
It's an entire shitshow now because x/ext is not tagged, so everyone's codebase just 
picked a new version that some other dep depends on, causing our code to fail building.

This PR updates the dep on our code too and does all the refactorings to follow upstream...
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
gartnera added a commit to zeta-chain/ethermint that referenced this pull request Jul 24, 2024
We cannot upgrade beyond 1.12.1 because of the x/ext slices mess:
- ethereum/go-ethereum#27909
- cosmos/cosmos-sdk#20159
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.

could not load export data: no export data for "github.com/ethereum/go-ethereum/metrics"
3 participants