Remove tools.go

[fasmat]

Since go install and go get support the installation of specific dependencies / tools via version identifier it should be used instead of a tools.go file to add those dependencies.

The reason is that a tool installed that was listed as dependency via tools.go is a custom build with (possibly) overwritten dependencies from the official release of that version. Here's an example:

Lets say [email protected] depends on golang.org/x/[email protected] and go-libp2p depends on this version of the dependency too. If the dependency in go-libp2p is updated to v0.20.0 mockgen will now be used in a custom version that is based on v0.4.0 but with an updated golang.org/x/tools dependency. This can lead to builds suddenly breaking in unexpected ways.

[fasmat]

To avoid having to specify the same version multiple times a Makefile could be used that installs the tools once in the required version.

With tools.go the issue remains that go.mod is polluted with dependencies that are only needed for its tools rather than the library itself and that the tools installed this way are not the versions specified (but custom builds with changed/updated dependencies).

[Jorropo]

I think this needs to be handled in go, it would be nice to define dev-dependencies which aren't transitively given to consumers.

For now is there a way we could define a second module for tools with it's own go.mod so it would fix your usecase when importing go-libp2p ? I guess we could make tools/tools.go, tools/go.mod and then cd into tools and go install the required tools, but that might polute GOBIN, idk how easy it would be to setup mktemp PATH or something like that.

[Jorropo]

Btw I get the problem you are describing could happen, but have you seen this in the wild ? I tried https://github.com/uber-go/mock (Uber's maintained fork) with golang.org/x/tools v0.21.0 and it builds fine:

> git clone [email protected]:uber-go/mock.git
Cloning into 'mock'...
cd mremote: Enumerating objects: 3359, done.
remote: Counting objects: 100% (786/786), done.
remote: Compressing objects: 100% (126/126), done.
Receiving objects:  54% (1814/3359)
remote: Total 3359 (delta 701), reused 691 (delta 660), pack-reused 2573
Receiving objects: 100% (3359/3359), 893.24 KiB | 1.93 MiB/s, done.
Resolving deltas: 100% (1925/1925), done.
> cd mock
hugo@rikus ~/k/mock (main)> go get golang.org/x/tools@latest
go: downloading golang.org/x/tools v0.21.0
go: upgraded golang.org/x/mod v0.15.0 => v0.17.0
go: upgraded golang.org/x/tools v0.18.0 => v0.21.0
> go mod tidy
go: downloading github.com/yuin/goldmark v1.4.13
> go build ./...
> git diff go.mod 
diff --git a/go.mod b/go.mod
index 4826710..d285f58 100644
--- a/go.mod
+++ b/go.mod
@@ -3,8 +3,8 @@ module go.uber.org/mock
 go 1.20
 
 require (
-       golang.org/x/mod v0.15.0
-       golang.org/x/tools v0.18.0
+       golang.org/x/mod v0.17.0
+       golang.org/x/tools v0.21.0
 )
 
 require github.com/yuin/goldmark v1.4.13 // indirect

[Jorropo]

I've tried to repro your problem and I'm a bit confused:

• libp2p is only one of many project in the module graph specifying tools.go and having a dependency on golang.org/x/tools so it isn't removed from the dependency graph: https://github.com/libp2p/go-libp2p/pull/2781/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R126
• It works ¿ I tried both https://github.com/golang/mock and https://github.com/uber-go/mock and they both work with golang.org/x/[email protected] (what go.mod currently specify) and golang.org/x/[email protected] (@latest) even tho golang/mock updates from golang.org/x/tools v0.1.8 !

[MarcoPolo]

I'm not opposed to this change, but I don't want to see versions strewn all throughout the codebase. A makefile would be preferred or another solution that lets you define the versions outside of this module (can we cd into another module and run the tool from there?).

[fasmat]

Theoretically you could have a tools module, cd into that directory and install the tools from there. This would prevent polluting dependencies in the libp2p module. However this still causes tools to overwrite dependencies of each other which can break unexpectedly:

Tool A and B use github.com/quic-go/[email protected]. Now tool A receives an update and uses github.com/quic-go/[email protected]. The new version of A cannot be used because it would break go install for tool B.

A simple solution is to have a Makefile that defines a target for the used tool dependencies:

install:
	go install google.golang.org/protobuf/cmd/[email protected]
	go install go.uber.org/mock/[email protected]

and in the code

//go:generate mockgen -package mocknetwork -destination mock_resource_manager.go github.com/libp2p/go-libp2p/core/network ResourceManager"

// instead of

//go:generate sh -c "go run go.uber.org/mock/[email protected] -package mocknetwork -destination mock_resource_manager.go github.com/libp2p/go-libp2p/core/network ResourceManager"

And then run make install before go generate ./... (or make generate which could have install as a dependency).

EDIT: it looks like the GH workflows for libp2p right now rely on installing the tool with the call to go generate:

https://github.com/libp2p/go-libp2p/blob/master/.github/workflows/go-check.yml#L16-L20 uses https://github.com/ipdxco/unified-github-workflows/blob/v1.0/.github/workflows/go-check.yml which then calls go generate and would need to first include a step where the relevant tools are installed on the runner.

[MarcoPolo]

I want to avoid installing anything globally on the user's path. This is brittle, and, for me at least, doesn't work.

[fasmat]

It doesn't need to be global:

export GOBIN := $(abspath .)/bin
export PATH := $(GOBIN):$(PATH)

.PHONY: install
install:
	go install google.golang.org/protobuf/cmd/[email protected]
	go install go.uber.org/mock/[email protected]

.PHONY: generate
generate: install
	go generate ./...

Now go install installs to ./bin in the directory of the Makefile and that directory is added to PATH so go generate can find the executables 🙂

[MarcoPolo]

What if a user has older binaries in their ./bin, would running make generate know to update the binaries? I think not

[fasmat]

go install overwrites existing binaries with the same name. Just like it does if used to install a tool globally:

~/workspace$ GOBIN=$(pwd)/bin go install go.uber.org/mock/[email protected]
go: downloading go.uber.org/mock v0.4.0
~/workspace$ ./bin/mockgen -version
v0.4.0
~/workspace$ GOBIN=$(pwd)/bin go install go.uber.org/mock/[email protected]
go: downloading go.uber.org/mock v0.3.0
~/workspace$ ./bin/mockgen -version
v0.3.0

The example Makefile in my previous comment will always ensure that the version specified is at the path specified. It is a bit inefficient because it re-builds mockgen every time make generate is executed. The build is cached however, so installing a version that is already available is (basically) a no-op. Also, at the moment mockgen is rebuild multiple times on a single go generate call (once per //go:generate comment) - however also cached and basically a no-op after the first.

[MarcoPolo]

ah, I see you added the .PHONY part, I didn't see that in the email. That makes sense. Can you try updating this PR to do that and we can play with it :)

[fasmat]

Interoperability testing fails for head, not because of this change but probably because of this PR: https://github.com/libp2p/go-libp2p/pull/2780

It seems like updated webtransport isn't backwards compatible.

[sukunrt]

I will defer to Marco on this, but I'd prefer not maintaining a make file for two go install commands. Can we just provide installation instructions in a doc file?

[MarcoPolo]

I've moved the Makefile to a new tools dir because I'm not a fan of having a Makefile in the repo root that isn't part of building the project.

I'd prefer not maintaining a make file for two go install commands. Can we just provide installation instructions in a doc file?

The problem is that we'd have to update the doc and CI and our local machines any time that changes. It's much easier to always do make generate and be correct. In this sense maintaining the Makefile is a lot easier because if it's out of date CI breaks :)

[MarcoPolo]

With tools.go the issue remains that go.mod is polluted with dependencies that are only needed for its tools rather than the library itself and that the tools installed this way are not the versions specified (but custom builds with changed/updated dependencies).

@fasmat, I haven't looked into it yet, but I would have expected us to see some removals in go.mod. A brief glance at the diff doesn't show that. Any ideas?

[fasmat]

With tools.go the issue remains that go.mod is polluted with dependencies that are only needed for its tools rather than the library itself and that the tools installed this way are not the versions specified (but custom builds with changed/updated dependencies).

@fasmat, I haven't looked into it yet, but I would have expected us to see some removals in go.mod. A brief glance at the diff doesn't show that. Any ideas?

Because the tools that have been used for libp2p so far have the same dependencies as libp2p - golang.org/x/tools has become an indirect dependency now 🙂

[Stebalien]

I am against this change for several reasons:

First, this seems to be complicating the go generate process with little benefit. The current approach means a user can run go generate in any package to re-generate specific components without having to re-generate everything and/or setup the "correct" path so they can do it with a simple go generate call.

Second, specifying these in the go.mod file significantly improves our security and protects us against supply-chain attacks via the go.sum file. Clearly that won't help us if we update a dependency, but most supply-chain attacks are short-lived.

Lets say [email protected] depends on golang.org/x/[email protected] and go-libp2p depends on this version of the dependency too. If the dependency in go-libp2p is updated to v0.20.0 mockgen will now be used in a custom version that is based on v0.4.0 but with an updated golang.org/x/tools dependency. This can lead to builds suddenly breaking in unexpected ways.

This is a general "updating dependencies can break things" issue.

[MarcoPolo]

The current approach means a user can run go generate in any package to re-generate specific components without having to re-generate everything and/or setup the "correct" path so they can do it with a simple go generate call.

Agreed, this is a nice quality of life improvement.

Second, specifying these in the go.mod file significantly improves our security and protects us against supply-chain attacks via the go.sum file.

It's a good callout that this is missing any form of checksumming. I'm pretty sure this could be fixed, but at the cost of something more complex. I should have thought about this from the start and brought it up.

I'm a bit disappointed that this change in practice doesn't actually reduce any of our dependencies. It just moved one to be indirect.

I do like that this change standardized the protoc version and automatically installs it for the user. That part is nice since that's an implied system dependency we rely on now. Although, in practice, devs rarely need to touch this and the version is standardized in CI.

The actual benefits here don't seem to outweigh the cost of maintaining this and adopting a non-standard go generate workflow. However, I do appreciate this exercise and the effort that went in (thanks @fasmat). Perhaps in the future we'll gain more by adopting something like this, but for now I think we should leave it as is.