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

Closed
bcmills opened this issue Dec 14, 2018 · 15 comments
Closed
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

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 NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. modules labels Dec 14, 2018
@bcmills bcmills added this to the Go1.12 milestone Dec 14, 2018
@thepudds
Copy link
Contributor

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
Copy link
Contributor

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
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
Copy link
Contributor 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
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
Copy link
Contributor 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
Copy link
Contributor 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
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 as completed Mar 15, 2019
@thepudds
Copy link
Contributor

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
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. label Apr 19, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 19, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@fannyhasbi
Copy link

Same with me. But it occurs when I create a project outside $GOPATH
I think it's because the project isn't in $GOPATH and make ambiguity.

With golang 1.12 or earlier I used

go mod init github.com/your-username/my-project

I hope it will help

@dylan-bourque
Copy link

I'm running into a similar issue.

I have 2 module-based projects, each hosted on our private GitLab instance. project2 depends on project1, but neither have any semver tags yet. In both projects, the only .go files in the root are tools.go (for tracking build-time dependencies that aren't imported directly) with a build tag of // +build tools and magefile.go (we're using Mage to build) with a build tag of // +build mage.

When I run GOPROXY= go get gitlab.com/ourcompany/project1@latest from the root folder of project2, I get an error like this:
package gitlab.com/ourcompany/project1: build constraints exclude all Go files in [$GOPATH]/pkg/mod/gitlab.com/ourcompany/project1@v0.0.1-0.20190826220557-dd3e4d630752

GOPROXY is unset b/c our private repos are not available on proxy.golang.org.

At the suggestion of several people in the Gophers Slack, I tried the variations below (all with GOPROXY unset):
go get -m gitlab.com/ourcompany/project1@latest -> no error message, but this retrieved the "wrong" content (not the most recent commit)
go get -m gitlab.com/ourcompany/project1@master -> this seems to have done the right thing
Both of the above with -d instead of -m -> generated the same "... build constraints exclude all Go files ..." message

$ go version
go version go1.12.9 linux/amd64

go env (slightly edited to redact private info):

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/snip/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/snip/dev/golang"
GOPROXY="https://goproxy.io"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/snip/project2/go.mod"
CGO_CFLAGS=""
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS=""
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build290359151=/tmp/go-build -gno-record-gcc-switches"

I agree with the earlier commenters that this should not be an error case, especially when the module contains multiple sub-packages that are being imported and used.

@thepudds
Copy link
Contributor

thepudds commented Feb 26, 2020

Here is what I think is a variation of this issue in Go 1.14:

$ go1.14 mod init tempmod
$ go1.14 get github.com/golang/mock
can't load package: package github.com/golang/mock: 
build constraints exclude all Go files in .../go/pkg/mod/github.com/golang/mock@v1.4.0

I think that is effectively the same as what @dylan-bourque reported above in #29268 (comment)

CC @broady

@hjkatz
Copy link

hjkatz commented May 8, 2020

So it appears that we have also been experiencing this issue, however, we've found that the problem stems from not having a proper main.go existing in the root of the module repo.

For example, if we add main.go with contents:

package main

// This file exists to define any Go code in the repo root
// so that it can be depended on as a library
func main() {}

Then our go get internal.repo.com/package works just fine.

@gopherbot
Copy link

Change https://golang.org/cl/289769 mentions this issue: cmd/go: suppress errors from 'go get -d' for packages that only conditionally exist

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

10 participants