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

cmd/internal/moddeps: TestAllDependencies fails when GOROOT involves symlinks #46269

Open
aarzilli opened this issue May 19, 2021 · 16 comments
Open
Labels
NeedsFix
Milestone

Comments

@aarzilli
Copy link
Contributor

@aarzilli aarzilli commented May 19, 2021

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

$ go version
go version devel go1.17-6c1c055d1e Wed May 19 15:20:08 2021 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/a/.cache/go-build"
GOENV="/home/a/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/a/n/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/a/n/go/"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR="/dev/shm/"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-6c1c055d1e Wed May 19 15:20:08 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/usr/local/go/src/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 -fmessage-length=0 -fdebug-prefix-map=/dev/shm/go-build777046897=/tmp/go-build -gno-record-gcc-switches"

What did you do?

./all.bash

What did you expect to see?

PASS

What did you see instead?

--- FAIL: TestAllDependencies (0.00s)
    moddeps_test.go:49: findGorootModules didn't find the well-known module "std"
--- FAIL: TestDependencyVersionsConsistent (0.00s)
    moddeps_test.go:321: findGorootModules didn't find the well-known module "std"
FAIL
FAIL	cmd/internal/moddeps	0.008s

It seems that this was introduced by https://go-review.googlesource.com/c/go/+/320991 6c1c055

cc @dmitshur

@dmitshur dmitshur added the NeedsInvestigation label May 19, 2021
@dmitshur dmitshur self-assigned this May 19, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 19, 2021

Thanks for reporting. That seems unexpected. This did not happen when I ran the test locally (darwin/amd64) nor on builders.

Is there any information you can add about how you checked out the GOROOT directory to help reproduce this?

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented May 19, 2021

The output of go env GOROOT is actually a symlink on my system. This is the only thing I can think of that could affect this.

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 19, 2021

Are you able to try with commit dade83a, and its parent, and see if it reproduces with those?

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 19, 2021

Can you describe your symlink setup in more detail? That would be very helpful in reproducing this and understanding the issue.

I expect either the test can be improved to handle this situation, if that's feasible, or otherwise detect it and skip running.

@dmitshur dmitshur changed the title cmd/internal/moddeps: TestAllDependencies fails "findGorootModules didn't find the well-known module "std" cmd/internal/moddeps: TestAllDependencies fails when GOROOT involves symlinks May 19, 2021
@dmitshur dmitshur added the WaitingForInfo label May 19, 2021
@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented May 19, 2021

Both dade83a and its parent fef5a15 pass all the tests.

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented May 19, 2021

I have a symlink called /usr/local/go that points to the directory /usr/local/go-tip which is a normal git checkout of the go repository. The env variable PATH contains /usr/local/go/bin. The output of go env GOROOT is /usr/local/go the GOROOT environment variable is not set.

@dmitshur dmitshur removed the WaitingForInfo label May 19, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 19, 2021

Thanks, that's very helpful. I'm able to reproduce the error with that symlink setup, both with ./all.bash and go test cmd/internal/moddeps.

I'll work on a fix.

I also want to confirm whether this type of symlink setup is something we support, or if it shouldn't be supported. If it should be supported, we should consider adding a builder (or test, if viable) to provide coverage to help catch similar regressions.

CC @bcmills.

@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels May 19, 2021
@dmitshur dmitshur added this to the Backlog milestone May 19, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 19, 2021

I understand this test never worked in the past, it just passed because it found no modules. This is because filepath.Walk doesn't follow symlinks.

I'm currently leaning towards fixing this by detecting when filepath.EvalSymlinks(runtime.GOROOT()) doesn't match runtime.GOROOT(), rather than trying to make these tests support symlink setups, unless it turns out very easy and reliable to get it to work. It's not a goal for moddeps tests to be able to run everywhere, they just need to run somewhere, so it's not worth it to add complexity to them.

@bcmills
Copy link
Member

@bcmills bcmills commented May 19, 2021

I also want to confirm whether this type of symlink setup is something we support, or if it shouldn't be supported.

It should be ok for GOROOT proper to be a symlink.
(I don't think we want to support constructing a GOROOT by symlinking in little parts like src or api, but it doesn't sound like that's the case here.)

@bcmills
Copy link
Member

@bcmills bcmills commented May 19, 2021

Based on previous experience, I suspect that the problem is here:

cmd.Env = append(os.Environ(), "GO111MODULE=on")
cmd.Dir = dir

When we set cmd.Dir to a different directory, we may also need to update the PWD environment variable to point to our desired name for that directory. Otherwise, os.Getwd may detect that the two don't match and fall back to a system-canonical name for the directory, when then might not syntactically match GOROOT.

@bcmills
Copy link
Member

@bcmills bcmills commented May 19, 2021

(We may need to make the same change in the other places where we set the Dir field of a command, too.)

@heschi
Copy link
Contributor

@heschi heschi commented May 19, 2021

golang.org/x/tools/internal/gocommand handles this stuff; I wonder if it would be worth cutting down its dependencies and using it in the stdlib tests.

@bcmills
Copy link
Member

@bcmills bcmills commented Jul 9, 2021

@aarzilli, was this fixed by CL 304449?

@bcmills bcmills added the WaitingForInfo label Jul 9, 2021
@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Jul 9, 2021

No, the original report is against a commit that happened after dade83a

@aarzilli
Copy link
Contributor Author

@aarzilli aarzilli commented Jul 9, 2021

PS. I actually took a look at fixing this a few days ago. It turns out that there are many places in that set of tests that assumes runtime.GOROOT() is not a symlink.

@bcmills bcmills removed the WaitingForInfo label Jul 9, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 3, 2021

Change https://golang.org/cl/347351 mentions this issue: cmd/internal/moddeps: skip some tests when GOROOT is a symlink

@dmitshur dmitshur removed their assignment Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

5 participants