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

x/tools/imports: permission denied causes non-deterministic updating of imports #31285

Open
nlsun opened this issue Apr 5, 2019 · 1 comment

Comments

@nlsun
Copy link

commented Apr 5, 2019

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

$ go version
go version go1.11.4 linux/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
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ubuntu/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ubuntu/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build277105160=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Have a project with one directory with code with a missing import, this import is vendored.
  2. Have another directory that is owned by root (non-readable by the current user)
  3. Run imports.Process

What did you expect to see?

Consistently see the import added to the file

What did you see instead?

The import is sometime added and sometimes not added.

What I think is happening

The problem appears to be that addExternalCandidates sometimes sees the package in the vendor dir and sometimes doesn't.
https://github.com/golang/tools/blob/d996c1aa53b10c3c47e1764d087a569bb6c32ed1/imports/fix.go#L682
I think it's because it calls gopathResolver.scan to find a list of all packages
https://github.com/golang/tools/blob/d996c1aa53b10c3c47e1764d087a569bb6c32ed1/imports/fix.go#L910
and scan will quit upon the first error it encounters because it uses fastwalk.Walk https://github.com/golang/tools/blob/923d25813098b2ffdf1b4bb7ee4ec425fef796a9/internal/fastwalk/fastwalk.go#L195
Because it quits on the first error, if it reads the vendor dir with the wanted import first, then the import will be there, and if it hits the dir it can't read first, then it will exit without the import.

I think we should always walk the entire tree and walk around unreadable directories rather than aborting the entire walk at that point.

I can see 2 options:

  1. Change behavior to gathering errors and keep walking the entire tree
  2. Add configuration options to the walker that allows for multiple walk behaviors (abort on first error, or gather errors)

I'm happy to help with making this change as well.

Related Issue(s)

This issue appears similar to #16890 but it seems like that question is more targetted towards what log messages are printed, here I'm more concerned about the actual behavior (which dirs are scanned).

@gopherbot gopherbot added this to the Unreleased milestone Apr 5, 2019

@nlsun nlsun changed the title x/tools/imports: permission denied causes non-deterministic behavior x/tools/imports: permission denied causes non-deterministic updating of imports Apr 5, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Apr 16, 2019

Change https://golang.org/cl/170863 mentions this issue: cmd/gopherbot: CC triaged issues to owners

@gopherbot gopherbot added the Tools label Sep 12, 2019

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