[vgo] cmd/go: diagnose misuse of path@version syntax
Russ Cox (Gerrit)
Russ Cox would like Bryan C. Mills to review this change.
cmd/go: diagnose misuse of path@version syntax
We should only see path@version arguments
in module mode, and then only for 'go get'.
Change-Id: I223a924b42bbd1710713c2202e5b4403fef7e18d
---
M vendor/cmd/go/internal/get/get.go
M vendor/cmd/go/internal/load/pkg.go
M vendor/cmd/go/internal/modload/load.go
M vendor/cmd/go/mod_test.go
4 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/vendor/cmd/go/internal/get/get.go b/vendor/cmd/go/internal/get/get.go
index 76ba238..36aa171 100644
--- a/vendor/cmd/go/internal/get/get.go
+++ b/vendor/cmd/go/internal/get/get.go
@@ -199,6 +199,12 @@
// in the hope that we can figure out the repository from the
// initial ...-free prefix.
func downloadPaths(args []string) []string {
+ for _, arg := range args {
+ if strings.Contains(arg, "@") {
+ base.Fatalf("go: cannot use path@version syntax in GOPATH mode")
+ }
+ }
+
args = load.ImportPathsForGoGet(args)
var out []string
for _, a := range args {
diff --git a/vendor/cmd/go/internal/load/pkg.go b/vendor/cmd/go/internal/load/pkg.go
index 0e5e758..edc29ad 100644
--- a/vendor/cmd/go/internal/load/pkg.go
+++ b/vendor/cmd/go/internal/load/pkg.go
@@ -442,6 +442,24 @@
}
}
+ if strings.Contains(path, "@") {
+ var text string
+ if cfg.ModulesEnabled {
+ text = "can only use path@version syntax with 'go get'"
+ } else {
+ text = "cannot use path@version syntax in GOPATH mode"
+ }
+ return &Package{
+ PackagePublic: PackagePublic{
+ ImportPath: path,
+ Error: &PackageError{
+ ImportStack: stk.Copy(),
+ Err: text,
+ },
+ },
+ }
+ }
+
// Determine canonical identifier for this package.
// For a local import the identifier is the pseudo-import path
// we create from the full directory to the package.
diff --git a/vendor/cmd/go/internal/modload/load.go b/vendor/cmd/go/internal/modload/load.go
index e8f5b79..51f06d6 100644
--- a/vendor/cmd/go/internal/modload/load.go
+++ b/vendor/cmd/go/internal/modload/load.go
@@ -528,6 +528,11 @@
func (ld *loader) findDir(pkg *loadPkg) {
path := pkg.path
+ if strings.Contains(path, "@") {
+ // Leave for error during load.
+ return
+ }
+
// Is the package in the standard library?
if search.IsStandardImportPath(path) {
if strings.HasPrefix(path, "golang_org/") {
diff --git a/vendor/cmd/go/mod_test.go b/vendor/cmd/go/mod_test.go
index cb3379c..bdfe9fc 100644
--- a/vendor/cmd/go/mod_test.go
+++ b/vendor/cmd/go/mod_test.go
@@ -93,6 +93,25 @@
tg.grepStdout(`"GOMOD": ""`, "expected module mode disabled")
}
+func TestModVersionsInGOPATHMode(t *testing.T) {
+ tg := testgo(t)
+ tg.setenv("GO111MODULE", "off") // GOPATH mode
+ defer tg.cleanup()
+ tg.makeTempdir()
+
+ tg.runFail("get", "rsc.io/qu...@v1.5.1")
+ tg.grepStderr(`go: cannot use path@version syntax in GOPATH mode`, "expected path@version error")
+
+ tg.runFail("build", "rsc.io/qu...@v1.5.1")
+ tg.grepStderr(`can't load package:.* cannot use path@version syntax in GOPATH mode`, "expected path@version error")
+
+ tg.setenv("GO111MODULE", "on") // GOPATH mode
+ tg.tempFile("x/go.mod", "module x")
+ tg.cd(tg.path("x"))
+ tg.runFail("build", "rsc.io/qu...@v1.5.1")
+ tg.grepStderr(`can't load package:.* can only use path@version syntax with 'go get'`, "expected path@version error")
+}
+
func TestModFindModuleRoot(t *testing.T) {
tg := testgo(t)
tg.setenv("GO111MODULE", "on")
To view, visit change 122405. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=bde7299c
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 1:TryBot-Result +1
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 2:TryBot-Result +1
Russ Cox (Gerrit)
Russ Cox uploaded patch set #3 to this change.
cmd/go: diagnose misuse of path@version syntax
We should only see path@version arguments
in module mode, and then only for 'go get'.
Change-Id: I223a924b42bbd1710713c2202e5b4403fef7e18d
---
M vendor/cmd/go/go_test.go
M vendor/cmd/go/internal/get/get.go
M vendor/cmd/go/internal/load/pkg.go
M vendor/cmd/go/internal/modload/load.go
M vendor/cmd/go/mod_test.go
5 files changed, 50 insertions(+), 2 deletions(-)
To view, visit change 122405. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 3:TryBot-Result +1
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 4:TryBot-Result +1
Bryan C. Mills (Gerrit)
1 comment:
File vendor/cmd/go/internal/load/pkg.go:
Patch Set #4, Line 445:
if strings.Contains(path, "@") {This diagnostic makes sense in general, but I think we need to rule out a successful load before we can emit it.
The spec says: “A compiler may restrict ImportPaths to non-empty strings using only characters belonging to Unicode's L, M, N, P, and S general categories (the Graphic characters without spaces) and may also exclude the characters !"#$%&'()*,:;<=>?[\]^`{|} and the Unicode replacement character U+FFFD.”
The @ sign is in category P and not explicitly excluded, so I don't think we can rule it out a priori.
For comparison, I tried the following example with the existing go tool and it built and ran successfully:/tmp/tmp.QjqRn8qwMH/src$ find . -type f
./bcm...@google.com/he...@v1.0.0/main/main.go
./bcm...@google.com/he...@v1.0.0/hello.go/tmp/tmp.QjqRn8qwMH/src$ cat $(find . -type f)
package mainimport (
hello "bcm...@google.com/he...@v1.0.0"
)func main() {
hello.Hello()
}
package helloimport "fmt"
func Hello() {
fmt.Println("Hello, @!")
}/tmp/tmp.QjqRn8qwMH/src$ go build bcm...@google.com/he...@v1.0.0/main
/tmp/tmp.QjqRn8qwMH/src$ ./main
Hello, @!
To view, visit change 122405. To unsubscribe, or for help writing mail filters, visit settings.
Russ Cox (Gerrit)
1 comment:
Patch Set #4, Line 445:
if strings.Contains(path, "@") {
This diagnostic makes sense in general, but I think we need to rule out a successful load before we […]
The go command is not really bound by the spec here. It is in charge of defining how import paths are interpreted, and it's well within its rights to say that import paths with @ signs have no interpretation (or rather they have an unavailable interpretation). The go command is simply asserting that import paths must not have @s in them. This is completely legitimate and a good way to eliminate annoying command-line ambiguity.
We've never allowed @ in the paths that go get will handle, and there's no point in starting now. It's true that this code will break uses where people carefully arrange GOPATH some way other than 'go get' and happen to put @ signs in their directory names. I'm OK with telling those people to find a way to get rid of the @ signs. In the last release we removed support for directories beginning with @ signs, because of a different command-line ambiguity. Now they're just gone entirely. I really don't think it will affect anyone, and it dramatically simplifies the code.
To view, visit change 122405. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
1 comment:
Patch Set #4, Line 445:
if strings.Contains(path, "@") {
We've never allowed @ in the paths that go get will handle, and there's no point in starting now.
It's fine to not support that syntax in `go get`, but this change is specifically for things that are *not* `go get`, like `go build` and `go test`, and (intentionally) affects previously-working commands and configurations that are not part of the versioning experiment.
If this change were to only take effect when `cfg.ModulesEnabled` is true, then I think it would be fine: in practice the accidental invocations are almost certainly going to be in module mode, and that gives anybody with funky @-paths in their GOPATH the option to remain in GOPATH mode for 1.11.
To view, visit change 122405. To unsubscribe, or for help writing mail filters, visit settings.
Bryan C. Mills (Gerrit)
Per offline discussion, I'm ok with this change with two caveats:
1. It needs a Go 1.11 release note.
2. If it breaks anybody during beta2 or the release candidate, we should at least scale it back to only apply in module mode. (https://xkcd.com/1172/)
Patch set 4:Code-Review +2
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 5:TryBot-Result +1
Russ Cox (Gerrit)
Russ Cox merged this change.
cmd/go: diagnose misuse of path@version syntax
We should only see path@version arguments
in module mode, and then only for 'go get'.
Change-Id: I223a924b42bbd1710713c2202e5b4403fef7e18d
Reviewed-on: https://go-review.googlesource.com/122405
Reviewed-by: Bryan C. Mills <bcm...@google.com>
---
M vendor/cmd/go/go_test.go
M vendor/cmd/go/internal/get/get.go
M vendor/cmd/go/internal/load/pkg.go
M vendor/cmd/go/internal/modload/load.go
M vendor/cmd/go/mod_test.go
5 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/vendor/cmd/go/go_test.go b/vendor/cmd/go/go_test.go
index 555b457..d549b94 100644
--- a/vendor/cmd/go/go_test.go
+++ b/vendor/cmd/go/go_test.go
@@ -6232,12 +6232,12 @@
tg.tempFile("src/@x/x.go", "package x\n")
tg.setenv("GOPATH", tg.path("."))
tg.runFail("build", "@x")
- tg.grepStderr("invalid input directory name \"@x\"", "did not reject @x directory")
+ tg.grepStderr("invalid input directory name \"@x\"|cannot use path@version syntax", "did not reject @x directory")
tg.tempFile("src/@x/y/y.go", "package y\n")
tg.setenv("GOPATH", tg.path("."))
tg.runFail("build", "@x/y")
- tg.grepStderr("invalid import path \"@x/y\"", "did not reject @x/y import path")
+ tg.grepStderr("invalid import path \"@x/y\"|cannot use path@version syntax", "did not reject @x/y import path")
tg.tempFile("src/-x/x.go", "package x\n")
tg.setenv("GOPATH", tg.path("."))
index fe3f64d..703d576 100644
--- a/vendor/cmd/go/internal/modload/load.go
+++ b/vendor/cmd/go/internal/modload/load.go
@@ -533,6 +533,11 @@
// It is also possible to return successfully with a zero module.Version,
// for packages in the standard library or when using vendored code.
func (ld *loader) findDir(path string) (dir string, mod module.Version, err error) {
+ if strings.Contains(path, "@") {
+ // Leave for error during load.
+ return
+ }
+
// Is the package in the standard library?
if search.IsStandardImportPath(path) {
if path == "C" || path == "unsafe" {
diff --git a/vendor/cmd/go/mod_test.go b/vendor/cmd/go/mod_test.go
index e723d4e..258066f 100644
--- a/vendor/cmd/go/mod_test.go
+++ b/vendor/cmd/go/mod_test.go
@@ -92,6 +92,25 @@