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 mod edit -exclude' erroneously rejects '+incompatible' versions [1.16 backport] #44498

Closed
gopherbot opened this issue Feb 22, 2021 · 6 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 22, 2021

@bcmills requested issue #44497 to be considered for backport to the next 1.16 minor release.

@gopherbot, please backport to Go 1.16: this results in confusing (and incorrect) errors on the command line.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 24, 2021

Change https://golang.org/cl/295930 mentions this issue: [release-branch.go1.16] cmd/go: fix version validation in 'go mod edit -exclude'

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Feb 24, 2021

Change https://golang.org/cl/295931 mentions this issue: [internal-branch.go1.16-vendor] modfile: check canonicalness against the relevant module path, not abstract semver

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Feb 25, 2021

Approved. This is a serious issue.

gopherbot pushed a commit to golang/mod that referenced this issue Mar 1, 2021
…the relevant module path, not abstract semver

In CL 279394 we started validating that versions in "exclude" and
"retract" directives are canonical. Unfortunately, we use the semver
package's notion of canonicalness, and the semver package doesn't know
anything about +incompatible versions or major-version suffixes.

The resulting error messages also don't indicate an appropriate fix if
the problem is that the user forgot either the "+incompatible" suffix
on the version string or the "/vN" suffix on the module path.

This change corrects both of those problems by validating the version
against the corresponding module path. (For "exclude" directives, that
is the module path to be excluded; for "retract" directives, it is the
module declared in the "module" directive of the same go.mod file.)

Updates golang/go#44497
For golang/go#44498

Change-Id: I39732d79c3ab3a43bb1fb8905062fe6cb26d3edc
Reviewed-on: https://go-review.googlesource.com/c/mod/+/295089
Trust: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
(cherry picked from commit 66f6d92)
Reviewed-on: https://go-review.googlesource.com/c/mod/+/295931
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Mar 1, 2021

There's an unresolved comment in CL 295930 that needs to resolved, then it can be submitted.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 1, 2021

CL 295930 is now ready to go in whenever the TryBots are happy.

@dmitshur dmitshur assigned dmitshur and unassigned bcmills and dmitshur Mar 1, 2021
gopherbot pushed a commit that referenced this issue Mar 1, 2021
…t -exclude'

The fix is to pull in CL 295931 from the x/mod repo.

Updates #44497
Fixes #44498

Change-Id: I008b58d0f4bb48c09d4f1e6ed31d11a714f87dc0
Reviewed-on: https://go-review.googlesource.com/c/go/+/295150
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
(cherry picked from commit 691ac80)
Reviewed-on: https://go-review.googlesource.com/c/go/+/295930
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Mar 1, 2021

Closed by merging e0bd146 to release-branch.go1.16.

@gopherbot gopherbot closed this Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants