[go] cmd/go: make mod init disallow invalid major version suffixes
Paschalis Tsilias (Gerrit)
Paschalis Tsilias has uploaded this change for review.
cmd/go: make mod init disallow invalid major version suffixes
This CL reuses the SplitPathVersion function from the module package to
detect invalid major version suffixes, and return a relevant error
message.
Fixes #44052
Change-Id: I6c06f31a134e864a1d9b6e00c048ca1c59b4365e
---
M src/cmd/go/internal/modload/init.go
A src/cmd/go/testdata/script/mod_init_invalid_major.txt
2 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go
index bc8d17e..da6483a 100644
--- a/src/cmd/go/internal/modload/init.go
+++ b/src/cmd/go/internal/modload/init.go
@@ -490,6 +490,10 @@
return errorf("contains disallowed path separator character %q", p[i])
}
+ if _, _, ok := module.SplitPathVersion(p); !ok {
+ return errorf("major version suffixes must be in the form of /vN and are only allowed for v2 or later")
+ }
+
// Ensure path.IsAbs and build.IsLocalImport are false, and that the path is
// invariant under path.Clean, also to avoid confusing the module cache.
if path.IsAbs(p) {
diff --git a/src/cmd/go/testdata/script/mod_init_invalid_major.txt b/src/cmd/go/testdata/script/mod_init_invalid_major.txt
new file mode 100644
index 0000000..d3813d1
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_init_invalid_major.txt
@@ -0,0 +1,23 @@
+env GO111MODULE=on
+env GOFLAGS=-mod=mod
+
+! go mod init example.com/user/repo/v0
+stderr '^go: invalid module path "example.com/user/repo/v0": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
+
+! go mod init example.com/user/repo/v02
+stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
+
+! go mod init example.com/user/repo/v1
+stderr '^go: invalid module path "example.com/user/repo/v1": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
+
+! go mod init example.com/user/repo/v2.0
+stderr '^go: invalid module path "example.com/user/repo/v2.0": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
+
+! go mod init example.com/user/repo/v2.1.4
+stderr '^go: invalid module path "example.com/user/repo/v2.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
+
+! go mod init example.com/user/repo/v2.
+stderr '^go: invalid module path "example.com/user/repo/v2.": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
+
+! go mod init example.com/user/repo/v.2
+stderr '^go: invalid module path "example.com/user/repo/v.2": major version suffixes must be in the form of /vN and are only allowed for v2 or later$'
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/internal/modload/init.go:
Patch Set #1, Line 494:
errorf("major version suffixes must be in the form of /vN and are only allowed for v2 or later")This assumes a non- gopkg.in module path.
That is certainly the more common kind of path (by far!), but perhaps it's worth emitting a different message for a malformed gopkg.in path?
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
Paschalis Tsilias uploaded patch set #2 to this change.
cmd/go: make mod init disallow invalid major version suffixes
This CL reuses the SplitPathVersion function from the module package to
detect invalid major version suffixes, and return a relevant error
message.
Fixes #44052
Change-Id: I6c06f31a134e864a1d9b6e00c048ca1c59b4365e
---
M src/cmd/go/internal/modload/init.go
A src/cmd/go/testdata/script/mod_init_invalid_major.txt
2 files changed, 51 insertions(+), 0 deletions(-)
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/internal/modload/init.go:
Patch Set #1, Line 494:
errorf("major version suffixes must be in the form of /vN and are only allowed for v2 or later")
This assumes a non- gopkg.in module path. […]
You're absolutely right! I added an alternate error message and some more test cases, according to what splitGopkgIn actually checks, let me know how it looks.
Do you think this should be moved in a helper function to keep things cleaner?
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
Bryan C. Mills removed Ian Lance Taylor from this change.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
Bryan C. Mills removed Russ Cox from this change.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/internal/modload/init.go:
if strings.HasPrefix(p, "gopkg.in/") {
return errorf("module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN")
}
return errorf("major version suffixes must be in the form of /vN and are only allowed for v2 or later")I think we could do one better on these error messages. Could the error message suggest the corrected version of what the user actually entered, so that they can just copy and paste it to try again?
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Paschalis Tsilias, Michael Matloob.
Paschalis Tsilias uploaded patch set #3 to this change.
cmd/go: make mod init disallow invalid major version suffixes
This CL reuses the SplitPathVersion function from the module package to
detect invalid major version suffixes, and return a relevant error
message along with a suggested fix.
Fixes #44052
Change-Id: I6c06f31a134e864a1d9b6e00c048ca1c59b4365e
---
M src/cmd/go/internal/modload/init.go
A src/cmd/go/testdata/script/mod_init_invalid_major.txt
2 files changed, 127 insertions(+), 0 deletions(-)
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Michael Matloob.
2 comments:
Patchset:
First off, I apologize for taking nearly two months for responding to a small change 😞
Secondly, I've amended the PR with better error messages. I feel that there's room for improvement in the way that we build the suggestions, but please let me know if at least that's aligned with what you had in mind.
Thanks a lot for your time!
File src/cmd/go/internal/modload/init.go:
I think we could do one better on these error messages. […]
Sounds like a great idea! I've added a first implementation for this, and modified the test script accordingly.
The two functions `suggestGopkgIn` and `suggestModulePath` which build the corrected 'suggestions' can definitely be improved a lot; I just wanted to know if that's the direction you were thinking of.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Michael Matloob.
5 comments:
File src/cmd/go/internal/modload/init.go:
Patch Set #3, Line 569:
; eg. gIf we're suggesting a `go mod init` command for the user to run, separate it with \n\t, like:
… in the form of .vN:
go mod init gopkg.in/banana.v1The Go project avoids Latin abbreviations in user-facing messages and documentation. Prefer English phrases like “such as” or “for example”.
(But see the comment below; I think a colon and newline would be more consistent with our other error messages here.)
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'I don't think these two cases should be allowed. `v02` and `v023` are not prefixes of any valid semantic version.
! go mod init example.com/user/repo/v2.0
stderr '^go: invalid module path "example.com/user/repo/v2.0": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v2.1.4
stderr '^go: invalid module path "example.com/user/repo/v2.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v3.5
stderr '^go: invalid module path "example.com/user/repo/v3.5": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v3$'
! go mod init example.com/user/repo/v4.1.4
stderr '^go: invalid module path "example.com/user/repo/v4.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v4$'I'm not sure whether we should reject these. They seem a lot less confusing than /v0 or /v1, and they could refer to the version of some other thing (such as the version of a C library being wrapped by a module that contains a cgo wrapper).
! go mod init example.com/user/repo/v2.
stderr '^go: invalid module path "example.com/user/repo/v2.": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'I think this path should be rejected on the grounds that it is not a valid import path (per #45025), not because it is not a valid major version.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Paschalis Tsilias, Michael Matloob.
Paschalis Tsilias uploaded patch set #4 to this change.
cmd/go: make mod init disallow invalid major version suffixes
This CL reuses the SplitPathVersion function from the module package to
detect invalid major version suffixes, and return a relevant error
message along with a suggested fix.
Fixes #44052
Change-Id: I6c06f31a134e864a1d9b6e00c048ca1c59b4365e
---
M src/cmd/go/internal/modload/init.go
A src/cmd/go/testdata/script/mod_init_invalid_major.txt
2 files changed, 127 insertions(+), 0 deletions(-)
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Michael Matloob.
5 comments:
Patchset:
The newest patchset now separates the suggestion with \n\t.
I've added a couple of comments for the other remarks; I think your input will be valuable, as I don't have really extensive experience with Go modules.
Thank you for your time!
File src/cmd/go/internal/modload/init.go:
Patch Set #3, Line 569:
; eg. g
If we're suggesting a `go mod init` command for the user to run, separate it with \n\t, like: […]
Done, it looks much better now!
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
I don't think these two cases should be allowed. […]
These two cases are indeed not allowed; I've only included them in the test script to make sure that we suggest that the user removes any leading zeroes.
Am I missing anything?
! go mod init example.com/user/repo/v2.0
stderr '^go: invalid module path "example.com/user/repo/v2.0": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v2.1.4
stderr '^go: invalid module path "example.com/user/repo/v2.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v3.5
stderr '^go: invalid module path "example.com/user/repo/v3.5": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v3$'
! go mod init example.com/user/repo/v4.1.4
stderr '^go: invalid module path "example.com/user/repo/v4.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v4$'
I wanted your input on this, and your example is indeed a good real-world scenario.
The spec might be a little contradictory here https://golang.org/ref/mod#go-mod-file-ident.
For a final path element of the form /vN where N looks numeric (ASCII digits and dots), N must not begin with a leading zero, must not be /v1, and must not contain any dots.
So it both says 'where N looks numeric (ASCII digits and dots)' _and_ 'must not contain any dots'.
Currently, the SplitPathVersion function rejects the module paths highlighted here, and also gives a similar failure example in its docstring.
Do you have any more information to clear this up?
Also, to be honest, while the module itself may be on a version with minor/patch (eg. v3.7.2), I haven't met a module path that includes them (eg. so I need to `go get github.com/user/pkg/v3.4`). I was under the impression that you'd only have a different module path if you bump your major, as stated on https://golang.org/doc/modules/version-numbers.
I understand that many projects don't respect semantic versioning, so this may be needed, but if that's the case, could the spec also need an amendment?
! go mod init example.com/user/repo/v2.
stderr '^go: invalid module path "example.com/user/repo/v2.": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
I think this path should be rejected on the grounds that it is not a valid import path (per #45025), […]
Thanks for catching this. Yes, this is a regression as Go 1.15.7 reports the correct error message, but the tip happily accepts it.
```
$ go version
go version go1.15.7 darwin/amd64
$ go mod init example.com/user/repo/v2.
go: malformed import path "example.com/user/repo/v2.": trailing dot in path element$ go version
go version devel go1.17-fd09593667 Thu Apr 29 20:46:14 2021 +0000 darwin/amd64
$ go mod init example.com/user/repo/v2.
go: creating new go.mod: module example.com/user/repo/v2.
```I'll dig around and find out if it's an easy fix, or if we if #45025 should be done first.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Michael Matloob.
Patch set 4:Trust +1
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
These two cases are indeed not allowed; I've only included them in the test script to make sure that […]
Sorry, I mistyped. I don't think these two cases should be *disallowed*, because there is no ambiguity here: IMO these paths are clearly not intended to be semantic major-versions.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Paschalis Tsilias, Michael Matloob.
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
Sorry, I mistyped. […]
IMO, 'go mod init' should forbid these. module.CheckPath rejects them because they're similar to major version suffixes but aren't valid. Better to let users know earlier.
(Basically, +1 to Paschalis' comment below)
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
IMO, 'go mod init' should forbid these. module. […]
If module.CheckPath already rejects them, then I'm fine with disallowing them — but I'm not entirely convinced that module.CheckPath ought to be rejecting them. 🤔
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
If module. […]
For consistency, this CL reuses `SplitPathVersion` from `module.CheckPath` and that's why both these and the ones in the other unresolved comment (like `example.com/user/repo/v4.1.4`) are failing.
I, too, think that we shouldn't allow initializing modules which then module.CheckPath would fail, but I wouldn't know which is the correct decision here.
My 2c, is that allowing `v02` is like allowing a loophole for people to not use a 'proper' v2, which might be confusing.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
For consistency, this CL reuses `SplitPathVersion` from `module. […]
Sorry, accidentally resolved this.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Attention is currently required from: Paschalis Tsilias, Jay Conrod, Michael Matloob.
Patch set 4:Code-Review +1
3 comments:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v02
stderr '^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v023
stderr '^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v23$'
Sorry, accidentally resolved this.
Ok, so it seems like things are at least consistent for now. Let's go with this and see if any issues turn up during the beta.
! go mod init example.com/user/repo/v2.0
stderr '^go: invalid module path "example.com/user/repo/v2.0": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v2.1.4
stderr '^go: invalid module path "example.com/user/repo/v2.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
! go mod init example.com/user/repo/v3.5
stderr '^go: invalid module path "example.com/user/repo/v3.5": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v3$'
! go mod init example.com/user/repo/v4.1.4
stderr '^go: invalid module path "example.com/user/repo/v4.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v4$'
I wanted your input on this, and your example is indeed a good real-world scenario. […]
From that text it seems that we are correct to reject these.
! go mod init example.com/user/repo/v2.
stderr '^go: invalid module path "example.com/user/repo/v2.": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
Thanks for catching this. Yes, this is a regression as Go 1.15. […]
Any luck figuring this one out?
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod, Michael Matloob.
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v2.
stderr '^go: invalid module path "example.com/user/repo/v2.": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
Any luck figuring this one out?
I found this behavior changing in CL 264621.
I think it's because the call to `module.CheckImportPath` that includes `checkElem` was replaced with a call to `checkModulePathLax`. As the major version checks implemented in the current CL are located in `checkModulePathLax`, it's currently failing again, but with this 'new' message.
I tried to change the call to `checkModulePathLax` back to `module.CheckImportPath` and it fails with the 'old' error message, but it also causes side effects. For example, `go mod init .` would fail with 'malformed import path' instead of 'invalid module path ".": is a local import path'.
There is a relevant-ish TODO comment under `checkModulePathLax`, that says that in the future we might switch back to calling `module.CheckImportPath` but that discussion has since been closed.
```
func checkModulePathLax(p string) error {
// TODO(matloob): Replace calls of this function in this CL with calls
// to module.CheckImportPath once it's been laxened, if it becomes laxened.
// See golang.org/issue/29101 for a discussion about whether to make CheckImportPath
// more lax or more strict.
```
Do you have any feedback on what's the best way to proceed?One option would be to replace this specific call to `checkModulePathLax` with `module.CheckImportPath`, and implement these new checks there, while fixing any other rising behaviors.
Another option would be to enhance `checkModulePathLax` with some new checks to emulate this error message, or to fix this issue first in #45025.
Please let me know what you think!
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Paschalis Tsilias, Michael Matloob.
Patch set 4:Trust +1
1 comment:
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
Also check a path with a space in the name.
If that works, please add "Fixes #46085" to the commit message.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Michael Matloob.
1 comment:
Patchset:
Since the latest changes in CL 320310 which deprecated `checkModulePathLax` in favor of `module.CheckImportPath` I'm not sure if the proposed changes here make sense, or if they should be applied on the external `module` package.
In any case, I'll have to take another look. Since this is one of the few tickets left for 1.17, maybe it should get bumped for 1.18 to trim down what's left for the milestone.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Paschalis Tsilias, Michael Matloob.
1 comment:
Patchset:
Since the latest changes in CL 320310 which deprecated `checkModulePathLax` in favor of `module. […]
I think this change still makes sense, but it will need to be updated.
'go mod init' is a bit strange in that it needs to be less strict than module.CheckModulePath but more strict than module.CheckImportPath: the module path definitely needs to be a valid import path, but paths like "example" or "test" are allowed for local modules, even though they aren't technically valid and won't work with 'go get'.
To move this forward, in modload.CreateModFile, after calling CheckImportPath, let's check module.SplitPathVersion and return those errors if it fails.
I'll bump the issue for 1.18.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Paschalis Tsilias, Michael Matloob.
Paschalis Tsilias uploaded patch set #5 to this change.
cmd/go: make mod init disallow invalid major version suffixes
This CL reuses the SplitPathVersion function from the module package to
detect invalid major version suffixes and return a relevant error
message along with a suggested fix.
Fixes #44052
Fixes #46085
Change-Id: I6c06f31a134e864a1d9b6e00c048ca1c59b4365e
---
M src/cmd/go/internal/modload/init.go
A src/cmd/go/testdata/script/mod_init_invalid_major.txt
2 files changed, 142 insertions(+), 0 deletions(-)
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Paschalis Tsilias (Gerrit)
Attention is currently required from: Bryan C. Mills, Jay Conrod, Michael Matloob.
4 comments:
Patchset:
I think this change still makes sense, but it will need to be updated. […]
Thanks for the guidance and for explaining the difference. I think that it fit nicely there after CheckModulePath, let me know how it looks!
Patchset:
Back with a new patchset after rebasing with the changes in CL 320310. Following Jay's advice, it worked in the same way without any fuss.
In the latest patchset
- Used `module.SplitPathVersion` in CreateModFile
- Added new test cases for paths with a trailing dot
- Added new test case for paths with spaces, so that we can also close #46085
Do you have any other edge cases in mind that we should check?
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
! go mod init example.com/user/repo/v2.
stderr '^go: invalid module path "example.com/user/repo/v2.": major version suffixes must be in the form of /vN and are only allowed for v2 or later; eg. go mod init example.com/user/repo/v2$'
I found this behavior changing in CL 264621. […]
This seems to be fixed somewhere around the CL 320310 revamp.
I've added a testcase to make sure that any such path is rejected as a 'malformed module path' instead.
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
Also check a path with a space in the name. […]
Done. Do you think we should change the name of the test script to something more generic now?
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Attention is currently required from: Bryan C. Mills, Paschalis Tsilias, Michael Matloob.
Patch set 5:Run-TryBot +1Code-Review +2Trust +1
2 comments:
Patchset:
TRY=linux-amd64-longtest
Looks good, thanks for working on this.
File src/cmd/go/testdata/script/mod_init_invalid_major.txt:
Done. […]
Test name seems fine to me.
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.
Jay Conrod (Gerrit)
Jay Conrod submitted this change.
cmd/go: make mod init disallow invalid major version suffixes
This CL reuses the SplitPathVersion function from the module package to
detect invalid major version suffixes and return a relevant error
message along with a suggested fix.
Fixes #44052
Fixes #46085
Change-Id: I6c06f31a134e864a1d9b6e00c048ca1c59b4365e
Reviewed-on: https://go-review.googlesource.com/c/go/+/288712
Reviewed-by: Jay Conrod <jayc...@google.com>
Trust: Jay Conrod <jayc...@google.com>
Trust: Bryan C. Mills <bcm...@google.com>
Run-TryBot: Jay Conrod <jayc...@google.com>
TryBot-Result: Go Bot <go...@golang.org>
---
M src/cmd/go/internal/modload/init.go
A src/cmd/go/testdata/script/mod_init_invalid_major.txt
2 files changed, 142 insertions(+), 0 deletions(-)
diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go
index d5f9d10..862b007 100644
--- a/src/cmd/go/internal/modload/init.go
+++ b/src/cmd/go/internal/modload/init.go
@@ -523,6 +523,13 @@
}
}
base.Fatalf("go: %v", err)
+ } else if _, _, ok := module.SplitPathVersion(modPath); !ok {
+ if strings.HasPrefix(modPath, "gopkg.in/") {
+ invalidMajorVersionMsg := fmt.Errorf("module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN:\n\tgo mod init %s", suggestGopkgIn(modPath))
+ base.Fatalf(`go: invalid module path "%v": %v`, modPath, invalidMajorVersionMsg)
+ }
+ invalidMajorVersionMsg := fmt.Errorf("major version suffixes must be in the form of /vN and are only allowed for v2 or later:\n\tgo mod init %s", suggestModulePath(modPath))
+ base.Fatalf(`go: invalid module path "%v": %v`, modPath, invalidMajorVersionMsg)
}
fmt.Fprintf(os.Stderr, "go: creating new go.mod: module %s\n", modPath)
@@ -1197,3 +1204,56 @@
func modkey(m module.Version) module.Version {
return module.Version{Path: m.Path, Version: m.Version + "/go.mod"}
}
+
+func suggestModulePath(path string) string {
+ var m string
+
+ i := len(path)
+ for i > 0 && ('0' <= path[i-1] && path[i-1] <= '9' || path[i-1] == '.') {
+ i--
+ }
+ url := path[:i]
+ url = strings.TrimSuffix(url, "/v")
+ url = strings.TrimSuffix(url, "/")
+
+ f := func(c rune) bool {
+ return c > '9' || c < '0'
+ }
+ s := strings.FieldsFunc(path[i:], f)
+ if len(s) > 0 {
+ m = s[0]
+ }
+ m = strings.TrimLeft(m, "0")
+ if m == "" || m == "1" {
+ return url + "/v2"
+ }
+
+ return url + "/v" + m
+}
+
+func suggestGopkgIn(path string) string {
+ var m string
+ i := len(path)
+ for i > 0 && (('0' <= path[i-1] && path[i-1] <= '9') || (path[i-1] == '.')) {
+ i--
+ }
+ url := path[:i]
+ url = strings.TrimSuffix(url, ".v")
+ url = strings.TrimSuffix(url, "/v")
+ url = strings.TrimSuffix(url, "/")
+
+ f := func(c rune) bool {
+ return c > '9' || c < '0'
+ }
+ s := strings.FieldsFunc(path, f)
+ if len(s) > 0 {
+ m = s[0]
+ }
+
+ m = strings.TrimLeft(m, "0")
+
+ if m == "" {
+ return url + ".v1"
+ }
+ return url + ".v" + m
+}
diff --git a/src/cmd/go/testdata/script/mod_init_invalid_major.txt b/src/cmd/go/testdata/script/mod_init_invalid_major.txt
new file mode 100644
index 0000000..ae93e70
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_init_invalid_major.txt
@@ -0,0 +1,82 @@
+env GO111MODULE=on
+env GOFLAGS=-mod=mod
+
+! go mod init example.com/user/repo/v0
+stderr '(?s)^go: invalid module path "example.com/user/repo/v0": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v2$'
+
+! go mod init example.com/user/repo/v02
+stderr '(?s)^go: invalid module path "example.com/user/repo/v02": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v2$'
+
+! go mod init example.com/user/repo/v023
+stderr '(?s)^go: invalid module path "example.com/user/repo/v023": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v23$'
+
+! go mod init example.com/user/repo/v1
+stderr '(?s)^go: invalid module path "example.com/user/repo/v1": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v2$'
+
+! go mod init example.com/user/repo/v2.0
+stderr '(?s)^go: invalid module path "example.com/user/repo/v2.0": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v2$'
+
+! go mod init example.com/user/repo/v2.1.4
+stderr '(?s)^go: invalid module path "example.com/user/repo/v2.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v2$'
+
+! go mod init example.com/user/repo/v3.5
+stderr '(?s)^go: invalid module path "example.com/user/repo/v3.5": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v3$'
+
+! go mod init example.com/user/repo/v4.1.4
+stderr '(?s)^go: invalid module path "example.com/user/repo/v4.1.4": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v4$'
+
+! go mod init example.com/user/repo/v.2.3
+stderr '(?s)^go: invalid module path "example.com/user/repo/v.2.3": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v2$'
+
+! go mod init example.com/user/repo/v.5.3
+stderr '(?s)^go: invalid module path "example.com/user/repo/v.5.3": major version suffixes must be in the form of /vN and are only allowed for v2 or later(.*)go mod init example.com/user/repo/v5$'
+
+! go mod init gopkg.in/pkg
+stderr '(?s)^go: invalid module path "gopkg.in/pkg": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg/v0
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg/v0": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg/v1
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg/v1": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg/v2
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg/v2": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v2$'
+
+! go mod init gopkg.in/user/pkg.v
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg.v": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg.v0.1
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg.v0.1": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg.v.1
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg.v.1": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg.v01
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg.v01": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v1$'
+
+! go mod init gopkg.in/user/pkg.v.2.3
+stderr '(?s)^go: invalid module path "gopkg.in/user/pkg.v.2.3": module paths beginning with gopkg.in/ must always have a major version suffix in the form of .vN(.*)go mod init gopkg.in/user/pkg.v2$'
+
+# module paths with a trailing dot are rejected as invalid import paths
+! go mod init example.com/user/repo/v2.
+stderr '(?s)^go: malformed module path "example.com/user/repo/v2.": trailing dot in path element$'
+
+! go mod init example.com/user/repo/v2..
+stderr '(?s)^go: malformed module path "example.com/user/repo/v2..": trailing dot in path element$'
+
+! go mod init gopkg.in/user/pkg.v.2.
+stderr '(?s)^go: malformed module path "gopkg.in/user/pkg.v.2.": trailing dot in path element$'
+
+! go mod init gopkg.in/user/pkg.v.2..
+stderr '(?s)^go: malformed module path "gopkg.in/user/pkg.v.2..": trailing dot in path element$'
+
+# module paths with spaces are also rejected
+! go mod init 'foo bar'
+stderr '(?s)^go: malformed module path "foo bar": invalid char '' ''$'
+
+! go mod init 'foo bar baz'
+stderr '(?s)^go: malformed module path "foo bar baz": invalid char '' ''$'
To view, visit change 288712. To unsubscribe, or for help writing mail filters, visit settings.