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

x/mod/module: CheckPath validation inconsistent for invalid major-version suffixes #33429

Closed
yittg opened this issue Aug 2, 2019 · 6 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

@yittg
Copy link

yittg commented Aug 2, 2019

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

$ go version
go version devel +2d6ee6e89a Thu Aug 1 20:37:08 2019 +0000 darwin/amd64

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

What did you do?

i'm not quit sure whether it's a bug cause i can not find the spec about it, reading the code in src/cmd/go/internal/module,

i think about a case like x.y/z/v2.0.0-pre for versions with prerelease tag, whether it should get the same checking result with x.y/z/v2.0,
for now, they get results like following:

	{"x.y/z/v2.0", false, true, true},
	{"x.y/z/v2.0.0-pre", true, true, true},

if this case make sense, i would like to push a PR to fix it, thanks.

@bcmills
Copy link
Contributor

bcmills commented Aug 2, 2019

reading the code in src/cmd/go/internal/module,

That package contains a lot of code. Which function in particular are you concerned about?

Note that many of the functions in that package are now available as golang.org/x/mod/module, which can be used in the Playground. A complete code example illustrating the concern would be helpful.

@bcmills bcmills added 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. labels Aug 2, 2019
@yittg
Copy link
Author

yittg commented Aug 2, 2019

@bcmills thanks for your reply, it's the case set checkPathTests used by TestCheckPath that i illustrated before. Particularly, it mainly about the result of CheckPath > SplitPathVersion .
for more clear detail, see https://play.golang.org/p/gTBzMfZUCZT, which print like following:

x.y/z/v2.0 check path got: malformed module path "x.y/z/v2.0": invalid version
x.y/z/v2.0.0-pre check path got: <nil>
x.y/z/v2.0 split path version got: x.y/z/v2.0,,false
x.y/z/v2.0.0-pre split path version got: x.y/z/v2.0.0-pre,,true

@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 5, 2019
@bcmills
Copy link
Contributor

bcmills commented Aug 5, 2019

The intent is for the path suffix to include only the major version, since minor-version updates to that path will all be compatible. (So the path should be x.y/z/v2, not x.y/z/v2.0.)

So the error for x.y/z/v2.0 seems correct, although perhaps we should not treat v2.0 as a major-version suffix at all. The fact that we do not treat v2.0.0-pre the same way could be a bug.

Do you have concrete examples of paths of either form in the wild?

CC @jayconrod @hyangah @heschik @katiehockman

@bcmills bcmills added this to the Go1.14 milestone Aug 5, 2019
@bcmills bcmills changed the title cmd/go/internal/module: module check path with prerelease version x/mod/module: CheckPath validation inconsistent for invalid major-version suffixes Aug 5, 2019
@yittg
Copy link
Author

yittg commented Aug 6, 2019

So the error for x.y/z/v2.0 seems correct, although perhaps we should not treat v2.0 as a major-version suffix at all. The fact that we do not treat v2.0.0-pre the same way could be a bug.

from the implementation, all version-like suffix /vX.Y[.Z]... will lead to this error, so thinking about whether other version pattern like /vX.Y[.Z]...[-prerelease[+buildmeta]] should lead to same error, + is not accepted in other place checking repo path, so looks like only prerelease is not coverd.

and the format is not need to strictly fit the semver, so may all version-like format "/vX.Y[.Z]...-SOMETHING` should be avoided?

Do you have concrete examples of paths of either form in the wild?

No, totally imagined, :)

@yittg
Copy link
Author

yittg commented Aug 12, 2019

@bcmills any news about this issue? does some more discussion need about how to address it?

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

bcmills commented Jun 4, 2020

(CC @matloob)

@yittg yittg closed this as not planned Won't fix, can't repro, duplicate, stale Jan 17, 2023
@golang golang locked and limited conversation to collaborators Jan 17, 2024
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