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

x/tools/internal/imports: TestScanNestedModuleInLocalReplace and 3 more failing on Go tip #35505

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

x/tools/internal/imports: TestScanNestedModuleInLocalReplace and 3 more failing on Go tip #35505

dmitshur opened this issue Nov 11, 2019 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

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
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 11, 2019
@gopherbot gopherbot added this to the Unreleased milestone Nov 11, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 11, 2019
@jayconrod jayconrod self-assigned this Nov 11, 2019
@jayconrod jayconrod added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 11, 2019
@bcmills
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@heschi
Copy link
Contributor

heschi 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
Copy link
Contributor

bcmills commented Nov 11, 2019 •
edited

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
Copy link
Contributor

bcmills commented Nov 11, 2019

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

@dmitshur
Copy link
Contributor Author

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
Copy link

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
internal/testenv: reject the resolved 'go' command if it does not mat…
7dd52f0 
…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
Copy link

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

@golang golang locked and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@jayconrod @dmitshur @bcmills @gopherbot @heschi