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.

Sign up for GitHub

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/go/internal/gcimporter: position filename inconsistency for os/signal/internal/pty #44339

Closed
mdempsky opened this issue Feb 17, 2021 · 8 comments
Closed

x/tools/go/internal/gcimporter: position filename inconsistency for os/signal/internal/pty #44339

mdempsky opened this issue Feb 17, 2021 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@mdempsky
Copy link
Contributor

In CL 292351, I added a test to compare cmd/compile's export data output against gcimporter.IExportData. Because cmd/compile generates paths prefixed with "$GOROOT" whereas go/loader generates them with the actual build.Context.GOROOT value, I included code to expand $GOROOT.

However, this turned out to break on the macOS machines, because for package os/signal/internal/pty (and only that package!) go/loader was using "/var/folders/..." but cmd/compile's expanded path became "/private/var/folders/...". The former is a symlink to the latter, so I submitted CL 292910 to workaround this special case.

But the toothrot builder is still unhappy (https://build.golang.org/?repo=golang.org%2fx%2ftools), now because of "/private/tmp" vs "/tmp" (and again, only for os/signal/internal/pty): https://build.golang.org/log/d0edf17481286f622c2764cf38ab1831375ccb94

I don't think the "/private" vs no-"/private" discrepancy is a big deal. They're semantically equivalent paths. But it's bizarre that os/signal/internal/pty is the only package affected by this.

One suspicion: I think os/signal/internal/pty is the only "internal" package that uses cgo?

/cc @findleyr @stamblerre
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 17, 2021
@gopherbot gopherbot added this to the Unreleased milestone Feb 17, 2021
@stamblerre stamblerre changed the title x/tools/go/internal/gcimporter: position fiilename inconsistency for os/signal/internal/pty on macOS x/tools/go/internal/gcimporter: position filename inconsistency for os/signal/internal/pty on macOS Feb 17, 2021
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 17, 2021
@toothrot
Copy link
Contributor

/cc @griesemer as well as per dev.golang.org/owners

gopherbot pushed a commit to golang/tools that referenced this issue Feb 17, 2021
@mdempsky
go/internal/gcimporter: reference golang/go#44339 in TODO
4534fc3 
Change-Id: Ifb3f1b03a8e0b58465e088cdda3e0357ce648124
Reviewed-on: https://go-review.googlesource.com/c/tools/+/293249
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/293249 mentions this issue: go/internal/gcimporter: reference golang/go#44339 in TODO

@mdempsky mdempsky changed the title x/tools/go/internal/gcimporter: position filename inconsistency for os/signal/internal/pty on macOS x/tools/go/internal/gcimporter: position filename inconsistency for os/signal/internal/pty Feb 17, 2021
@mdempsky
Copy link
Contributor Author

mdempsky commented Feb 17, 2021 •
edited
Loading

I can reproduce the failure on Linux. All you need to do is (from within the x/tools checkout):

ln -s $(go env GOROOT) /tmp/fake-goroot
GOROOT=/tmp/fake-goroot go test -run IExportData_stdlib ./go/internal/gcimporter

@mdempsky
Copy link
Contributor Author

I think it's go/loader that's being inconsistent here. It uses /tmp/fake-goroot-prefixed paths for all of the declared objects, except for in os/signal/internal/pty.

@mdempsky
Copy link
Contributor Author

The internal path is a red herring. It's actually that os/signal/internal/pty is the only package to have exported declarations in a file that uses import "C".

When the file is rewritten by cgo, cgo is invoked from the source directory and uses the actual path in //line directives, instead of the symlink-based path that go/loader uses for everything else.

@bcmills
Copy link
Contributor

bcmills commented Feb 17, 2021

cgo is invoked from the source directory and uses the actual path

That probably points to a PWD issue somewhere in cmd/go or cmd/cgo (compare #43862).

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/293250 mentions this issue: go/internal/cgo: set pkgdir with -srcdir instead of CWD

leitzler added a commit to govim/govim that referenced this issue Feb 18, 2021
@leitzler
deps: update x/tools and gopls to 4b197900
e147cdd 
* txtar: minor fix in unit test input 4b197900
* internal/lsp: 'go get' packages instead of modules 9eb35354
* go/internal/cgo: set pkgdir with -srcdir instead of CWD 67e49ef2
* go/analysis/unitchecker: tell the user how to list the flags and analyzers 19ff21fb
* internal/lsp/command: rename package generate to gen d5b83329
* go/internal/gcimporter: reference golang/go#44339 in TODO 4534fc34
* go/internal/gcimporter: fix tests on darwin 35839b70
* go/internal/gcimporter: add "bundled" export data formats a1db63cc
* go/internal/gcimporter: fix reexporting compiler data b79f76fe
* go/internal/gcimporter: refactor IExportData tests 7fde01ff
* go/internal/gcimporter: simplify IExportData API 6055ccf0
* internal/lsp: refactor go command error handling 1e7abacf
* internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil ffc20750
* internal/lsp: handle nil pointer with import shortcut = link fca89925
* internal/typesinternal: sync error codes with go1.16 5848b84f
* go/analysis/passes: add sigchanyzer Analyzer 3a5a0519
* godoc/vfs: add io/fs adapter 123adc86
@leitzler leitzler mentioned this issue Feb 18, 2021
Merged
leitzler added a commit to govim/govim that referenced this issue Feb 18, 2021
@leitzler
deps: update x/tools and gopls to 4b197900 (#1047)
fc43d75 
* txtar: minor fix in unit test input 4b197900
* internal/lsp: 'go get' packages instead of modules 9eb35354
* go/internal/cgo: set pkgdir with -srcdir instead of CWD 67e49ef2
* go/analysis/unitchecker: tell the user how to list the flags and analyzers 19ff21fb
* internal/lsp/command: rename package generate to gen d5b83329
* go/internal/gcimporter: reference golang/go#44339 in TODO 4534fc34
* go/internal/gcimporter: fix tests on darwin 35839b70
* go/internal/gcimporter: add "bundled" export data formats a1db63cc
* go/internal/gcimporter: fix reexporting compiler data b79f76fe
* go/internal/gcimporter: refactor IExportData tests 7fde01ff
* go/internal/gcimporter: simplify IExportData API 6055ccf0
* internal/lsp: refactor go command error handling 1e7abacf
* internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil ffc20750
* internal/lsp: handle nil pointer with import shortcut = link fca89925
* internal/typesinternal: sync error codes with go1.16 5848b84f
* go/analysis/passes: add sigchanyzer Analyzer 3a5a0519
* godoc/vfs: add io/fs adapter 123adc86
myitcv added a commit to govim/govim that referenced this issue Feb 19, 2021
@myitcv
deps: update x/tools and gopls to f3748ed8
7b92c06 
* internal/lsp/source: filter out comparable from completion results f3748ed8
* go/analysis/passes/buildtag: update check for //go:build f47cb783
* go/loader: fix race 06713c25
* internal/regtest: fix regtests for the dev.typeparams Go branch 1f00549a
* txtar: minor fix in unit test input 4b197900
* internal/lsp: 'go get' packages instead of modules 9eb35354
* go/internal/cgo: set pkgdir with -srcdir instead of CWD 67e49ef2
* go/analysis/unitchecker: tell the user how to list the flags and analyzers 19ff21fb
* internal/lsp/command: rename package generate to gen d5b83329
* go/internal/gcimporter: reference golang/go#44339 in TODO 4534fc34
* go/internal/gcimporter: fix tests on darwin 35839b70
* go/internal/gcimporter: add "bundled" export data formats a1db63cc
* go/internal/gcimporter: fix reexporting compiler data b79f76fe
* go/internal/gcimporter: refactor IExportData tests 7fde01ff
* go/internal/gcimporter: simplify IExportData API 6055ccf0
* internal/lsp: refactor go command error handling 1e7abacf
* internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil ffc20750
* internal/lsp: handle nil pointer with import shortcut = link fca89925
* internal/typesinternal: sync error codes with go1.16 5848b84f
* go/analysis/passes: add sigchanyzer Analyzer 3a5a0519
* godoc/vfs: add io/fs adapter 123adc86
myitcv added a commit to govim/govim that referenced this issue Feb 19, 2021
@myitcv
deps: update x/tools and gopls to f3748ed8
7a70b56 
Also includes some fixes for our CI build to work around
golang.org/issue/40067

* internal/lsp/source: filter out comparable from completion results f3748ed8
* go/analysis/passes/buildtag: update check for //go:build f47cb783
* go/loader: fix race 06713c25
* internal/regtest: fix regtests for the dev.typeparams Go branch 1f00549a
* txtar: minor fix in unit test input 4b197900
* internal/lsp: 'go get' packages instead of modules 9eb35354
* go/internal/cgo: set pkgdir with -srcdir instead of CWD 67e49ef2
* go/analysis/unitchecker: tell the user how to list the flags and analyzers 19ff21fb
* internal/lsp/command: rename package generate to gen d5b83329
* go/internal/gcimporter: reference golang/go#44339 in TODO 4534fc34
* go/internal/gcimporter: fix tests on darwin 35839b70
* go/internal/gcimporter: add "bundled" export data formats a1db63cc
* go/internal/gcimporter: fix reexporting compiler data b79f76fe
* go/internal/gcimporter: refactor IExportData tests 7fde01ff
* go/internal/gcimporter: simplify IExportData API 6055ccf0
* internal/lsp: refactor go command error handling 1e7abacf
* internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil ffc20750
* internal/lsp: handle nil pointer with import shortcut = link fca89925
* internal/typesinternal: sync error codes with go1.16 5848b84f
* go/analysis/passes: add sigchanyzer Analyzer 3a5a0519
* godoc/vfs: add io/fs adapter 123adc86
@myitcv myitcv mentioned this issue Feb 19, 2021
Merged
myitcv added a commit to govim/govim that referenced this issue Feb 19, 2021
@myitcv
deps: update x/tools and gopls to f3748ed8
452f888 
Also includes some fixes for our CI build to work around
golang.org/issue/40067

* internal/lsp/source: filter out comparable from completion results f3748ed8
* go/analysis/passes/buildtag: update check for //go:build f47cb783
* go/loader: fix race 06713c25
* internal/regtest: fix regtests for the dev.typeparams Go branch 1f00549a
* txtar: minor fix in unit test input 4b197900
* internal/lsp: 'go get' packages instead of modules 9eb35354
* go/internal/cgo: set pkgdir with -srcdir instead of CWD 67e49ef2
* go/analysis/unitchecker: tell the user how to list the flags and analyzers 19ff21fb
* internal/lsp/command: rename package generate to gen d5b83329
* go/internal/gcimporter: reference golang/go#44339 in TODO 4534fc34
* go/internal/gcimporter: fix tests on darwin 35839b70
* go/internal/gcimporter: add "bundled" export data formats a1db63cc
* go/internal/gcimporter: fix reexporting compiler data b79f76fe
* go/internal/gcimporter: refactor IExportData tests 7fde01ff
* go/internal/gcimporter: simplify IExportData API 6055ccf0
* internal/lsp: refactor go command error handling 1e7abacf
* internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil ffc20750
* internal/lsp: handle nil pointer with import shortcut = link fca89925
* internal/typesinternal: sync error codes with go1.16 5848b84f
* go/analysis/passes: add sigchanyzer Analyzer 3a5a0519
* godoc/vfs: add io/fs adapter 123adc86
myitcv added a commit to govim/govim that referenced this issue Feb 19, 2021
@myitcv
deps: update x/tools and gopls to f3748ed8 (#1048)
b670a89 
Also includes some fixes for our CI build to work around
golang.org/issue/40067

* internal/lsp/source: filter out comparable from completion results f3748ed8
* go/analysis/passes/buildtag: update check for //go:build f47cb783
* go/loader: fix race 06713c25
* internal/regtest: fix regtests for the dev.typeparams Go branch 1f00549a
* txtar: minor fix in unit test input 4b197900
* internal/lsp: 'go get' packages instead of modules 9eb35354
* go/internal/cgo: set pkgdir with -srcdir instead of CWD 67e49ef2
* go/analysis/unitchecker: tell the user how to list the flags and analyzers 19ff21fb
* internal/lsp/command: rename package generate to gen d5b83329
* go/internal/gcimporter: reference golang/go#44339 in TODO 4534fc34
* go/internal/gcimporter: fix tests on darwin 35839b70
* go/internal/gcimporter: add "bundled" export data formats a1db63cc
* go/internal/gcimporter: fix reexporting compiler data b79f76fe
* go/internal/gcimporter: refactor IExportData tests 7fde01ff
* go/internal/gcimporter: simplify IExportData API 6055ccf0
* internal/lsp: refactor go command error handling 1e7abacf
* internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil ffc20750
* internal/lsp: handle nil pointer with import shortcut = link fca89925
* internal/typesinternal: sync error codes with go1.16 5848b84f
* go/analysis/passes: add sigchanyzer Analyzer 3a5a0519
* godoc/vfs: add io/fs adapter 123adc86
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/295049 mentions this issue: [gopls-release-branch.0.6] all: merge master into gopls-release-branch.0.6

gopherbot pushed a commit to golang/tools that referenced this issue Feb 22, 2021
@stamblerre
[gopls-release-branch.0.6] all: merge master into gopls-release-branc…
77e0312 
…h.0.6

0e232fa gopls: add scheme to CodeDescription.href
2363391 all: go fmt ./...
b369640 all: fmt tests with new gofmt
f6e0443 go/analysis: add unusedwrite pass
bdaa8bf go/gcexportdata: warn that {Read,Write}Bundle are experimental
f4301d9 internal/imports: update stdlib index for 1.16
f3748ed internal/lsp/source: filter out comparable from completion results
f47cb78 go/analysis/passes/buildtag: update check for //go:build
06713c2 go/loader: fix race
1f00549 internal/regtest: fix regtests for the dev.typeparams Go branch
4b19790 txtar: minor fix in unit test input
9eb3535 internal/lsp: 'go get' packages instead of modules
67e49ef go/internal/cgo: set pkgdir with -srcdir instead of CWD
19ff21f go/analysis/unitchecker: tell the user how to list the flags and analyzers
d5b8332 internal/lsp/command: rename package generate to gen
4534fc3 go/internal/gcimporter: reference golang/go#44339 in TODO
35839b7 go/internal/gcimporter: fix tests on darwin
a1db63c go/internal/gcimporter: add "bundled" export data formats
b79f76f go/internal/gcimporter: fix reexporting compiler data
7fde01f go/internal/gcimporter: refactor IExportData tests
6055ccf go/internal/gcimporter: simplify IExportData API
1e7abac internal/lsp: refactor go command error handling
ffc2075 internal/lsp: fix nil pointer in hover when (types.Object).Pkg() is nil
fca8992 internal/lsp: handle nil pointer with import shortcut = link
5848b84 internal/typesinternal: sync error codes with go1.16
3a5a051 go/analysis/passes: add sigchanyzer Analyzer

Change-Id: I1d4a41669e2f8e8115fb5d62cc25a62f7c755ba2
@golang golang locked and limited conversation to collaborators Feb 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. 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
4 participants
@toothrot @mdempsky @bcmills @gopherbot