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 @latest and @upgrade, ignore +incompatible versions if the latest compatible one has a go.mod file #34165

Closed
andig opened this issue Sep 7, 2019 · 14 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

@andig
Copy link
Contributor

andig commented Sep 7, 2019

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

$ go version
go version go1.13 darwin/amd64

I've also seen this on go1.12, so its not recently introduced.

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/andig/Library/Caches/go-build"
GOENV="/Users/andig/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/andig/htdocs/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/andig/htdocs/mbmd/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/w5/_c0nb6n90fn96tzw04dtc6240000gn/T/go-build373981879=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm using github.com/spf13/cobra/doc, with cobra locked in my go.mod:

github.com/spf13/cobra v0.0.5

cobra/doc indirectly depends on blackfriday:

❯ go mod why github.com/russross/blackfriday
# github.com/russross/blackfriday
github.com/volkszaehler/mbmd/cmd
github.com/spf13/cobra/doc
github.com/cpuguy83/go-md2man/md2man
github.com/russross/blackfriday

❯ go mod graph | grep blackfriday
github.com/cpuguy83/go-md2man@v1.0.10 github.com/russross/blackfriday@v1.5.2

What did you expect to see?

Running go get -u I expect module updates. As for blackfriday, that update should remain in the compatible v1 import path.

What did you see instead?

❯ go get -u
go: finding golang.org/x/tools latest
go: finding github.com/influxdata/line-protocol latest
go: finding github.com/crabmusket/gosunspec latest
go: finding github.com/tcnksm/go-latest latest
go: finding golang.org/x/net latest
go: finding github.com/grid-x/modbus latest
go: finding golang.org/x/sys latest
go: finding github.com/influxdata/influxdb1-client latest
go: finding github.com/grid-x/serial latest
# github.com/cpuguy83/go-md2man/md2man
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:11:16: undefined: blackfriday.EXTENSION_NO_INTRA_EMPHASIS
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:12:16: undefined: blackfriday.EXTENSION_TABLES
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:13:16: undefined: blackfriday.EXTENSION_FENCED_CODE
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:14:16: undefined: blackfriday.EXTENSION_AUTOLINK
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:15:16: undefined: blackfriday.EXTENSION_SPACE_HEADERS
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:16:16: undefined: blackfriday.EXTENSION_FOOTNOTES
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:17:16: undefined: blackfriday.EXTENSION_TITLEBLOCK
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/md2man.go:19:29: too many arguments to conversion to blackfriday.Markdown: blackfriday.Markdown(doc, renderer, extensions)
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/roff.go:19:9: cannot use &roffRenderer literal (type *roffRenderer) as type blackfriday.Renderer in return argument:
        *roffRenderer does not implement blackfriday.Renderer (missing RenderFooter method)
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/roff.go:102:11: undefined: blackfriday.LIST_TYPE_ORDERED
../go/pkg/mod/github.com/cpuguy83/go-md2man@v1.0.10/md2man/roff.go:102:11: too many errors

The error is due to pulling blackfriday v2 into go.mod which does not have the required constants from 1.5.2 anymore (https://github.com/russross/blackfriday/tree/v2):

github.com/russross/blackfriday v2.0.0+incompatible // indirect

I feel that go get -u should not pull in the blackfriday v2 dependency as it is not used. md2man in its latest version still relies on 1.5.2 so import path should not change (https://github.com/cpuguy83/go-md2man/blob/7762f7e404f8416dfa1d9bb6a8c192aa9acb4d19/go.mod):

module github.com/cpuguy83/go-md2man

go 1.12

require github.com/russross/blackfriday v1.5.2

Refs cpuguy83/go-md2man#48

@andig andig changed the title cmd/go: go get -u updates to incompatible module version cmd/go: go get -u updates to incompatible v2 module version Sep 7, 2019
@andig andig changed the title cmd/go: go get -u updates to incompatible v2 module version cmd/go: go get -u updates indirect dependency to incompatible v2 module version Sep 7, 2019
@andig
Copy link
Contributor Author

andig commented Sep 10, 2019

I've added https://github.com/andig/blackfriday/ for a small demo of the go get -u behaviour with a direct dependency. Any insight what is wrong here would be highly appreciated.

@andig andig changed the title cmd/go: go get -u updates indirect dependency to incompatible v2 module version cmd/go: go get -u updates dependency to incompatible v2 module version Sep 10, 2019
@FiloSottile FiloSottile added modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 10, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Sep 10, 2019
@FiloSottile
Copy link
Contributor

/cc @bcmills @jayconrod

@jayconrod
Copy link
Contributor

This is currently working as designed, but the behavior is unfortunate. #34217 discusses some alternative behaviors.

The reason this happens is that a v2.0.0 tag was added to blackfriday before they migrated to modules, along with the breaking changes that a major version bump implies. Normally, the go command requires v2.0.0 to be associated with modules whose paths end with a /v2 suffix (github.com/russross/blackfriday/v2), but that's only mandatory for repositories that have added a go.mod file. Without a go.mod file, v2.0.0 is considered part of the original module, and go get -u upgrades over the breaking change.

#34217 is a pretty large proposal, but perhaps a narrower fix for this issue would be that go get -u should not upgrade across incompatible major versions.

@andig
Copy link
Contributor Author

andig commented Sep 12, 2019

Normally, the go command requires v2.0.0 to be associated with modules whose paths end with a /v2 suffix (github.com/russross/blackfriday/v2), but that's only mandatory for repositories that have added a go.mod file.

Could you elaborate on that? There ist a go.mod file in place: https://github.com/russross/blackfriday/blob/master/go.mod

Or is what we‘re seeing here (keeping https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher in mind) the added case of a go.mod in place but not following the spec for case 1) where it needs to add a /v2 to the module name?

@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2019

There [is] a go.mod file in place:

Not in v2.0.0:
https://github.com/russross/blackfriday/tree/v2.0.0

The go.mod file was added in v2.0.1. IMO that is not the appropriate tag according to the rules at https://semver.org — adding a go.mod file is “adding functionality”, not a “backwards compatible bug fix”, so it should be at least a minor release — but it is what it is.

At any rate, because v2.0.0 lacks a go.mod file' the go command still allows it to be used with the +incompatible suffix, and given support for +incompatible at all it is correct to do so: that version could be used for github.com/russross/blackfriday before v2.0.1 was introduced, so it should continue to be valid afterwards.

@bcmills
Copy link
Contributor

bcmills commented Sep 12, 2019

If we don't do #34217, then we should consider this as an alternative.

If there is no existing requirement on a module at all, we should probably also prefer the latest compatible version with a go.mod file (if such a version exists) over the semantically-highest +incompatible version.

Unfortunately, that doesn't address the problem of what to do if one of your dependencies already requires an incompatible major version higher than what your own package needs. (At that point, it seems that the only scalable option is to update to the new API and upgrade your way out, and try to stick to dependencies that follow the import compatibility rule from there on out.)

@andig
Copy link
Contributor Author

andig commented Sep 14, 2019

Without a go.mod file, v2.0.0 is considered part of the original module, and go get -u upgrades over the breaking change.

That is indeed very unfortunate as it seems to contradict semver or just expected behavior.

If there is no existing requirement on a module at all, we should probably also prefer the latest compatible version with a go.mod file (if such a version exists) over the semantically-highest +incompatible version.

+1. I do not understand the incompatible use case (need to read up) but sticking to compatible versions would be what I‘d expect without reading the docs.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 15, 2019

If we don't do #34217, then we should consider this as an alternative.

I withdrew #34217, so I think we should do this. Not sure whether it will make 1.14, though.

@bcmills bcmills changed the title cmd/go: go get -u updates dependency to incompatible v2 module version cmd/go: for @latest and @upgrade, ignore +incompatible versions if the latest compatible one has a go.mod file Oct 15, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 23, 2019

I think we should do this in 1.14. I'll see if I can get something in before the freeze.

@bcmills bcmills modified the milestones: Backlog, Go1.14 Oct 23, 2019
@bcmills bcmills self-assigned this Oct 23, 2019
@gopherbot
Copy link

Change https://golang.org/cl/204440 mentions this issue: cmd/go: avoid upgrading to +incompatible versions if the latest compatible one has a go.mod file

@gopherbot
Copy link

Change https://golang.org/cl/204439 mentions this issue: cmd/go/internal/modfetch: prune +incompatible versions more aggressively

gopherbot pushed a commit that referenced this issue Nov 5, 2019
codeRepo.Versions previously checked every possible +incompatible
version for a 'go.mod' file. That is wasteful and counterproductive.

It is wasteful because typically, a project will adopt modules at some
major version, after which they will (be required to) use semantic
import paths for future major versions.

It is counterproductive because it causes an accidental
'+incompatible' tag to exist, and no compatible tag can have higher
semantic precedence.

This change prunes out some of the +incompatible versions in
codeRepo.Versions, eliminating the “wasteful” part but not all of the
“counterproductive” part: the extraneous versions can still be fetched
explicitly, and proxies may include them in the @v/list endpoint.

Updates #34165
Updates #34189
Updates #34533

Change-Id: Ifc52c725aa396f7fde2afc727d0d5950acd06946
Reviewed-on: https://go-review.googlesource.com/c/go/+/204439
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/212317 mentions this issue: cmd/go: adjust heuristics for skipping +incompatible versions

@bcmills
Copy link
Contributor

bcmills commented Jan 8, 2020

We discovered a regression for certain queries (such as github.com/stripe/stripe-go@latest) that will need to be fixed before the release.

The fix is under review at https://golang.org/cl/212317.

@bcmills
Copy link
Contributor

bcmills commented Jan 8, 2020

The regression is now fixed.

@bcmills bcmills closed this as completed Jan 8, 2020
gopherbot pushed a commit that referenced this issue Jan 8, 2020
We know of at least one module (github.com/stripe/stripe-go) that has
a run of +incompatible versions, followed by a run of versions with
go.mod files, followed by another run of +incompatible versions.

We want the heuristics for showing +incompatible versions to reflect
the authors' current intent, and it seems clear that the current
intent of the authors of that module is for users of the unversioned
import path to still be on +incompatible versions.

To respect that intent, we need to keep checking for +incompatible
versions even after we have seen a lower major version with an
explicit go.mod file.

However, we still don't want to download every single version of the
module to check it. A given major version should have a consistent,
canonical import path, so the path (as inferred by the presence or
absence of a go.mod file) should be the same for every release across
that major version.

To avoid unnecessary overhead — and to allow module authors to correct
accidental changes to a major version's import path — we check only
the most recent release of each major version. If a release
accidentally changes the import path in either direction (by deleting
or adding a go.mod file), it can be corrected by issuing a single
subsequent release of that major version to restore the correct path.

I manually verified that, with this change,
github.com/stripe/stripe-go@latest reverts to v68.7.0+incompatible
as it was in Go 1.13.
The other regression tests for #34165 continue to pass.

Updates #34165

Change-Id: I5daff3cd2123f94c7c49519babf4eecd509f169e
Reviewed-on: https://go-review.googlesource.com/c/go/+/212317
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Jan 7, 2021
@rsc rsc unassigned bcmills Jun 23, 2022
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

6 participants