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

Open
yittg opened this issue Aug 2, 2019 · 5 comments

Comments

@yittg
Copy link

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

This comment has been minimized.

Copy link
Member

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.

@yittg

This comment has been minimized.

Copy link
Author

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 label Aug 5, 2019

@bcmills

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

commented Aug 12, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.