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

go/build: importGo invokes 'go list' with the wrong working directory in module mode #34860

Closed
CorentinDeBoisset opened this issue Oct 12, 2019 · 11 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@CorentinDeBoisset
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.13 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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/corentin/Library/Caches/go-build"
GOENV="/Users/corentin/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/corentin/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/corentin/code/my_recipes/backend/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sz/z0q535dx2876nknnf0dfk2fh0000gn/T/go-build204703159=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have the following (simple) project, with a main.go:

package main

import (
	"fmt"
	"github.com/spf13/viper"
)
func init() {
        // this is to avoid the "imported but not used" error
	viper.SetConfigFile("./main")
}
func main() {
	fmt.Println("hello")
}

And the following go.mod:

module testissue
go 1.13
require github.com/spf13/viper v1.4.0

I can build and run the binary, but if y run gotype -t -e main.go I get the following error :

main.go:5:2: could not import github.com/spf13/viper (type-checking package "github.com/spf13/viper" failed (/Users/corentin/go/pkg/mod/github.com/spf13/viper@v1.4.0/flags.go:3:8: could not import github.com/spf13/pflag (go/build: importGo github.com/spf13/pflag: exit status 1
error writing go.mod: open /Users/corentin/go/pkg/mod/github.com/spf13/viper@v1.4.0/go.mod298498081.tmp: permission denied

)))
zsh: exit 2     gotype -t -e main.go

This is only the case with viper, if i use another package, such as cobra by the same team, there is no error.

Even wierder, if I run gotype from the gotype extention of Sublime-Linter, it manages to lint correctly every time with no errors.

What did you expect to see?

No error when running gotype, and no mention of writing a go.mod in an external package.

What did you see instead?

The error mentioned above.

@CorentinDeBoisset
Copy link
Author

I also opened an issue in the viper repository because I don't know if the error is caused by gotype or viper.

@agnivade agnivade changed the title Gotype crashes in terminal with package viper x/tools/cmd/gotype: crashes in terminal with package viper Oct 12, 2019
@gopherbot gopherbot added this to the Unreleased milestone Oct 12, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 12, 2019
@katiehockman
Copy link
Contributor

Not sure what's going on here. /cc @bcmills in case this is modules related, and /cc @griesemer in case this is related to gotypes.

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 14, 2019
@griesemer griesemer changed the title x/tools/cmd/gotype: crashes in terminal with package viper x/tools/cmd/gotype: error reported with package viper Oct 14, 2019
@griesemer
Copy link
Contributor

This isn't a crash, it's a properly returned error (adjusted title).

I can reproduce this, and the underlying error seems to be:

go/build: importGo github.com/spf13/pflag: exit status 1
error writing go.mod: open /Users/gri/pkg/mod/github.com/spf13/viper@v1.4.0/go.mod298498081.tmp: permission denied

which points at go/build. @bcmills ?

@bcmills
Copy link
Contributor

bcmills commented Oct 14, 2019

This is directly related to a suspected existing bug in go/build:

go/src/go/build/build.go

Lines 1057 to 1060 in b649bdc

// TODO(bcmills): This is wrong if srcDir is in a vendor directory, or if
// srcDir is in some module dependency of the main module. The main module
// chooses what the import paths mean: individual packages don't.
cmd.Dir = srcDir

It's nice to have a concrete reproducer, though. 😅

CC @jayconrod, who has been looking at some other go/build issues in 1.14.

@bcmills bcmills changed the title x/tools/cmd/gotype: error reported with package viper go/build: importGo invokes 'go list' with the wrong working directory in module mode Oct 14, 2019
@bcmills bcmills modified the milestones: Unreleased, Backlog Oct 14, 2019
@griesemer
Copy link
Contributor

#33541 seems to be related.

I'm going to bump this to Go1.14. Would be nice to get this fixed.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/203857 mentions this issue: internal/imports: set ctx.WorkindDir if such a field exists

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. and removed help wanted labels Oct 28, 2019
@bcmills bcmills self-assigned this Oct 28, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 28, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/203820 mentions this issue: go/build: use the main module's root when locating module sources

gopherbot pushed a commit to golang/tools that referenced this issue Oct 29, 2019
CL 203820 removes an assumption in go/build that srcDir is in the main
module, since in general it need not be. That requires the use of some
other mechanism for callers to communicate the correct location of the
main module.

Fortunately, we already have a WorkingDir field on the ProcessEnv
struct that does exactly that. We can simply propagate it through if
the corresponding field is present on go/build.Context.

Updates golang/go#34860

Change-Id: Idabf9ae06d8383a72772d5a589fae1d10f206c01
Reviewed-on: https://go-review.googlesource.com/c/tools/+/203857
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre stamblerre removed the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 7, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/216817 mentions this issue: go/build: update TestImportDirNotExist to accept more detailed error strings

gopherbot pushed a commit that referenced this issue Jan 29, 2020
…strings

In CL 203820, we switched go/build to use the caller's working
directory for the main module (rather than srcDir), so that go/build
resolution now respects the requirements and replacements of the main
module. When the passed-in srcDir is empty, as of that CL we use "go
list" instead of falling back to in-process (GOPATH-mode) path lookup.

Unfortunately, that broke go/build.TestImportDirNotExist when
GO111MODULE=on: the test was looking for the specific error message
produced by the in-process lookup.

This change relaxes the test to accept the error message produced by
"go list" when srcDir is empty.

Updates #34769
Updates #34860
Updates #35734
Fixes #36867

Change-Id: Id0f7814a4b7dabe8917216eb013bb4eaee283648
Reviewed-on: https://go-review.googlesource.com/c/go/+/216817
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/217124 mentions this issue: doc/go1.14: mention new field go/build.Context.Dir

gopherbot pushed a commit that referenced this issue Jan 31, 2020
Updates #34860
Updates #36168
Updates #36878

Change-Id: I484d7fea5d77d6dcd451d4fdffe0c450eed73636
Reviewed-on: https://go-review.googlesource.com/c/go/+/217124
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/218817 mentions this issue: go/build: populate partial package information in importGo

gopherbot pushed a commit that referenced this issue Feb 12, 2020
This is a followup to CL 199840 and CL 203820. Cumulatively, they caused
a previously known bug to trigger more often while also nearly fixing it.

This change is a small fixup to CL 199840 that resolves the known bug
and prevents it from causing an additional regression in Go 1.14.

Part 1

The intention in CL 199840 was to return the same error that 'go list'
reported when the package wasn't located, so an early return was added.
However, to determine whether the package was located or not, p.Dir was
unintentionally checked instead of dir.

p is initialized to &Package{ImportPath: path} at top of Context.Import,
and its Dir field is never set before that line in importGo is reached.
So return errors.New(errStr) was always executed whenever errStr != "".

Originally, in CL 125296, the "go list" invocation did not include an
'-e' flag, so it would return a non-zero exit code on packages where
build constraints exclude all Go files, and importGo would return an
error like "go/build: importGo import/path: unexpected output: ...".

CL 199840 added an '-e' flag to the "go list" invocation, but checking
the wrong dir variable caused partial package information to never get
populated, and thus issue #31603 continued to occur, although with a
different error message (which ironically included the location of the
package that was supposedly "not found").

Now that the right dir is checked, issue #31603 is fixed.

Part 2

importGo checks whether it can use the go command to find the directory
of a package. In Go 1.13.x and earlier, one of the conditions to use the
go command was that the source directory must be provided.

CL 203820 made a change such that knowing the source directory was
no longer required:

	 // To invoke the go command,
	-// we must know the source directory,
	 // ...

That meant build.Import invocations where srcDir is the empty string:

	build.Import(path, "", build.FindOnly)

Started using the go command to find the directory of the package, and
started to run into issue #31603 as well. That's the #37153 regression.

Since this change fixes issue #31603, it also fixes issue #37153.

Part 3

There is one more thing. Delete the debugImportGo constant, it's unused.

Updates #26504 (CL 125296)
Updates #34752 (CL 199840)
Updates #34860 (CL 203820)
Fixes #31603
Fixes #37153

Change-Id: Iaa7dcc45ba0f708a978950c75fa4c836b87006f4
Reviewed-on: https://go-review.googlesource.com/c/go/+/218817
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/255397 mentions this issue: internal/imports: don't set Context.WorkingDir, which was renamed

gopherbot pushed a commit to golang/tools that referenced this issue Sep 17, 2020
This field was renamed to Dir during the 1.14 development cycle. We
don't need to look for it anymore.

For golang/go#34860

Change-Id: I313a931f070e3dc5a711233e7523c309995fe655
Reviewed-on: https://go-review.googlesource.com/c/tools/+/255397
Run-TryBot: Jay Conrod <jayconrod@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Sep 16, 2021
@rsc rsc unassigned bcmills Jun 23, 2022
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

6 participants