Open jeffw-wherethebitsroam opened 8 years ago
Hey guys, happy to help but what should the behaviour be:
I would lean towards 2.
Jeff
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.
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.
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.
@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 https://github.com/golang/go/issues/16890#issuecomment-242926004 ... we can't be interactive or chatty by default, as we're likely running underneath an editor process.
@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.
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
chmod 0000 gotest/src/github.com/wherethebitsroam/cloudcp/test
cd gotest/src/github.com/wherethebitsroam/test
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
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.
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.
@jeffw-wherethebitsroam thank you for being specific. I'll take in count your observation for my next tests.
@luigi-riefolo are you still planning on fixing it ? if not I may try to pick it up Cheers
@asticode you can pick it up
CL https://golang.org/cl/30751 mentions this issue.
@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!
Please answer these questions before submitting your issue. Thanks!
go version
)?go env
)?One unreadable dir in GOPATH will cause the walk to fail and ignore all directories after that point, with a confusing message like:
In this case, it was a test dir with permissions 0000:
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:
goimports to skip the directory with permission denied. Or at least give an error with the directory which was the problem.
goimports failed to insert the imports for known packages in the file being processed.