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: go build, list etc can upgrade existing dependency versions #28692

Closed
rogpeppe opened this issue Nov 9, 2018 · 12 comments
Closed

cmd/go: go build, list etc can upgrade existing dependency versions #28692

rogpeppe opened this issue Nov 9, 2018 · 12 comments
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Nov 9, 2018

go version devel +644ddaa842 Wed Nov 7 16:12:02 2018 +0000 linux/amd64

% cat go.mod
module example.com/foo

require golang.org/x/tools v0.0.0-20181008205924-a2b3f7f249e9
% go mod graph
example.com/foo golang.org/x/tools@v0.0.0-20181008205924-a2b3f7f249e9
% cat main.go
package main

import _ "golang.org/x/tools/go/packages/packagestest"

func main() {
}
% go build
go: finding golang.org/x/tools/go/packages/packagestest latest
go: finding golang.org/x/tools/go/packages latest
go: finding golang.org/x/tools/go latest
go: finding golang.org/x/tools latest
% git diff go.mod
diff --git a/go.mod b/go.mod
index de400c6..5040d7d 100644
--- a/go.mod
+++ b/go.mod
@@ -1,3 +1,3 @@
 module example.com/foo
 
-require golang.org/x/tools v0.0.0-20181008205924-a2b3f7f249e9
+require golang.org/x/tools v0.0.0-20181109152631-138c20b93253
% go list -m golang.org/x/tools@latest
go: finding golang.org/x/tools latest
golang.org/x/tools v0.0.0-20181109152631-138c20b93253
% go mod graph
example.com/foo golang.org/x/tools@v0.0.0-20181109152631-138c20b93253

It seems that because the listed dependency does not contain the imported package, the go tool is silently upgrading to the latest version. This happens with go list too.

Upgrading a dependency is a significant action and should not, I believe, happen as a result of go build or go list.

I'd expect a "package not found" error instead.

@rogpeppe rogpeppe changed the title cmd/go: go build, test etc can change existing dependencies cmd/go: go build, test etc can upgrade existing dependency versions Nov 9, 2018
@rogpeppe rogpeppe changed the title cmd/go: go build, test etc can upgrade existing dependency versions cmd/go: go build, list etc can upgrade existing dependency versions Nov 9, 2018
@hyangah
Copy link
Contributor

hyangah commented Nov 9, 2018

Is this because golang.org/x/tools uses pseudo-version?

@bcmills
Copy link
Contributor

bcmills commented Nov 9, 2018

It's also possible for an arbitrary new dependency to bump up the requirements for existing dependencies.

For example, suppose that golang.org/x/tools/go were split into its own module. Then go build might resolve it by adding a requirement on golang.org/x/tools/go, which may itself impose some minimum requirement on golang.org/x/tools.

I suppose we could add a special case for “found the package in a newer version of an existing requirement”, but in that case the resolution is clear anyway: the obvious thing to do is to upgrade to some version that contains the package in question, and our default for “some version” is always latest. Given that there is a clear step we can take to resolve the import, it seems like we should just do that instead of making the user issue another command.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 9, 2018
@bcmills bcmills added this to the Go1.12 milestone Nov 9, 2018
@rogpeppe
Copy link
Contributor Author

rogpeppe commented Nov 9, 2018

It's also possible for an arbitrary new dependency to bump up the requirements for existing dependencies.

Yes, but I don't believe that the go.mod dependencies need to be updated in such a case. The MVS calculations will increase the final required version, but the requirements in the local go.mod file should not need to be upgraded accordingly IMHO, because the final MVS calculation results will reflect that new version anyway.

I am still of the opinion that go list (and go build etc) should never alter existing module versions stated in go.mod.

Preserving the existing version requirement is important because the dependency that leads to a greater required version may only be an internal implementation detail that may change over time. If we later change to a dependency that doesn't require the later version, then it's fine (and desirable) for the minimum version requirement to drop back to the originally stated version.

@bcmills
Copy link
Contributor

bcmills commented Nov 9, 2018

The MVS calculations will increase the final required version, but the requirements in the local go.mod file should not need to be upgraded accordingly IMHO, because the final MVS calculation results will reflect that new version anyway.

In general we maintain the invariant that the go.mod file does not include misleading requirements: if you explicitly asked for v1.0.0 but you're actually getting v1.1.0, the next go command to notice that will remove the entry for v1.0.0. (Search for “misleading requirements” in https://golang.org/cmd/go/#hdr-The_go_mod_file.)

Preserving the existing version requirement is important because the dependency that leads to a greater required version may only be an internal implementation detail that may change over time.

The only notion of “time” that Go modules really support is “versions” — even the timestamps of pseudo-versions are converted to fragments of the version string. If some published version of the module has a higher version requirement, there's little reason to go back: the cat is already out of the bag.

On the other hand, if you're just talking about unpublished versions, it's easy enough to recover a minimal change to the go.mod file by checking out the version from the most recent tagged version and then re-running go mod tidy. In general, the go command does not distinguish between “removing a dependency” and “undoing the addition of a dependency”: it supports the former, but leaves the latter up to the user's version-control system.

If we later change to a dependency that doesn't require the later version, then it's fine (and desirable) for the minimum version requirement to drop back to the originally stated version.

It might be fine? On the other hand, if we've incidentally picked up a fix to some bug and then written some code that depends on that fix, dropping it back out as a side-effect of removing some other package would be very frustrating.

I understand where you're coming from, but I don't think the benefit of minimizing changes outweighs the added subtlety from preserving misleading requirements.

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Nov 9, 2018

In general we maintain the invariant that the go.mod file does not include misleading requirements: if you explicitly asked for v1.0.0 but you're actually getting v1.1.0, the next go command to notice that will remove the entry for v1.0.0. (Search for “misleading requirements” in https://golang.org/cmd/go/#hdr-The_go_mod_file.)

Ah, I'd missed that. That's very interesting (and I think I'm on board with your reasoning there, although I have misgivings because ever-spreading test dependencies could result in a much greater min version requirement than needed) but it doesn't cover this particular case AFAICS.

I suppose we could add a special case for “found the package in a newer version of an existing requirement”, but in that case the resolution is clear anyway: the obvious thing to do is to upgrade to some version that contains the package in question, and our default for “some version” is always latest. Given that there is a clear step we can take to resolve the import, it seems like we should just do that instead of making the user issue another command.

I honestly think that that special case would be worth having. In this case, the upgrade caused the code to fail (because go/packages hasn't been maintaining backward compatibility) and it was frustrating trying to work out what was going on and why my explicitly stated version requirement was being overridden every time with no indication as to why.

If nothing else, the tool should say explicitly what has happened (in fact I think that it should be shout a little louder every time it adds or changes a dependency in go.mod, but that's probably another issue).

@bcmills
Copy link
Contributor

bcmills commented Nov 9, 2018

In this case, the upgrade caused the code to fail (because go/packages hasn't been maintaining backward compatibility) and it was frustrating trying to work out what was going on and why my explicitly stated version requirement was being overridden every time with no indication as to why.

Perhaps we need a go mod whynot, to tell you which packages would break if you downgraded to a given set of module versions? 🙂

@rogpeppe
Copy link
Contributor Author

FWIW I could downgrade just fine using go get. Any subsequent operation undid that operation though.

@myitcv
Copy link
Member

myitcv commented Nov 13, 2018

@rogpeppe - do you think this issue's original question is now answered? Does it make sense therefore to re-title it to something along the lines of @bcmills's suggestion?

Perhaps we need a go mod whynot, to tell you which packages would break if you downgraded to a given set of module versions?

@bcmills
Copy link
Contributor

bcmills commented Dec 8, 2020

I think this is addressed by #40728 (for Go 1.16). @rogpeppe, do you agree?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 8, 2020
@rogpeppe
Copy link
Contributor Author

rogpeppe commented Dec 9, 2020

I think this is addressed by #40728 (for Go 1.16). @rogpeppe, do you agree?

Yes in general, but it's a pity that AFAICS (maybe the behaviour has changed since then) you still can't run go list at all even when something quite minor such as go.sum needs to be updated. For go list at any rate, perhaps "best effort" would be appropriate. That said, it's a big step forward!

@bcmills
Copy link
Contributor

bcmills commented Dec 9, 2020

For go list at any rate, perhaps "best effort" would be appropriate.

go list -e should make a best effort in -mod=readonly mode. (If it is not moving past errors that you think it should, please file a separate issue for that — last I checked, list -e still has a lot of room for improvement.)

@bcmills
Copy link
Contributor

bcmills commented Dec 9, 2020

Duplicate of #40728

@bcmills bcmills marked this as a duplicate of #40728 Dec 9, 2020
@bcmills bcmills closed this as completed Dec 9, 2020
@golang golang locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

7 participants