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: don't follow symlinks #45740

Open
adam-azarchs opened this issue Apr 23, 2021 · 3 comments
Open

x/tools/cmd/goimports: don't follow symlinks #45740

adam-azarchs opened this issue Apr 23, 2021 · 3 comments

Comments

@adam-azarchs
Copy link
Contributor

@adam-azarchs adam-azarchs commented Apr 23, 2021

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

$ go version
1.16.3

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="/.../go-build"
GOENV="/.../.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="..."
GONOPROXY="..."
GONOSUMDB="..."
GOOS="linux"
GOPATH=".../gopath"
GOPRIVATE="..."
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/.../tools/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/.../go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="/.../bin/llvm-ar"
CC="/.../bin/clang"
CXX="/.../bin/clang++"
CGO_ENABLED="1"
GOMOD="/.../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=/tmp/go-build2861605680=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Used goimports on a file after having built with bazel.

What did you expect to see?

An import to abcxyz/package/name added to my file, relatively quickly.

What did you see instead?

An import to abcxyz/bazel-abcxyz/package/name added to my file, after a long time spent searching.

This is somewhat similar to the issue with e.g. node_modules (see for example #16417 or #30058) but the resolution seems a bit more clear in this case, at least to me: goimports should ignore symlinks the same way that go mod and go build do.

@gopherbot gopherbot added this to the Unreleased milestone Apr 23, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 26, 2021

@heschi
Copy link
Contributor

@heschi heschi commented Apr 26, 2021

In my experience, someone in the world is using symlinks for every possible purpose. Ignoring them may or may not make sense, but doing it will probably annoy someone. And it won't actually make bazel work, since goimports won't be able to find generated code.

Unfortunately, without a way to predict the impact of the change, my inclination here is toward inaction. Sorry.

The bazel user community might be big enough to consider a goimports fork that understands bazel.

@adam-azarchs
Copy link
Contributor Author

@adam-azarchs adam-azarchs commented Apr 26, 2021

If you're expecting go to find packages in symlinked locations, you're already going to be running into problems because go mod tidy ignores symlinks and will probably remove a bunch of your dependencies. It'll also cause a problem for you if you don't have your generated code in the source tree, and the generated code has dependencies which go mod tidy then removes. I don't think it makes sense for goimports to consider paths which go mod tidy and go build do not - if you're depending on symlinks that way, your workflow is already broken (at least in module mode).

The situation I'm concerned with is the fairly common situation where a monorepo is built with bazel but developers working primarily on go parts of the repo still usually build with go build during iterative development, and want tools such as editor integrations which run goimports on save to still work correctly, in which case the generated code will still usually be checked in (or at least present in the source tree but maybe .gitignored).

And it won't actually make bazel work, since goimports won't be able to find generated code.

It's common for packages containing generated code to also contain code that isn't generated (at the very least, a source file with a //go:generate line). Thus, goimports would still be able to find the package. Plus there's plenty of projects that don't use generated code at all but would still like for goimports to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants