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: package is replaced but not required #44529

Closed
karalabe opened this issue Feb 23, 2021 · 19 comments
Closed

cmd/go: package is replaced but not required #44529

karalabe opened this issue Feb 23, 2021 · 19 comments

Comments

@karalabe
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

We have a fairly large repo (go-ethereum), which is a standalone application, but it can also be consumed as a library. We also have a code generator in our command suite which takes some API definitions and generates Go wrappers for them. The generated code relies on our main repository as a library.

The above is nothing special, but we'd like to have a test suite that uses the generator to wrap some APIs an then run them to make sure the generated code is executable and does what we expect. Currently the test uses the generator to create the wrapper Go files and then attempts to build it. Since the build depends on the main repo, we need to tell go (or go mod) about it.

Previously we used go mod init ... and then go mod edit --replace https://github.com/ethereum/go-ethereum=path/to/checkout to have the tester use the current working code (needed both for local development as well as PRs; that they always test the checked out code, not a pinned upstream version). Up until Go 1.15 this worked fine. Starting from Go 1.16, this setup is rejected.

What did you expect to see?

Go mod in 1.16 refuses to accept a replacement directive if the package being replaced isn't already part of the mod file. I could add a call to go get github.com/ethereum/go-ethereum or go mod edit --require github.com/ethereum/go-ethereum@master to the build, but that would do a network request and download out repo of >500MB. It makes no sense to have a test download external code when it's not even needed at all (the needed code is the checked out working copy the test runs from).

I guess my question is how can I convince Go mod to add a replace directive to a local copy without having it download an upstream copy first, which just gets discarded anyway.

What did you see instead?

callbackparam.go:10:2: module github.com/ethereum/go-ethereum provides package github.com/ethereum/go-ethereum and is replaced but not required; to add it:
      	go get github.com/ethereum/go-ethereum
[...]
@mvdan
Copy link
Member

mvdan commented Feb 23, 2021

Try go mod tidy? I think that should not do a network request, as it should use the replacement module on your filesystem.

@mvdan
Copy link
Member

mvdan commented Feb 23, 2021

It's also worth considering:

  1. Should go get be doing a module download when the module is replaced with a local filesystem path? Intuitively, I think not. @karalabe can you confirm whether you checked this?

  2. If I'm wrong and go get is meant to do a module download, should the advice above still use go get instead of something that won't do a download, like go mod tidy?

@karalabe
Copy link
Contributor Author

I think I misdiagnosed my problem. Still investigating it. I'm seeing strange build times and I assumed it was downloading data, but maybe it's just doing some re-builds I'm not expecting.

@jayconrod
Copy link
Contributor

go mod tidy and go get may both hit the network to look up imported packages that aren't provided by any required module. If a module is replace locally, the go command will look there first, but I think it may still go out to the network for other prefixes of the module path.

Instead, you can add a requirement on a non-existent version while replacing that version:

go mod edit -require example.com/mod@v0.0.0-local -replace example.com/mod@v0.0.0-local=../local

Adding a replacement, even one without a version on the left side, doesn't automatically add that module to the build list. If it did, the go command would read its go.mod file and apply its requirements. That could influence selected versions of other modules, even if the replaced module didn't provide any packages.

@bcmills
Copy link
Contributor

bcmills commented Feb 23, 2021

go mod tidy should never do a network lookup if it could add a replaced module instead.¹

go get, on the other hand, will perform a network lookup in order to identify the true latest version, taking your replacements into account,² and then that version will be replaced instead of downloaded. It does that so that the latest version added by go get is always consistent with go list -m [⋯]@latest, and so that (if possible) your require directive always specifies a valid version for downstream consumers (if any), so that they won't break when they require your module. (Downstream consumers will not pick up your replace directives, so they need a valid version.)

If you are not using a proxy for the repo in question, that lookup may involve cloning the upstream repo. So that can be a pretty expensive operation. (Note that the official distributions of the go command use proxy.golang.org by default, but the Fedora fork of the go command does not.)

If that network lookup fails, then go get will also fall back to replacement versions.³

¹

for mp, mv := range index.highestReplaced {
if !maybeInModule(path, mp) {
continue
}
if mv == "" {
// The only replacement is a wildcard that doesn't specify a version, so
// synthesize a pseudo-version with an appropriate major version and a
// timestamp below any real timestamp. That way, if the main module is
// used from within some other module, the user will be able to upgrade
// the requirement to any real version they choose.
if _, pathMajor, ok := module.SplitPathVersion(mp); ok && len(pathMajor) > 0 {
mv = modfetch.ZeroPseudoVersion(pathMajor[1:])
} else {
mv = modfetch.ZeroPseudoVersion("v0")
}
}
mods = append(mods, module.Version{Path: mp, Version: mv})
}

²

repoVersions, err := rr.repo.Versions(prefix)
if err != nil && !errors.Is(err, os.ErrNotExist) {
return nil, err
}
versions := repoVersions
if index != nil && len(index.replace) > 0 {
path := rr.ModulePath()
for m, _ := range index.replace {
if m.Path == path && strings.HasPrefix(m.Version, prefix) && m.Version != "" && !modfetch.IsPseudoVersion(m.Version) {
versions = append(versions, m.Version)
}
}
}

³

if v, ok := index.highestReplaced[path]; ok {
if v == "" {
// The only replacement is a wildcard that doesn't specify a version, so
// synthesize a pseudo-version with an appropriate major version and a
// timestamp below any real timestamp. That way, if the main module is
// used from within some other module, the user will be able to upgrade
// the requirement to any real version they choose.
if _, pathMajor, ok := module.SplitPathVersion(path); ok && len(pathMajor) > 0 {
v = modfetch.PseudoVersion(pathMajor[1:], "", time.Time{}, "000000000000")
} else {
v = modfetch.PseudoVersion("v0", "", time.Time{}, "000000000000")
}
}
if err != nil || semver.Compare(v, info.Version) > 0 {
return rr.replacementStat(v)
}
}

@seankhliao seankhliao added modules WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Feb 24, 2021
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@marystern
Copy link

Dear gopherbot, please can you re-open this issue?

I am having this exact same problem when trying to use local code with go version go1.16

Something that used to work no longer works, so I think this is a clear bug!

(FYI: ref my "local builds" issue #43829 which details a new "standard" way of working which is the best that we can do for now with modules until it's properly fixed for local code. Sadly, 1.16 seems to have broken even this way of working!)

@marystern
Copy link

retried with go version "go version go1.16.4 linux/amd64", and the same problem exists.

However, doing a "mod tidy" works around the issue (presumably by adding a:

require _/xxx v0.0.0-00010101000000-000000000000

and then it builds ok (but of course, this step should not be required?...)

@mvdan mvdan reopened this May 21, 2021
@mvdan mvdan removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 21, 2021
@mvdan mvdan reopened this May 21, 2021
@marystern
Copy link

Note: the issue seems to be known:

ref: https://stackoverflow.com/questions/66469396/go-module-is-found-and-replaced-but-not-required
"
To summarize: this is a side-effect of the change to default to -mod=readonly as of Go 1.16. Your build succeeded before because the default behavior was -mod=mod, which causes go build to implicitly add new requirements as needed. With the new default, you need to add those requirements explicitly (such as with go mod tidy or go get) before building. – bcmills Mar 11 at 14:49
"

So my question now @bcmills is: why was this default behavior changed as it now increases the barrier to entry for simple tasks (ie run a simple, formerly quick, test!)? And, it also creates for a really strange and confusing coding experience where you have to look up this issue to simply run a short test of some package: it's not intuitive at all!

Before:
go mod init

Now:
go mod init
go mod tidy (whenever you add a new local dependency?)

@marystern
Copy link

Maybe a simple fix is to change the error message, ie, instead of this (which I knew I didn't want to do):

main.go:4:2: module _/xxx provides package _/xxx/log and is replaced but not required; to add it:
	go get _/xxx

tell the user that a simple "go mod tidy" would also work (and avoid hitting the network), eg:

main.go:4:2: module _/xxx provides package _/xxx/log and is replaced but not required; to add it:
	go get _/xxx
or
        go mod tidy

`

@marystern
Copy link

Sorry for the multiple comments, but I'm noting these as they occur.

@bcmills you said "add new requirements as needed", so this suggests that having a "replace" directive does not imply a "require": my thought would be that if i add a replace directive, it's clearly an indication that I also "require" that same package. Maybe the additional step is that if it's not remote, it's a local dependency and should hence always be added? (I admit I find all of this too confusing!)

@jayconrod
Copy link
Contributor

This is working as intended.

replace may be used without a corresponding require in the main module's go.mod. That's especially useful when fixing a bug in an indirect dependency. If the version that the replace refers to isn't actually part of the module graph (that is, it's not required directly or indirectly), perhaps the go command should report an error that it's unused. That would be difficult to reconcile with lazy loading though (#36460).

replace doesn't imply require. If it did, that may cause a significant change in selected versions: the go command would read the go.mod file from the replacement (which isn't currently read if it's not required), then apply the version constraints from that file.

About adding go mod tidy to the error message: go mod tidy does many things and can delete requirements. I think it's usually the right way to add missing requirements, but since it has other side effects, go get is a better command to recommend since it's narrower.

@marystern
Copy link

My problem with "go get" is "go get, on the other hand, will perform a network lookup" (and I'm trying to work 100% locally with no network lookups (hence my preference for "go mod tidy").

Also, can you point me to any information on why the default mode was changed (ie away from -mod=mod which seemed to work ok)? Thanks.

@jayconrod
Copy link
Contributor

@marystern Both go get and go mod tidy may do network lookups for missing imports. I'm not sure you'll find one has an advantage over the other in this case.

#40728 was the issue to change the default to -mod=readonly.

@marystern
Copy link

Thanks @jayconrod , issue #40728 was interesting, and linked to a similar concern to mine over "developer flow/ease" in #44212. Personally, I think this is another one of those "rough edges" that still needs ironing out (getting going in go is proving harder by the release when it should be getting easier!)

@bcmills
Copy link
Contributor

bcmills commented May 21, 2021

@marystern

My problem with "go get" is "go get, on the other hand, will perform a network lookup" (and I'm trying to work 100% locally with no network lookups (hence my preference for "go mod tidy").

Note that as of Go 1.16 go get will fall back to replacement modules if the network lookup fails with a “not found” error. (The fact that it didn't before was a arguably a bug — #32567.)

So if you set GOPROXY=off (to tell the go command that you really don't want it to hit the network, causing every access of the proxy to fail with a “not found” error), you can still use the ordinary go get workflow to adjust the dependency graph.

@marystern
Copy link

A final note (sorry, I've only just realized this after the issue was closed). The critical thing, in hindsight, was my reading and comprehension of the error message.

The message:

main.go:4:2: module _/xxx provides package _/xxx/log and is replaced but not required; to add it:
	go get _/xxx

is clear, simple English, and hence, sadly failed to shout the problem out clearly to me (ie I interpreted "but not required" as a general statement, not as something that referred to a missing "require" clause!)

So, here's another suggestion that I believe would make this clearer.
Change the message to be more explicit:

main.go:4:2: module _/xxx provides package _/xxx/log and is replaced but not required; to add it:
  a) add a "require _/xxx" clause to go.mod, or
  b) Use one of the following to update go.mod for you
	go get _/xxx
        go mod tidy  [optional]

You might dislike this too, but I tried at least! :)

@jayconrod
Copy link
Contributor

If there's a way to rephrase the error message to make it more clear what's wrong, I think that's reasonable. However, we want to keep it concise, and recommend a single command to fix the problem without other side effects. go mod tidy has potentially destructive side effects (it may delete require directives and sums), so we almost never recommend it in error messages, even if it's the easiest command to use most of the time.

Something like this might get the point across, but it still seems pretty verbose to me.

main.go:4:2: module _/xxx provides package _/xxx/log and is replaced but not referenced with a require directive; to add it:
	go get _/xxx

@gll940129

This comment has been minimized.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants