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/cmd/goimports: permission denied causes imports to stop #16890

Open
jeffw-wherethebitsroam opened this issue Aug 26, 2016 · 14 comments

Comments

@jeffw-wherethebitsroam
Copy link

commented Aug 26, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
go version go1.7 darwin/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jeffwilliams/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/jc/ctl0fvhs34v21m_hg1z3w0zh0000gp/T/go-build379181819=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
  1. What did you do?
    If possible, provide a recipe for reproducing the error.
    A complete runnable program is good.
    A link on play.golang.org is best.

One unreadable dir in GOPATH will cause the walk to fail and ignore all directories after that point, with a confusing message like:

2016/08/26 15:39:24 goimports: scanning directory /Users/jeffwilliams/go/src: permission denied

In this case, it was a test dir with permissions 0000:

/Users/jeffwilliams/go/src/github.com/wherethebitsroam/cloudcp/test/fred/dave

Admittedly, moving this test content into testdata where it should have been fixed the problem. But this is not very obvious given the error.

The relevant code was golang.org/x/tools/imports/fastwalk_unix.go:

fd, err := syscall.Open(dirName, 0, 0)
  1. What did you expect to see?

goimports to skip the directory with permission denied. Or at least give an error with the directory which was the problem.

  1. What did you see instead?

goimports failed to insert the imports for known packages in the file being processed.

@quentinmit quentinmit added this to the Unreleased milestone Aug 26, 2016

@jeffw-wherethebitsroam

This comment has been minimized.

Copy link
Author

commented Aug 27, 2016

Hey guys, happy to help but what should the behaviour be:

  1. Skip the directory silently
  2. Skip the directory and give a error message
  3. Stop processing and have error message give the directory with the error

I would lean towards 2.

Jeff

@josharian

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

1 or 2 seems better. goimports is frequently run from editor hooks, so one important question is how they handle output on stderr. Will be it be presented to the user? If not what's the point, but if so, will it be annoying? Will output on stderr be interpreted as failure and prevent formatting? Given all that I personally lean towards 1.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

goimports has a verbose (-v) flag as of recently. Let's be skip & silent by default, but in verbose mode let's show a warning (but still skip it). Because in verbose mode, presumably a human is running it, instead of an editor, and they're more likely debugging and seeing stderr.

@luigi-riefolo

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2016

Hi guys!
I'm a noob but totally available to fix this issue. Could you please let me know whether I need to mark myself as the assignee (and how), so that we can avoid duplicate work? Or do you follow a different path?
For the fix: ideally we could use all three strategies proposed by Jeff. Using option 3 (strict mode) during the import process would let the user decide if the issue is a severe one or can be ignored.
For option 1 (skip & silent) I'd rather give a warning message instead. That would be useful while investigating a build-automation log. Hiding such an error could be very painful to debug.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 3, 2016

@luigi-riefolo, no need to use Assignee. Just send a change and drop a comment here with the URL to the Gerrit review.

Please use the design I proposed in #16890 (comment) ... we can't be interactive or chatty by default, as we're likely running underneath an editor process.

@luigi-riefolo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2016

@jeffw-wherethebitsroam I've tried to reproduce the issue using the same environment but with no results.
I've chmod the dir wherethebitsroam/cloudcp/test/fred/dave to 000 and the error message for both
go build test.go and goimports -v -e test.go is:

open /Users/luigi/Workspace/Go/src/github.com/wherethebitsroam/cloudcp/test/fred/dave: permission denied

As you can see it specifies the 000 dir.
So could you please be a bit more specific on the issue?

Thank you.

@jeffw-wherethebitsroam

This comment has been minimized.

Copy link
Author

commented Sep 5, 2016

Hi Luigi,

Are you running the goimports from the cloudcp directory? If so, can you try from another directory?

Otherwise, I have attached a little example.
gotest.tar.gz

  1. extract the tar.gz
  2. set your GOPATH to the new directory
  3. chmod 0000 gotest/src/github.com/wherethebitsroam/cloudcp/test
  4. cd gotest/src/github.com/wherethebitsroam/test
  5. goimports -v -e main.go

Which should show an error like:

2016/09/05 09:53:00.769508 goimports: scanning directory /Users/jeffwilliams/gotest/src: permission denied

Jeff

@luigi-riefolo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2016

Thank you @jeffw-wherethebitsroam for the accurate example.
I've run each step and did get the import for "fmt":

GOPATH=$HOME/Downloads/TEST/gotest goimports main.go

2016/09/05 15:56:26 goimports: scanning directory /Users/luigi/Downloads/TEST/gotest/src: permission denied
package main

import "fmt"

func main() {
    f := cloudcp.DoStuff()
    fmt.Printf(f)
}

So apparently we do get the imports for anything after the 000 dir. But please correct if I'm wrong.

@jeffw-wherethebitsroam

This comment has been minimized.

Copy link
Author

commented Sep 7, 2016

Probably not a great example, it was just to show you the misleading error line.

The fmt package is in GOROOT. This bug will only affect packages in GOPATH which would have been walked after the permissions error occurs.

@luigi-riefolo

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2016

@jeffw-wherethebitsroam thank you for being specific. I'll take in count your observation for my next tests.

@asticode

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2016

@luigi-riefolo are you still planning on fixing it ? if not I may try to pick it up
Cheers

@luigi-riefolo

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2016

@asticode you can pick it up

@gopherbot

This comment has been minimized.

Copy link

commented Oct 8, 2016

CL https://golang.org/cl/30751 mentions this issue.

@ysmolsky

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

@bradfitz I have tried to fix it according your ideas here and in that last CL, but I fail to see good solution without refactoring many things and adding ugly hook into fastwalk.

What if we just improve error report printed? It will help users to see the problematic dir:

goimports: scanning directory /Users/t/go/src: permission denied while reading dir: /Users/t/go/src/imp/cloudcp/test

Let me know if I need to create separate issue for that. Thank you!

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