Closed codyoss closed 1 year ago
Thank you for the report. I can reproduce with gopls, and with the Go command:
> go list cloud.google.com/go/internal/postprocessor/...
pattern cloud.google.com/go/internal/postprocessor/...: cloud.google.com/go@v0.110.0: missing go.sum entry
> go mod tidy
> go list cloud.google.com/go/internal/postprocessor/...
pattern cloud.google.com/go/internal/postprocessor/...: cloud.google.com/go@v0.110.0: missing go.sum entry
@bcmills @matloob is this a Go command bug?
This is not a go
command bug per se. go mod tidy
is correct that no checksum is needed for cloud.google.com/go
, because that module is not directly required in the go.mod
file and does not provide any package needed to build or test any package imported by github.com/googleapis/google-cloud-go/internal/postprocessor
:
~/src/github.com/googleapis/google-cloud-go/internal/postprocessor$ go list -f '{{with .Module}}{{.Path}}{{end}}' -deps -test all | sort -u | grep cloud.google
cloud.google.com/go/internal/gensnippets
cloud.google.com/go/internal/godocfx
cloud.google.com/go/internal/postprocessor
However, the command go list cloud.google.com/go/internal/postprocessor/...
does require that checksum, because it has a well-defined version for cloud.google.com/go
and needs to check whether that version of that module contains any packages matching the pattern cloud.google.com/go/internal/postprocessor/...
.
So, I would say that this is a bug in the query that gopls
is using to load packages: it should be using a module-relative query (such as ./...
, with the command's working directory in cloud.google.com/go/internal/postprocessor
), rather than a wildcard query that could span multiple modules.
However, the command go list cloud.google.com/go/internal/postprocessor/... does require that checksum, because it has a well-defined version for cloud.google.com/go and needs to check whether that version of that module contains any packages matching the pattern cloud.google.com/go/internal/postprocessor/....
So you're saying that this query also checks for the package ./internal/postprocessor inside the cloud.google.com/go module, right?
it should be using a module-relative query (such as ./..., with the command's working directory in cloud.google.com/go/internal/postprocessor
So in the presence of a go.work file using dirA, dirB, dirC, we'd have to call go/packages.Load("./...") three times, once from each dir, right?
So you're saying that this query also checks for the package ./internal/postprocessor inside the cloud.google.com/go module, right?
Not that package specifically, because it already knows that that one is in the main module. But that pattern would also match packages like cloud.google.com/go/internal/postprocessor/subpkg
, so it has to check whether any subpackages like that exist in the selected version of cloud.google.com/go
.
So in the presence of a go.work file using dirA, dirB, dirC, we'd have to call go/packages.Load("./...") three times, once from each dir, right?
Hmm... You could do that, yes. You could also give directory paths instead of import paths, I think: go list dirA/... dirB/... dirC/...
, which will stop at module boundaries assuming that dirA
, dirB
, and dirC
are absolute paths.
I thought we also had a proposal somewhere about allowing ./...
to cross module boundaries when both modules are in the workspace, but I can't find it at the moment. 🤷♂️
See also:
You could also give directory paths instead of import paths, I think: go list dirA/... dirB/... dirC/..., which will stop at module boundaries assuming that dirA, dirB, and dirC are absolute paths.
We have the absolute absolute paths available, so it sounds like this is the way to go.
I thought we also had a proposal somewhere about allowing ./... to cross module boundaries when both modules are in the workspace,
FWIW that wouldn't help us here, because we don't really want to get into the business of finding a common root for otherwise unrelated dirs (and it shouldn't matter where the GOWORK value lives).
Thanks for the explanation. This is a long standing gopls bug: we've loaded <modulePath>/...
for as long as I can remember.
I thought we also had a proposal somewhere about allowing
./...
to cross module boundaries when both modules are in the workspace, but I can't find it at the moment.
Maybe #51283 is what you were thinking of, though it's more of a tracking issue with Thinking label than a proposal.
I thought we also had a proposal somewhere about allowing ./... to cross module boundaries when both modules are in the workspace, but I can't find it at the moment.
Maybe this https://github.com/golang/go/issues/50745 (for go.work) or https://github.com/golang/go/issues/59480 (for general multi-module repo support)
Change https://go.dev/cl/485840 mentions this issue: gopls/internal/lsp/cache: load modules by dir, not module path
@bcmills I'm working on fixing this in CL 485840, linked above.
This fix is failing on windows only: https://storage.googleapis.com/go-build-log/903a25ae/windows-amd64-2016_fee7a6f8.log
Params: {"token":"2888608224459394542","value":{"kind":"begin","title":"Error loading workspace","message":"3 modules have errors: \tC:\\Users\\gopher\\AppData\\Local\\Temp\\1\\gopls-regtest-4152842788\\TestUseGoWorkOutsideTheWorkspace\\default\\work\\other\\c:pattern C:\\Users\\gopher\\AppData\\Local\\Temp\\1\\gopls-regtest-4152842788\\TestUseGoWorkOutsideTheWorkspace\\default\\work\\other\\c\\...: directory prefix ..\\other\\c does not contain modules listed in go.work or their selected dependencies \tC:\\Users\\gopher\\AppData\\Local\\Temp\\1\\gopls-regtest-4152842788\\TestUseGoWorkOutsideTheWorkspace\\default\\work\\work\\a:pattern C:\\Users\\gopher\\AppData\\Local\\Temp\\1\\gopls-regtest-4152842788\\TestUseGoWorkOutsideTheWorkspace\\default\\work\\work\\a\\...: directory prefix a does not contain modules listed in go.work or their selected dependencies \tC:\\Users\\gopher\\AppData\\Local\\Temp\\1\\gopls-regtest-4152842788\\TestUseGoWorkOutsideTheWorkspace\\default\\work\\work\\b:pattern C:\\Users\\gopher\\AppData\\Local\\Temp\\1\\gopls-regtest-4152842788\\TestUseGoWorkOutsideTheWorkspace\\default\\work\\work\\b\\...: directory prefix b does not contain modules listed in go.work or their selected dependencies "}}
Test is defined here: https://cs.opensource.google/go/x/tools/+/master:gopls/internal/regtest/workspace/fromenv_test.go;l=15;drc=bd48b9a5d7872f3d6b457d49038c4657c7443740
This is a strange failure mode. On one hand the go command appears to be correctly computing the relative directory paths (see the "directory prefix"), yet the query fails.
The GOWORK
environment variable in that test appears to be incorrect:
https://cs.opensource.google/go/x/tools/+/master:gopls/internal/regtest/workspace/fromenv_test.go;l=52;drc=bd48b9a5d7872f3d6b457d49038c4657c7443740
It is using slashes to separate the path elements, where it should be using filepath.Join
. Cleaned paths underneath that directory will use backslashes instead, but the backslashes won't include slashes as a prefix.
@bcmills thanks, I'll see if I can fix the failure by using the correct separators.
However, I'll also note that this test was previously succeeding. It seems inconsistent that loading <modulePath>/...
would (presumably) work, yet loading by directory does not.
It seems inconsistent that loading /... would (presumably) work, yet loading by directory does not.
Agreed! We probably have a path-cleaning bug in cmd/go
somewhere. (Perhaps we should be calling filepath.Clean
here?)
Aha, so I have tests passing now.
I don't think the problem was the GOWORK environment variable. Rather, it appears to be necessary to use \
-separated paths in the use directives.
We have other tests for GOWORK that don't fail. The only difference is that in this case, the GOWORK value is outside the workspace directory.
Ah, right. Slash-separated use
paths should be fine as long as they are relative, but absolute use
paths mixing slashes and backslashes are... weird.
Ah, right. Slash-separated use paths should be fine as long as they are relative, but absolute use paths mixing slashes and backslashes are... weird.
You're right: the real problem is that the absolute paths use a mix of \
and /
. Strange that they ever worked...
In any case, since this is such a weird edge case that wouldn't reasonably arise in practice, I don't think it's worth considering this a Go command bug.
since this is such a weird edge case that wouldn't reasonably arise in practice
Um... I didn't say that. 😅
Thank you 😄
Change https://go.dev/cl/487136 mentions this issue: gopls/internal/lsp/source: more ITV filtering
gopls version
go env
What did you do?
Open my editor, vscode to do some work.
What did you expect to see?
A working editor
What did you see instead?
Editor and settings
Notes
go mod tidy
produces no diffgo list
command that produced this error, although I was not sure how to get the exact command. This may actually be a go list issue thoughgo mod vendor
and my editor works fine. When I do this it does not vendor the module that the editor is complains aboutcloud.google.com/go
, rightfully soVendor modules.txt