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: suppress errors for 'go get' of package paths that contain only tests #29268

Open
bcmills opened this Issue Dec 14, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@bcmills
Copy link
Member

bcmills commented Dec 14, 2018

A Go module path is not necessarily a valid Go package path: for example, golang.org/x/tools is a valid module path, but since there are no .go files in the repo root it is not an importable package.

go get in GOPATH mode returns a non-zero exit code if that happens, but today in module mode we explicitly suppress that error:

if strings.Contains(p.Error.Err, "cannot find module providing") && modload.ModuleInfo(p.ImportPath) != nil {
// Explicitly-requested module, but it doesn't contain a package at the
// module root.
continue
}

On the other hand, if the user intends to pass a module path (rather than a package path), they can indicate that explicitly using the -m flag.

Should we change the error-suppressing path to instead produce a non-zero exit code? We could still make the error explicit about the fact that the requested path is a module but not a package.

(CC @rsc @hyangah @myitcv @thepudds @jayconrod )

@bcmills bcmills added this to the Go1.12 milestone Dec 14, 2018

@thepudds

This comment has been minimized.

Copy link

thepudds commented Dec 14, 2018

One comment on the current implementation: it seems a repo root that only has a *_test.go file (but no other *.go files) does trigger an error in 1.11.

k8s.io/api is an example repo that only has a test file roundtrip_test.go file in its repo root, which triggers an error if you do go get k8s.io/api@master:

GO111MODULE=on go get k8s.io/api@master
go: finding k8s.io/api master
go: downloading k8s.io/api v0.0.0-20181130031204-d04500c8c3dd
go build k8s.io/api: no non-test Go files in .../go/pkg/mod/k8s.io/api@v0.0.0-20181130031204-d04500c8c3dd

go get k8s.io/apimachinery@master on the other hand works (where that repo root has no *.go files).

CC @bhcleek

@jayconrod

This comment has been minimized.

Copy link
Contributor

jayconrod commented Dec 14, 2018

I don't have a strong opinion about this, but I lean toward reporting the error, especially if it's consistent with non-module behavior.

I assume go.mod would be updated as if -m had been passed either way though?

@bcmills bcmills modified the milestones: Go1.12, Go1.13 Dec 18, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 18, 2018

Module paths being the import path of the root directory of the module is just a fundamental design choice we've made (and are not going to unmake). That creates a little bit of ambiguity, which can easily lead to confusion. But some confusion will exist no matter what we choose here. The choice is what the confusion looks like.

In the world where go get module@version simply doesn't build code that isn't there (but still updates go.mod and therefore has a useful effect), users mostly ignore the distinction between module path and import path and they go on with their work. I think this is the best world; it's the one we're in now.

In the world where go get module@version succeeds when there is code in the root of the module but fails when there is not, we will get a steady stream of bugs complaining that some modules are compatible with 'go get without -m' while other modules are not, and people will be confused about why. Some people will learn to type 'go get -m' literally every time they run 'go get' (because downloads now happen automatically, the only time you really run 'go get' anymore is for manipulating modules). Other people will write blog posts explaining that best practice is to write dummy empty packages in the roots of your modules so that they work even when users forget to type -m. This seems to me a worse world. It has strictly more confusion for end users than the current world.

The k8s.io/api failure (only test go files in package) should be suppressed for all go get targets, not just the root of the module.

@bcmills

This comment has been minimized.

Copy link
Member Author

bcmills commented Dec 18, 2018

The particularly confusing behavior here for repos with major-version breaks that predate semantic import paths.

To draw an example from #27123: the module github.com/lxc/lxd at tag lxd-2.16 contains the package github.com/lxc/lxd, but that same module at the latest commit does not. So what happens if you already have a dependency on github.com/lxc/lxd at lxd-2.16 and you run go get github.com/lxc/lxd?

The options are unfortunate either way:

  • If we implicitly promote the package path to a module path, then go get on that path might actually remove the requested package.
  • On the other hand, if we ensure that existing packages do not disappear, then go get $MODULE will mean “update $MODULE to the latest version” except when that module happens to have removed the root package, in which case it will mean “update the package with the same path as $MODULE to the latest version”.

This may be a corner case either way, since packages should not disappear in general, but there certainly are plenty for existing repos that behave this way.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 18, 2018

I'm missing something: I don't understand what the github.com/lxc/lxd example has to do with requiring -m.

@bcmills

This comment has been minimized.

Copy link
Member Author

bcmills commented Dec 18, 2018

go get -m github.com/lxc/lxd would be unambiguous: it would mean “update the module github.com/lxc/lxd to the latest version”, regardless of what packages it contains.

The ambiguity occurs without -m because the user can't easily predict whether go get github.com/lxc/lxd will update the package or the module, and those have different semantics: one keeps the module pinned to an old version, while the other removes the package entirely.

@bcmills

This comment has been minimized.

Copy link
Member Author

bcmills commented Jan 14, 2019

Here is another neat example, from a conversation with @lopezator. The root of google.golang.org/genproto contains .go source files tagged with // +build ignore. Since the directory contains a Go source file, go get treats it as a package location; however, it cannot be built because the current configuration excludes that source file.

The message is not cannot find module providing, so go get decides to report an error.
(The behavior with go1.11.4 is even worse: it reports no install location for directory, even when GOBIN is set!)

$ go mod init golang.org/issue/scratch
go: creating new go.mod: module golang.org/issue/scratch

$ export GOBIN=$GOPATH/bin

$ go1.12beta2 env | egrep 'GOBIN|GOPATH|GOMOD'
GOBIN="/tmp/tmp.WuuV1dnHy0/_gopath/bin"
GOPATH="/tmp/tmp.WuuV1dnHy0/_gopath"
GOMOD="/tmp/tmp.WuuV1dnHy0/go.mod"

$ go1.12beta2 get google.golang.org/genproto@db91494
go: finding google.golang.org/genproto db91494
package google.golang.org/genproto: build constraints exclude all Go files in /tmp/tmp.WuuV1dnHy0/_gopath/pkg/mod/google.golang.org/genproto@v0.0.0-20190111180523-db91494dd46c

$ go1.11.4 get google.golang.org/genproto@db91494
go: finding google.golang.org/genproto db91494
go get: /tmp/tmp.WuuV1dnHy0/_gopath/pkg/mod/google.golang.org/genproto@v0.0.0-20190111180523-db91494dd46c outside GOPATH
        For more details see: 'go help gopath'
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Mar 14, 2019

My comment from Dec 18 #29268 (comment) still stands. I think we should not report the error and should close this bug.

@bcmills bcmills closed this Mar 15, 2019

@thepudds

This comment has been minimized.

Copy link

thepudds commented Mar 15, 2019

Is this comment from Russ from #29268 (comment) still pending:

The k8s.io/api failure (only test go files in package) should be suppressed for all go get targets, not just the root of the module.

... which I think references #29268 (comment)?

(Maybe it is addressed by another issue, or maybe there is no plan to address it, but just wanted to double check).

@thepudds

This comment has been minimized.

Copy link

thepudds commented Apr 5, 2019

@bcmills Re-opening for tracking purposes regarding question in immediately prior comment.

Feel free to close again.

@thepudds thepudds reopened this Apr 5, 2019

@bcmills bcmills changed the title cmd/go: should 'go get path@version' (without '-m') report an error if 'path' is only a module? cmd/go: suppress errors for 'go get' of package paths that contain only tests Apr 19, 2019

@bcmills bcmills added the NeedsFix label Apr 19, 2019

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