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

cmd/go: "mod tidy" tries to require the latest version of a replacement module #70137

Closed
developStorm opened this issue Oct 31, 2024 · 8 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@developStorm
Copy link

Go version

go version go1.21.1 darwin/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='~/Library/Caches/go-build'
GOENV='~/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='~/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='~/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/Cellar/go/1.21.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/Cellar/go/1.21.1/libexec/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='.../zdns/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/tmp/go-build0000000=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Bump version for zmap/dns in this replace line to v1.1.63, then run go mod tidy

https://github.com/zmap/zdns/pull/468/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R20

What did you see happen?

go mod tidy
go: github.com/zmap/dns@v1.1.63 used for two different module paths (github.com/miekg/dns and github.com/zmap/dns)

What did you expect to see?

go mod tidy executes without errors.

The issue arises when tidy attempts to pull the latest version of the replacement target module (zmap/dns in this case) and includes it as an indirect requirement. When the replace target version is the latest version, the indirect require that tidy tries to add caused the above error.

Adjusting the replace directive to a non-latest version - e.g., replace github.com/miekg/dns => github.com/zmap/dns v1.1.62-zdns as what we have in the referenced PR now - allows tidy to complete without errors. However, tidy will try to introduce an indirect requirement for v1.1.63 (latest), which I believe is also undesirable, as I specifically requested a version that is not the latest in the replace directive.

I expected go mod tidy to respect whatever version specified in replace, and does not attempt to introduce the latest version as an indirect require.

@developStorm developStorm changed the title import/path: issue title cmd/go: "mod tidy" tries to require the latest version of a replacement module Oct 31, 2024
@cagedmantis cagedmantis added GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 31, 2024
@cagedmantis cagedmantis added this to the Backlog milestone Oct 31, 2024
@cagedmantis
Copy link
Contributor

cc @matloob @samthanawalla

@seankhliao
Copy link
Member

Duplicate of #26904

@seankhliao seankhliao marked this as a duplicate of #26904 Oct 31, 2024
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
@developStorm
Copy link
Author

developStorm commented Oct 31, 2024

Hi @seankhliao, thanks for reviewing this issue. Could you clarify why this is considered a duplicate of #26904? In #26904, the user module involved both the upstream and fork1, aiming to use fork2 as a replacement for both, with neither upstream nor fork1 containing a go.mod.

In this case, however, we only have the upstream github.com/miekg/dns and a single fork github.com/zmap/dns. The project initially used only the upstream, and our intention is to replace it with our fork directly in our "user module" (github.com/zmap/zdns) . We aren’t aiming to use both upstream and our fork simultaneously - though go mod tidy seems to attempt that, which is central to the issue here. FWIW, both the upstream repo and our fork have a valid go.mod and declare their module path.

@seankhliao
Copy link
Member

seankhliao commented Oct 31, 2024

Replace replaces source code, but doesn't rewrite import paths. meikg/dns is replaced by zmap/dns, but this replaced version of meikg/dns has imports that require zmap/zdns, that's are where your indirect dependency comes from.

So zmap/dns is used as both the replacement and itself (required by the replacement).
For most replacement targets this isn't an issue because they maintain the original import path / module declaration.

@developStorm
Copy link
Author

developStorm commented Oct 31, 2024

Thank you for the explanation! Sorry about the confusing names here, but I think zmap/dns shouldn’t have a dependency on zmap/zdns, as the latter is our "user module." If our fork zmap/dns required our user module zmap/zdns, this wouldn't have worked from the beginning due to circular dependency, right? I double-checked the zmap/dns repo using rg "zmap/zdns" and go mod graph | grep "zdns" and indeed didn’t find any requirement for zmap/zdns.

To elaborate further, our user module zmap/zdns builds and works fine with go.mod set up as any of the following (irrelevant entries omitted):

module github.com/zmap/zdns

go 1.20

require (
	github.com/miekg/dns v1.1.62
)

replace github.com/miekg/dns => github.com/zmap/dns v1.1.62-zdns

require (
	github.com/zmap/dns v1.1.63 // indirect, introduced automatically by go mod tidy
)
module github.com/zmap/zdns

go 1.20

require (
	github.com/miekg/dns v1.1.62
)

replace github.com/miekg/dns => github.com/zmap/dns v1.1.63

require (
	// this with the following line commented out builds, but go mod tidy will try to add the following and fail
	// github.com/zmap/dns v1.1.63 // indirect, introduced automatically by go mod tidy
)
module github.com/zmap/zdns

go 1.20

require (
	// Also builds if we just require the fork, then replace all imports
	// of meikg/dns with zmap/dns in **/*.go files 
	github.com/zmap/dns v1.1.63
)

require (

)

It’s only in the second go.mod example above that running go mod tidy introduces github.com/zmap/dns v1.1.63 // indirect, which conflicts with the replace. In the first case, this indirect dependency is introduced but doesn’t cause a conflict due to a different version being used in replace (though it’s still undesired, as we only want to depend on the specific version in the replace).

Here's go mod graph output for zmap/dns ("the fork"):

github.com/zmap/dns go@1.19
github.com/zmap/dns golang.org/x/mod@v0.18.0
github.com/zmap/dns golang.org/x/net@v0.28.0
github.com/zmap/dns golang.org/x/sync@v0.7.0
github.com/zmap/dns golang.org/x/sys@v0.23.0
github.com/zmap/dns golang.org/x/tools@v0.22.0
golang.org/x/mod@v0.18.0 golang.org/x/tools@v0.13.0
golang.org/x/net@v0.28.0 golang.org/x/crypto@v0.26.0
golang.org/x/net@v0.28.0 golang.org/x/sys@v0.23.0
golang.org/x/net@v0.28.0 golang.org/x/term@v0.23.0
golang.org/x/net@v0.28.0 golang.org/x/text@v0.17.0
golang.org/x/tools@v0.22.0 github.com/google/go-cmp@v0.6.0
golang.org/x/tools@v0.22.0 github.com/yuin/goldmark@v1.4.13
golang.org/x/tools@v0.22.0 golang.org/x/mod@v0.18.0
golang.org/x/tools@v0.22.0 golang.org/x/net@v0.26.0
golang.org/x/tools@v0.22.0 golang.org/x/sync@v0.7.0
golang.org/x/tools@v0.22.0 golang.org/x/telemetry@v0.0.0-20240521205824-bda55230c457
golang.org/x/tools@v0.22.0 golang.org/x/sys@v0.21.0

@seankhliao
Copy link
Member

Sorry, zmap/dns depends on zmap/dns (itself).
When used as a replace target, zmap/dns is used as meikg/dns but still depends on zmap/dns, e.g. here https://github.com/zmap/dns/blob/master/dnsutil/util.go#L11

@developStorm
Copy link
Author

developStorm commented Oct 31, 2024

Ah, I see what's going on now - thank you so much for your help! I don’t believe we actually use the dnsutil and dns_test package. Do you have any recommendations for preventing this package from being imported on the user module side or from being exported on the forked zmap/dns side, so we can avoid that self-require? Or is our only option to remove the package from the source tree in our fork altogether?

Would having dnsutil and dns_test to import from github.com/miekg/dns, then replace github.com/miekg/dns => ./ in go.mod of zmap/dns be a good idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants