cmd/go: fix listing of ambiguous paths #34663

Helcaraxan · 2019-10-02T17:25:59Z

Passing ambiguous patterns, ending in .go, to go list results in them
being interpreted as Go files despite potentially being package references.
This can then result in errors on other package references.

The parsing logic is modified to check for a locally present file
corresponding to any pattern ending in .go. If no such file is present
the pattern is considered to be a package reference.

We're also adding a variety of non-regression tests that fail with the
original parsing code but passes after applying the fix.

Fixes #32483
Fixes #34653

gopherbot · 2019-10-02T17:27:39Z

This PR (HEAD: bd2d8f6) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/198459 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-10-02T17:29:44Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-02T18:01:10Z

Message from Bryan C. Mills:

Patch Set 1:

(5 comments)

Thanks for the CL!

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-02T18:21:14Z

Message from Jay Conrod:

Patch Set 1:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T08:19:24Z

This PR (HEAD: 0afdc3e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/198459 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-10-03T08:22:27Z

Message from Duco van Amstel:

Patch Set 1:

(8 comments)

I have addressed the comments. There was initially some confusion on my side (now resolved thanks to your suggestions!) about the supported use-cases for patterns passed to go list.

The new patchset is smaller and should be significantly less impactful from a performance perspective with the os.Stat moved to only affect the 'Go file path' case.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T20:29:28Z

Message from Jay Conrod:

Patch Set 4: Run-TryBot+1

(1 comment)

Looks good, with one very small nit to fix.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T20:29:29Z

Message from Gobot Gobot:

Patch Set 4:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=86b58c07

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T20:34:39Z

Message from Bryan C. Mills:

Patch Set 4: Code-Review+2

LGTM modulo Jay's comment.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T20:34:40Z

Message from Gobot Gobot:

Patch Set 4:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/86b58c07/freebsd-amd64-12_0_9869e350.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T20:36:43Z

Message from Bryan C. Mills:

Patch Set 4: Code-Review+1

TryBot failure looks legitimate. Please update mod_get_trailing_slash.txt.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-03T20:40:42Z

Message from Gobot Gobot:

Patch Set 4: TryBot-Result-1

6 of 19 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/86b58c07/freebsd-amd64-12_0_9869e350.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/86b58c07/linux-386_560e755f.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/86b58c07/linux-amd64_e63e2d49.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/86b58c07/openbsd-amd64-64_85d0af1d.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/86b58c07/windows-amd64-2016_a85e94ee.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/86b58c07/windows-386-2008_69d9804b.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T10:14:50Z

This PR (HEAD: 4a9bc42) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/198459 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-10-04T10:22:01Z

Message from Duco van Amstel:

Patch Set 6:

I have updated the comment as per the request.

The test failure led me to also consider the initial fix that was made for golang.org/issue/32483 as it appears to no longer be necessary.

I have updated the failing test to assert the new and fixed behaviour of go list on packages ending in ".go". It was also possible to remove the code that was appending a "/" in go get for such packages as this CL's change also affects (and fixes) the codepath for go get.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T10:33:23Z

Message from Duco van Amstel:

Patch Set 6:

I have updated the failing test to assert the new and fixed behaviour of go list on packages ending in ".go". It was also possible to remove the code that was appending a "/" in go get for such packages as this CL's change also affects (and fixes) the codepath for go get.

Aside from the existing unit-tests I have confirmed that the removal of the previous fix is not inducing a regression via a local check on my machine

With go1.13.1:

-> go version
go version go1.13.1 linux/amd64er
-> go get github.com/nats-io/nats.go
stat github.com/nats-io/nats.go: no such file or directory

With the HEAD of this CL's Patchset 6:

-> /data/go/bin/go version
go version devel +4a9bc42b9f Fri Oct 4 11:05:02 2019 +0100 linux/amd64
-> /data/go/bin/go get github.com/nats-io/nats.go
-> echo $?
0

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:23:55Z

Message from Bryan C. Mills:

Patch Set 6: Run-TryBot+1 Code-Review+2

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:24:58Z

Message from Gobot Gobot:

Patch Set 6:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=3bf8b3bd

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:31:42Z

Message from Gobot Gobot:

Patch Set 6:

Build is still in progress...
This change failed on windows-amd64-2016:
See https://storage.googleapis.com/go-build-log/3bf8b3bd/windows-amd64-2016_c0f4ddc4.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:35:58Z

Message from Gobot Gobot:

Patch Set 6: TryBot-Result-1

2 of 19 TryBots failed:
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/3bf8b3bd/windows-amd64-2016_c0f4ddc4.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/3bf8b3bd/windows-386-2008_75acf772.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:39:19Z

Message from Bryan C. Mills:

Patch Set 6:

Patch Set 6:

Build is still in progress...
This change failed on windows-amd64-2016:
See https://storage.googleapis.com/go-build-log/3bf8b3bd/windows-amd64-2016_c0f4ddc4.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Test failure looks genuine. Probably just need to make the expected outputs conditional on the operating system, or agnostic to the errno text.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:44:59Z

Message from Bryan C. Mills:

Patch Set 6: Code-Review+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T12:47:35Z

Message from Bryan C. Mills:

Patch Set 6:

Patch Set 6:

Patch Set 6:

Build is still in progress...
This change failed on windows-amd64-2016:
See https://storage.googleapis.com/go-build-log/3bf8b3bd/windows-amd64-2016_c0f4ddc4.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Test failure looks genuine. Probably just need to make the expected outputs conditional on the operating system, or agnostic to the errno text.

Some examples of those approaches to consider:
•

go/src/cmd/go/testdata/script/env_write.txt

Lines 7 to 14 in 323212b

    
           [aix] stdout $HOME/.config/go/env 
        
           [darwin] stdout $HOME'/Library/Application Support/go/env' 
        
           [freebsd] stdout $HOME/.config/go/env 
        
           [linux] stdout $HOME/.config/go/env 
        
           [netbsd] stdout $HOME/.config/go/env 
        
           [openbsd] stdout $HOME/.config/go/env 
        
           [plan9] stdout $HOME/plan9home/lib/go/env 
        
           [windows] stdout $HOME\\windowsappdata\\go\\env

•

go/src/cmd/go/testdata/script/mod_sumdb_file_path.txt

Line 16 in 6232dad

stderr '^go get golang.org/x/[email protected]: golang.org/x/[email protected]: verifying module: golang.org/x/[email protected]: reading file://.*/sumdb/sum.golang.org/lookup/golang.org/x/[email protected]: (no such file or directory|.*cannot find the file specified.*)'

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T13:05:21Z

This PR (HEAD: 5abc200) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/198459 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-10-04T13:11:05Z

Message from Duco van Amstel:

Patch Set 7:

Have addressed the failing test by platform-dependent error message checks.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T13:20:30Z

Message from Bryan C. Mills:

Patch Set 7: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T13:20:31Z

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=1bfaa22f

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T13:35:52Z

Message from Gobot Gobot:

Patch Set 7: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-10-04T13:44:58Z

Message from Duco van Amstel:

Patch Set 7:

FWIW: I just tested it locally and this change cherry-picks clean on top of release-branch.go1.13 in case we want to back-port this.

Please don’t reply on this GitHub thread. Visit golang.org/cl/198459.
After addressing review feedback, remember to publish your drafts!

Passing ambiguous patterns, ending in `.go`, to `go list` results in them being interpreted as Go files despite potentially being package references. This can then result in errors on other package references. The parsing logic is modified to check for a locally present file corresponding to any pattern ending in `.go`. If no such file is present the pattern is considered to be a package reference. We're also adding a variety of non-regression tests that fail with the original parsing code but passes after applying the fix. Fixes #32483 Fixes #34653 Change-Id: I073871da0dfc5641a359643f95ac14608fdca09b GitHub-Last-Rev: 5abc200 GitHub-Pull-Request: #34663 Reviewed-on: https://go-review.googlesource.com/c/go/+/198459 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>

gopherbot · 2019-10-04T13:49:40Z

This PR is being closed because golang.org/cl/198459 has been merged.

Passing ambiguous patterns, ending in `.go`, to `go list` results in them being interpreted as Go files despite potentially being package references. This can then result in errors on other package references. The parsing logic is modified to check for a locally present file corresponding to any pattern ending in `.go`. If no such file is present the pattern is considered to be a package reference. We're also adding a variety of non-regression tests that fail with the original parsing code but passes after applying the fix. Updates #34653 Fixes #34694 Change-Id: I073871da0dfc5641a359643f95ac14608fdca09b GitHub-Last-Rev: 5abc200 GitHub-Pull-Request: #34663 Reviewed-on: https://go-review.googlesource.com/c/go/+/198459 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]> (cherry picked from commit 33683f1d64df0cef2c598a84b741abb5af8abe5e) Reviewed-on: https://go-review.googlesource.com/c/go/+/198957 Reviewed-by: Jay Conrod <[email protected]>

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Oct 2, 2019

Helcaraxan force-pushed the fix-34653 branch from bd2d8f6 to 0afdc3e Compare October 3, 2019 08:18

Helcaraxan changed the title ~~cmd/go: fix package listing with mixed patterns~~ cmd/go: fix listing of ambiguous paths Oct 3, 2019

Helcaraxan force-pushed the fix-34653 branch from 0afdc3e to 4a9bc42 Compare October 4, 2019 10:14

Helcaraxan and others added 4 commits October 4, 2019 14:03

Add testcase for ambiguous path listing

24bc13d

Ensure we can list packages ending in '.go'

015e5ba

testdata fixup

1e45eda

Remove unnecessary fix for modules ending in '.go'

5abc200

Helcaraxan force-pushed the fix-34653 branch from 4a9bc42 to 5abc200 Compare October 4, 2019 13:04

gopherbot closed this Oct 4, 2019

Helcaraxan deleted the fix-34653 branch October 4, 2019 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/go: fix listing of ambiguous paths #34663

cmd/go: fix listing of ambiguous paths #34663

Helcaraxan commented Oct 2, 2019 •

edited

Loading

gopherbot commented Oct 2, 2019

gopherbot commented Oct 2, 2019

gopherbot commented Oct 2, 2019

gopherbot commented Oct 2, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

cmd/go: fix listing of ambiguous paths #34663

cmd/go: fix listing of ambiguous paths #34663

Conversation

Helcaraxan commented Oct 2, 2019 • edited Loading

gopherbot commented Oct 2, 2019

gopherbot commented Oct 2, 2019

gopherbot commented Oct 2, 2019

gopherbot commented Oct 2, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 3, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 4, 2019

Helcaraxan commented Oct 2, 2019 •

edited

Loading