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: for v0 dependencies, consider changing 'go get -u[=patch]' behavior, or otherwise improve related behavior #28396

Closed
thepudds opened this issue Oct 25, 2018 · 7 comments
Labels
FrozenDueToAge modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@thepudds
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

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

go version go1.11.1 windows/amd64

Background

The current 1.11 behavior for go get -u and go get -u=patch is nice overall, and the ability to easily upgrade to the latest available dependencies is a nice counterbalance to default MVS behavior for indirect dependencies (where it tends to select older versions for indirect dependencies compared to a more traditional approach).

However, the current behavior for go get -u and go get -u=patch works best when the dependencies are v1 or higher, and there is some danger when some dependencies are instead in the v0 "compatibility free zone". This is especially true for any indirect v0 dependencies (given the author of the current top-level module has less visibility/insight into indirect dependencies).

Suggestion

Consider changing the default behavior of go get -u and go get -u=patch to work better with v0 dependencies.

I am not necessarily trying to suggest a specific solution, but for illustration purposes, some alternative approaches might be:

  1. keep go get -u behavior as it currently defined for 1.11 modules, but redefine go get -u=patch to not upgrade any v0 dependencies.

  2. keep go get -u and go get -u=patch behavior as they are currently defined for 1.11 modules, but introduce a more conservative go get -u=somenewflag that upgrades all v1+ dependencies while leaving v0 dependencies alone.

  3. introduce a go get -u=all that is the current and more aggressive go get -u 1.11 modules behavior, and redefine go get -u and go get -u=patch to not upgrade any v0 dependencies

(And for any of the alternatives that "leave v0 dependencies alone" but upgrade v1+ dependencies, one of those upgraded v1+ dependencies might include a require that then triggers a v0 upgrade. Presumably that would be allowed, but the require would be a strong hint that that particular v0 version was at least tested by one or more v1+ dependency).

If #28395 or similar is adopted, then that would likely play into what the desired behavior here would be (e.g., alternative 2 might be more appealing in that case).

I suspect most of the comments here apply to both normal v0 versions (such as v0.1.0) as well as v0 pseudo-versions, though something like an upgrade from v0.0.0-20140202185014-0b12d6b521d8 to v0.0.0-20180823135443-60711f1a8329 does not have a concept of a patch vs. non-patch upgrade.

Finally, a related issue is that many pre-existing repos include instructions that state the user should do go get -u foo. A quick and unscientific survey shows that 3 of the first 5 packages listed under "Popular Packages" on https://godoc.org currently include go get -u foo instructions. Some of those existing instructions likely assume pre-modules vendoring behavior or otherwise rely on pre-module behavior. In a module world, leftover go get -u foo instructions are somewhat dangerous if there are v0 dependencies, but it might take some time for pre-existing repos to alter or remove go get -u instructions (if that is what ends up being needed).

@thepudds
Copy link
Contributor Author

This issue is more about the general concern, but at this point I have seen multiple problems with modules where someone followed older instructions saying go get -u foo that then upgraded them to an incompatible v0 dependency.

One concrete example was that if you opted in to modules, and then followed the installation instructions for aws-sdk-go on the AWS site to do go get -u github.com/aws/aws-sdk-go, and then tried to use aws-sdk-go in your top-level project, it would fail to build with error:

.../go/pkg/mod/github.com/jmespath/go-jmespath@v0.0.0-20180206201540-c2b33e8439af/api.go:8:7: undefined: ASTNode

I first saw this example with v1.15.21 of aws-sdk-go, which did have a go.mod:

https://github.com/aws/aws-sdk-go/blob/v1.15.21/go.mod

module github.com/aws/aws-sdk-go

require (
	github.com/go-ini/ini v1.25.4
	github.com/jmespath/go-jmespath v0.0.0-20160202185014-0b12d6b521d8
)

The go-jmespath version listed in the aws-sdk-go go.mod file worked, but the AWS instructions of go get -u github.com/aws/aws-sdk-go would cause the version of go-jmespath to be upgraded from the v0.0.0-20160202... version in the aws-sdk-go go.mod to be the latest go-jmespath version v0.0.0-20180206..., which would then fail to compile.

That particular instance of this problem was diagnosed and then worked around (by not using -u), but it is an example of problematic -u behavior for v0 dependencies, as well as an example of the overhang of old install instructions that include -u.

@bcmills bcmills added this to the Go1.13 milestone Oct 26, 2018
@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 13, 2018
@bcmills
Copy link
Contributor

bcmills commented Nov 13, 2018

We certainly should not change -u=patch for v0 dependencies: the patch component of the version has a common, well-defined meaning even at v0. If repo owners issue patch versions with breaking changes, that seems like a social issue more than a technical one. (Note that @jba is working on a tool to automatically suggest a version bump based on the changes to the exported API.)

@bcmills
Copy link
Contributor

bcmills commented Nov 13, 2018

As you note, the possibility of breaking changes with go get -u is not unique to module mode: the same instructions are already dangerous in GOPATH mode.

At any rate, it's not at all clear to me why the folks maintaining aws-sdk-go didn't either fix their installation instructions or make the library compatible with the upgraded dependency in the commit that added the go.mod file. (@jasdel, was that an oversight or an intentional decision?)

@jasdel
Copy link
Contributor

jasdel commented Nov 16, 2018

Thanks @bcmills this was an oversight, the instructions on aws-sdk-go need to be updated to accounted for the go module use case.

@thepudds
Copy link
Contributor Author

@jasdel could you comment briefly on the prior instructions that used go get -u? Was the approach to rely on a vendor directory being there, for example?

@jasdel
Copy link
Contributor

jasdel commented Nov 16, 2018

aws-sdk-go used the -u flag in the README to simplify the instructions for both getting the library originally, and retrieving an update. We had feedback from users initially that thought they were updating the library, but not actually getting any update because they forgot to use the -u flag.

Since the library used a vendor directory the usage of the update flag did not impact users getting the library using Go 1.10 and earlier since the dependencies were vendored. This behavior changed when a module file (go.mod) is defined with Go 1.11's module support enabled. The update flag does get the latest dependencies, ignoring the content of the vendor folder. I've updated the library's instructions.

Also @thepudds would you mind opening up a GitHub issue with aws-sdk-go for tracking why the library isn't working with the latest version of go-jmespath.

jasdel added a commit to jasdel/aws-sdk-go that referenced this issue Nov 16, 2018
Updates the SDK's README.md to clarify the instructions for getting the
SDK. This also avoids the potential issues getting the SDK with Go
modules and retrieving a non-working version of SDK dependencies.

Related to golang/go#28396
jasdel added a commit to aws/aws-sdk-go that referenced this issue Nov 16, 2018
Updates the SDK's README.md to clarify the instructions for getting the
SDK. This also avoids the potential issues getting the SDK with Go
modules and retrieving a non-working version of SDK dependencies.

Related to golang/go#28396
@bcmills
Copy link
Contributor

bcmills commented Jan 17, 2019

Even if there are breaking changes within v0, they are often confined to relatively low-usage parts of the API, so actual breaks should be relatively rare even within v0 releases: I don't think it's worth holding back upgrades “just in case”, especially given that they're easy to revert if they fail.

Note that the @patch suffix we're discussing for #26812 should make it straightforward to write a wrapper script that upgrades only v1+ modules. Perhaps experiment with that outside of the go tool, and we can revisit based on whether folks tend to use/need it.

@bcmills bcmills closed this as completed Jan 17, 2019
@golang golang locked and limited conversation to collaborators Jan 17, 2020
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.
Projects
None yet
Development

No branches or pull requests

4 participants