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/internal/imports: TestScanNestedModuleInLocalReplace and 3 more failing on Go tip #35505

Closed
dmitshur opened this issue Nov 11, 2019 · 9 comments
Assignees
Labels
Milestone

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 11, 2019

From https://build.golang.org and https://go-review.googlesource.com/c/go/+/204830/4#message-7c1298f605c9bae9bf98fc61fbd88c2b029bdcb1:

--- FAIL: TestScanNestedModuleInLocalReplace (0.03s)
    mod_test.go:66: running go: exit status 1 (stderr:
        invalid version ""
        )
--- FAIL: TestModReplace2 (0.03s)
    mod_test.go:395: running go: exit status 1 (stderr:
        invalid version ""
        )
--- FAIL: TestModReplace3 (0.02s)
    mod_test.go:421: running go: exit status 1 (stderr:
        invalid version ""
        )
--- FAIL: TestModReplaceImport (0.01s)
    mod_test.go:454: running go: exit status 1 (stderr:
        invalid version ""
        invalid version ""
        invalid version ""
        invalid version ""
        invalid version ""
        )
FAIL
FAIL	golang.org/x/tools/internal/imports	27.104s

https://build.golang.org/log/3a4fbffe75af5fddd26d444d1dd2af3d805edee6

/cc @heschik

@gopherbot gopherbot added this to the Unreleased milestone Nov 11, 2019
@gopherbot gopherbot added the Tools label Nov 11, 2019
@jayconrod jayconrod self-assigned this Nov 11, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 11, 2019

I've tried to reproduce this locally but haven't had any success. It seems to have something to do with the builder configuration.

~/src/golang.org/x/tools$ gotip version
go version devel +275a7be3 Mon Nov 11 17:44:21 2019 +0000 linux/amd64

~/src/golang.org/x/tools$ gotip test golang.org/x/tools/internal/imports
ok      golang.org/x/tools/internal/imports     21.546s
@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Nov 11, 2019

I was able to reproduce this locally. The test is setting up a workspace and running go mod download there as part of the setup. An empty version gets passed to one of the modfetch functions.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 11, 2019

It's probably related to CL 189797, then.

I'm honestly more worried about the non-reproducibility of the failure than the failure itself — neither @iWdGo nor I were able to reproduce it locally, despite attempting to do so.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Nov 11, 2019

Like many x/tools tests, these run the go command, so gotip test is insufficient. Tip go needs to be the first thing on $PATH. Should be easily reproducible from there.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 11, 2019

gotip adds itself to PATH, but at the end rather than the beginning:
https://github.com/golang/dl/blob/530a690d93eb507301a30e1a91f5819971abaec8/internal/version/version.go#L59-L63

Probably it should add itself to the beginning of PATH instead.

OTOH, it's not uncommon for me to run something like ~go-review/bin/go test […] and expect that to actually test against the go-review build of the go standard library and toolchain. Arguably the tests of packages in x/tools should be verifying their behavior against the go command that runs them, not whatever go command happens to be in PATH. That suggests that the test itself should update PATH to prefer GOROOT/bin.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Nov 11, 2019

(There are likely many tests in the standard library that need a similar fix.)

@dmitshur

This comment has been minimized.

Copy link
Member Author

@dmitshur dmitshur commented Nov 11, 2019

Tip go needs to be the first thing on $PATH.

All other golang.org/dl/go... binaries arrange for that to happen, as @bcmills just said. golang.org/dl/gotip needs to be fixed to do the same. Filed #35507.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 11, 2019

Change https://golang.org/cl/206517 mentions this issue: internal/testenv: reject the resolved 'go' command if it does not match runtime.GOROOT

gopherbot pushed a commit to golang/tools that referenced this issue Nov 11, 2019
…ch runtime.GOROOT

Many tests in x/tools invoke the 'go' command found from $PATH.
If that command does not match the 'go' command used to invoke 'go test',
the result will be misleading.

Instead of silently accepting the mismatched result, check the 'go'
tool's self-reported GOROOT and reject it if it doesn't match the 'go'
tool used to invoke 'go test'.

That rejection will cause the x/tools tests to fail if x/tools is the main module.

Updates golang/go#35505

Change-Id: I581906468ef736fad42a0164376a07f876907621
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206517
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 11, 2019

Change https://golang.org/cl/206538 mentions this issue: cmd/go/internal/modcmd: skip modules with empty version strings

@gopherbot gopherbot closed this in d431804 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.