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

chore: downgrade x/tools to avoid explicit Go toolchain #3102

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link

@rvagg rvagg commented Dec 17, 2024

Would you mind downgrading golang.org/x/tools so we (downstream consumers) aren't forced in to a toolchain while this is currently being forced on us (I'm hoping that one day this could be an opt-in thing)?

The only uses of golang/x/tools are for goimports on go:generate:

$ git grep golang.org/x/tools -- \*.go
p2p/transport/quic/conn_test.go://go:generate sh -c "go run go.uber.org/mock/mockgen -package libp2pquic -destination mock_connection_gater_test.go github.com/libp2p/go-libp2p/core/connmgr ConnectionGater && go run golang.org/x/tools/cmd/goimports -w mock_connection_gater_test.go"
p2p/transport/webtransport/transport_test.go://go:generate sh -c "go run go.uber.org/mock/mockgen -package libp2pwebtransport_test -destination mock_connection_gater_test.go github.com/libp2p/go-libp2p/core/connmgr ConnectionGater && go run golang.org/x/tools/cmd/goimports -w mock_connection_gater_test.go"
tools.go:       _ "golang.org/x/tools/cmd/goimports"

And the uses of the golang/x/exp package being downgraded here too are the pseudo-random number generator:

$ git grep golang.org/x/exp -- \*.go
p2p/protocol/autonatv2/autonat.go:      "golang.org/x/exp/rand"
p2p/protocol/autonatv2/client.go:       "golang.org/x/exp/rand"
p2p/transport/webrtc/transport.go:      mrand "golang.org/x/exp/rand"

I haven't looked at the specifics of what's changed between the two versions of these but I can get more specifics if it would help the case here.

@MarcoPolo
Copy link
Collaborator

We can do this. How did you isolate it to these two packages?

This seems at best a short term fix as more upstream packages add this, no?

@rvagg
Copy link
Author

rvagg commented Dec 17, 2024

We can do this. How did you isolate it to these two packages?

A bit of educated guesswork and fiddling; I'm just frustrated that these directives are viral in that a dependency can force it on you even if you don't want it. But I might just be old-man-yelling-at-cloud on this one. I'll be interested to see the response to golang/tools#548 but I'm not holding my breath. I suspect they're locked into this one. But the big problem seems to be that toolchain gets updated by each go mod tidy depending on the version of go that's run so it's going to mess with everyone's CI checks.

This seems at best a short term fix as more upstream packages add this, no?

I don't know, I find it hard to believe everyone's cool with the churn this is going to involve. It's reasonable for shippable programs being built with a fixed toolchain version, but for libraries to be able to force that on their upstream dependants for no good reason (if there was a good reason, then sure, opt-in!), it just seems like it's ripe for people fighting with dependencies and blocking upgrades.

🤷 your call on this one, I won't be upset if you don't accept it.

@rvagg rvagg closed this Dec 18, 2024
@rvagg rvagg deleted the rvagg/notoolchain branch December 18, 2024 04:52
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.

2 participants