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

Jump to bottom

cmd/go: TestScript/cgo_stale_precompiled fails when GOROOT_FINAL points to another Go tree but GOROOT isn't explicitly set #50893

Closed
dmitshur opened this issue Jan 28, 2022 · 6 comments
Closed

cmd/go: TestScript/cgo_stale_precompiled fails when GOROOT_FINAL points to another Go tree but GOROOT isn't explicitly set #50893

dmitshur opened this issue Jan 28, 2022 · 6 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.18

Comments

@dmitshur
Copy link
Contributor

At tip:

$ go version
go version devel go1.18-9ff0039848 Fri Jan 28 02:40:34 2022 +0000 darwin/amd64

The TestScript/cgo_stale_precompiled test passes when GOROOT_FINAL is empty:

$ GOROOT_FINAL= go test -count=1 -run='^TestScript/cgo_stale_precompiled$' cmd/go 
ok  	cmd/go	2.549s

But fails when non-empty:

$ GOROOT_FINAL=/usr/local/go go test -count=1 -run='^TestScript/cgo_stale_precompiled$' cmd/go 
go test proxy running at GOPROXY=http://127.0.0.1:51586/mod
--- FAIL: TestScript (0.01s)
    --- FAIL: TestScript/cgo_stale_precompiled (0.25s)
        script_test.go:257: 
            # Regression test for https://go.dev/issue/47215 and https://go.dev/issue/50183:
            # A mismatched $GOROOT_FINAL or missing $CC caused the C dependencies of the net
            # package to appear stale, and it could not be rebuilt due to a missing $CC. (0.000s)
            # Control case: net must not already be stale. (0.186s)
            > ! stale net
            FAIL: testdata/script/cgo_stale_precompiled.txt:8: net is unexpectedly stale
            
            
FAIL
FAIL	cmd/go	2.653s
FAIL

gotip $ cd src 
src $ ./make.bash
Building Go cmd/dist using /usr/local/go. (go1.17.6 darwin/amd64)
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for darwin/amd64.
---
Installed Go for darwin/amd64 in /Users/dmitshur/gotip
Installed commands in /Users/dmitshur/gotip/bin
src $ export PATH="/Users/dmitshur/gotip/bin:$PATH"
src $ go version
go version devel go1.18-9ff0039848 Fri Jan 28 02:40:34 2022 +0000 darwin/amd64
src $ which go
/Users/dmitshur/gotip/bin/go
src $ go test -count=1 -v -run='^TestScript/cgo_stale_precompiled$' cmd/go
=== RUN   TestScript
=== RUN   TestScript/cgo_stale_precompiled
=== PAUSE TestScript/cgo_stale_precompiled
=== CONT  TestScript/cgo_stale_precompiled
go test proxy running at GOPROXY=http://127.0.0.1:50177/mod
    script_test.go:257: 
        WORK=$WORK
        PATH=/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/cmd-go-test-1358067462/tmpdir465175891/testbin:/Users/dmitshur/gotip/bin:/Users/dmitshur/google-cloud-sdk/bin:/Users/dmitshur/homebrew/bin:/Users/dmitshur/homebrew/sbin:/usr/local/git/current/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/local/go/bin:/Library/Apple/usr/bin:/Users/dmitshur/go/bin:/Users/dmitshur/bin
        HOME=/no-home
        CCACHE_DISABLE=1
        GOARCH=amd64
        GOCACHE=/Users/dmitshur/Library/Caches/go-build
        GODEBUG=
        GOEXE=
        GOOS=darwin
        GOPATH=$WORK/gopath
        GOPROXY=http://127.0.0.1:50177/mod
        GOPRIVATE=
        GOROOT=/Users/dmitshur/gotip
        GOROOT_FINAL=
        GOTRACEBACK=system
        TESTGO_GOROOT=/Users/dmitshur/gotip
        GOSUMDB=localhost.localdev/sumdb+00000c67+AcTrnkbUA+TU4heY3hkjiSES/DSQniBqIeQ/YppAUtK6
        GONOPROXY=
        GONOSUMDB=
        GOVCS=*:all
        PWD=$WORK/gopath/src
        TMPDIR=$WORK/tmp
        devnull=/dev/null
        goversion=1.18
        :=:
        /=/
        
        # Regression test for https://go.dev/issue/47215 and https://go.dev/issue/50183:
        # A mismatched $GOROOT_FINAL or missing $CC caused the C dependencies of the net
        # package to appear stale, and it could not be rebuilt due to a missing $CC. (0.000s)
        > [!cgo] skip
        # Control case: net must not already be stale. (0.223s)
        > ! stale net
        # https://go.dev/issue/47215: a missing $(go env CC) caused the precompiled net to be stale. (0.240s)
        > [!plan9] env PATH=''  # Guaranteed not to include $(go env CC)!
        > [plan9] env path=''
        > ! stale net  # issue #47215
        # https://go.dev/issue/50183: a mismatched GOROOT_FINAL caused net to be stale. (0.201s)
        > env GOROOT_FINAL=$WORK${/}goroot
        > ! stale net
        PASS
        
--- PASS: TestScript (0.01s)
    --- PASS: TestScript/cgo_stale_precompiled (0.73s)
PASS
ok  	cmd/go	5.487s
src $ GOROOT_FINAL=/usr/local/go go test -count=1 -v -run='^TestScript/cgo_stale_precompiled$' cmd/go
=== RUN   TestScript
=== RUN   TestScript/cgo_stale_precompiled
=== PAUSE TestScript/cgo_stale_precompiled
=== CONT  TestScript/cgo_stale_precompiled
go test proxy running at GOPROXY=http://127.0.0.1:50240/mod
    script_test.go:257: 
        WORK=$WORK
        PATH=/var/folders/zb/5p8cwfhj29gf_m8vdy8ylmlr00jwcj/T/cmd-go-test-1387689323/tmpdir4260322383/testbin:/Users/dmitshur/gotip/bin:/Users/dmitshur/google-cloud-sdk/bin:/Users/dmitshur/homebrew/bin:/Users/dmitshur/homebrew/sbin:/usr/local/git/current/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/local/go/bin:/Library/Apple/usr/bin:/Users/dmitshur/go/bin:/Users/dmitshur/bin
        HOME=/no-home
        CCACHE_DISABLE=1
        GOARCH=amd64
        GOCACHE=/Users/dmitshur/Library/Caches/go-build
        GODEBUG=
        GOEXE=
        GOOS=darwin
        GOPATH=$WORK/gopath
        GOPROXY=http://127.0.0.1:50240/mod
        GOPRIVATE=
        GOROOT=/usr/local/go
        GOROOT_FINAL=/usr/local/go
        GOTRACEBACK=system
        TESTGO_GOROOT=/usr/local/go
        GOSUMDB=localhost.localdev/sumdb+00000c67+AcTrnkbUA+TU4heY3hkjiSES/DSQniBqIeQ/YppAUtK6
        GONOPROXY=
        GONOSUMDB=
        GOVCS=*:all
        PWD=$WORK/gopath/src
        TMPDIR=$WORK/tmp
        devnull=/dev/null
        goversion=1.18
        :=:
        /=/
        
        # Regression test for https://go.dev/issue/47215 and https://go.dev/issue/50183:
        # A mismatched $GOROOT_FINAL or missing $CC caused the C dependencies of the net
        # package to appear stale, and it could not be rebuilt due to a missing $CC. (0.000s)
        > [!cgo] skip
        # Control case: net must not already be stale. (0.185s)
        > ! stale net
        FAIL: testdata/script/cgo_stale_precompiled.txt:8: net is unexpectedly stale
        
        
--- FAIL: TestScript (0.01s)
    --- FAIL: TestScript/cgo_stale_precompiled (0.24s)
FAIL
FAIL	cmd/go	6.340s
FAIL
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Jan 28, 2022
@dmitshur dmitshur added this to the Go1.18 milestone Jan 28, 2022
@dmitshur dmitshur mentioned this issue Jan 28, 2022
Closed
@dmitshur
Copy link
Contributor Author

But fails when non-empty

I've learned it happens only when GOROOT_FINAL is set to a non-empty directory. In my case, /usr/local/go has a Go 1.17.6 installation. When testing with GOROOT_FINAL set to another empty directory (or by temporarily moving /usr/local/go out of the way) the test passes.

@dmitshur
Copy link
Contributor Author

dmitshur commented Jan 28, 2022 •
edited

More findings, it seems this failure goes away if GOROOT is also set explicitly:

~ $ cd gotip 
gotip $ export PATH="$HOME/gotip/bin:$PATH"
gotip $ go test -count=1 -run='^TestScript/cgo_stale_precompiled$' cmd/go
ok  	cmd/go	2.504s
gotip $ go test -count=1 -run='^TestScript/cgo_stale_precompiled$' cmd/go
ok  	cmd/go	2.206s
gotip $ export GOROOT_FINAL=/usr/local/go
gotip $ go test -count=1 -run='^TestScript/cgo_stale_precompiled$' cmd/go
go test proxy running at GOPROXY=http://127.0.0.1:59284/mod
--- FAIL: TestScript (0.01s)
    --- FAIL: TestScript/cgo_stale_precompiled (0.25s)
        script_test.go:257: 
            # Regression test for https://go.dev/issue/47215 and https://go.dev/issue/50183:
            # A mismatched $GOROOT_FINAL or missing $CC caused the C dependencies of the net
            # package to appear stale, and it could not be rebuilt due to a missing $CC. (0.000s)
            # Control case: net must not already be stale. (0.188s)
            > ! stale net
            FAIL: testdata/script/cgo_stale_precompiled.txt:8: net is unexpectedly stale
            
            
FAIL
FAIL	cmd/go	3.204s
FAIL
gotip $ export GOROOT=$HOME/gotip                                  
gotip $ go test -count=1 -run='^TestScript/cgo_stale_precompiled$' cmd/go
ok  	cmd/go	2.224s

x/build/cmd/release sets both GOROOT and GOROOT_FINAL explicitly, suggesting this issue may be less connected to #50892 than I originally thought.

@bcmills
Copy link
Contributor

bcmills commented Jan 28, 2022

cmd/go builds a test variant of itself using testenv.GoTool() with an unmodified environment.

If GOROOT_FINAL is set explicitly in the environment but GOROOT is not set, then testenv.GoTool() will report the location of filepath.Join(runtime.GOROOT(), "bin", "go"+exeSuffix), which will be $GOROOT_FINAL/bin/go. If GOROOT is also unset, then go build will try to bootstrap itself using the go in GOROOT_FINAL, and AFAIK the whole thing will end up running with the wrong GOROOT baked in. I don't know exactly how that will work out, but it can't be good.

Probably a good first step would be to avoid the self-rebuild cycle (probably with something like https://go.dev/cl/378294, thanks @mvdan!). But I'm also learning that it's probably not a good idea to set GOROOT_FINAL without also explicitly setting GOROOT at the same time. 😅

@dmitshur
Copy link
Contributor Author

Now that we understand the problem in #50892 better, it is clear my original reproduce attempt that only set GOROOT_FINAL explicitly, but did not set GOROOT explicitly, was misleading.

But I'm also learning that it's probably not a good idea to set GOROOT_FINAL without also explicitly setting GOROOT at the same time. 😅

Agree. I think if it is possible, we should simply make it a fatal error if GOROOT_FINAL has a non-empty value while GOROOT environment variable is not set, and avoid needing to support such a scenario. That would close this issue.

@dmitshur dmitshur changed the title cmd/go: TestScript/cgo_stale_precompiled fails when GOROOT_FINAL is set cmd/go: TestScript/cgo_stale_precompiled fails when GOROOT_FINAL points to another Go tree but GOROOT isn't explicitly set Jan 28, 2022
@dmitshur dmitshur added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 28, 2022
@gopherbot
Copy link

Change https://golang.org/cl/381854 mentions this issue: cmd/go: rewrite TestScript/cgo_stale_precompiled to be agnostic to staleness

@gopherbot
Copy link

Change https://golang.org/cl/381834 mentions this issue: internal/testenv: add a test for the GoTool function

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 31, 2022
@dmitshur dmitshur mentioned this issue Mar 5, 2022
Open
gopherbot pushed a commit that referenced this issue Aug 17, 2022
@gopherbot
internal/testenv: add a test for the GoTool function
bd1bff4 
GoTool was added in CL 20967, and revised in CL 21292, for #14901.

I don't fully understand what problem the GoTool function was added to
solve: the discussion on that issue was pretty sparse, but it seems
like when we run tests of GOROOT packages they always know their own
location relative to GOROOT (and thus always know where to find the
'go' tool).

Lacking that understanding, I don't want to change its behavior, but I
do at least want to verify that it resolves to the real 'go' tool in
the common case (running 'go test' on a package in GOROOT/src).

For #50892
For #50893
Updates #14901

Change-Id: I06d831e6765be631dfc4854d7fddc3d27fc1de34
Reviewed-on: https://go-review.googlesource.com/c/go/+/381834
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jan 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@dmitshur @bcmills @gopherbot