go/build: Import does not find source directory for code in modules #26504
Comments
bcmills
added
modules
NeedsFix
|
Maybe this is a silly question, but why do we need to shell out to the Are tools invoking |
|
Change https://golang.org/cl/125296 mentions this issue: |
A very real problem we've had with go/build is that tools built with one Go version are used with another Go version. Concretely, if I It matters a lot that there's one source of truth.
If we must, we could think about using -deps and adding a cache. But I don't think we must - I think most things will run plenty fast enough. And there are two significant drawbacks to using -deps and adding a cache: -deps and the cache. First, -deps: Go 1.10 and earlier did not have -deps, so a Go 1.11-compiled mage or golint will fail if used with a Go 1.10 or earlier go command. We really do want tools that work with multiple releases. We may issue a point release for Go 1.10 with more flags in go list, for use by golang.org/x/tools/go/packages, but if we can avoid making go/build depend on that hypothetical point release, it's better not to. Second, the cache: for a one-shot program analysis, yes, the cache would make sense. But in general go/build.Import returns information about the current file system, so there are likely to be long-running programs that occasionally load packages, do things, and throw them away. Those need to not see stale results. The API has no cache invalidation because it has never had a cache. It does not seem prudent to add one at this point. Programs that are too slow with the modified go/build should almost certainly update to golang.org/x/tools/go/packages instead of us adding a cache to go/build. |
Thank you! I was really worried about that. |
As another data point, some time ago I created the very much temporary https://github.com/myitcv/hybridimporter:
My experience with This isn't per se an argument in favour of caching; just a note of what I observed. I don't know what the "best" answer for |
If you mean |
|
How about modifying dirname of go modules? For example, From to Then we could just append |
|
Change https://golang.org/cl/183991 mentions this issue: |
The current cmd/doc implementation uses go/build.Import in a few places to check whether a package is findable and importable. go/build has limited support for finding packages in modules, but to do so, build.Import requires knowing the source directory to use when performing the lookup (so it can find the go.mod file). Otherwise, it only looks inside the GOPATH workspace. Start passing the current working directory to build.Import calls, so that it can correctly look for packages in modules when in cmd/doc is executed in module mode. Before this change, cmd/doc in module mode could mistakenly find and use a package in the GOPATH workspace, instead of the current module. Since the result of os.Getwd is needed in even more places, assign it to a local variable in parseArgs now. Fixes #28992 Updates #26504 Change-Id: I7571618e18420d2d3b3890cc69ade2d97b1962bf Reviewed-on: https://go-review.googlesource.com/c/go/+/183991 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
|
Change https://golang.org/cl/218817 mentions this issue: |
This is a followup to CL 199840 and CL 203820. Cumulatively, they caused
a previously known bug to trigger more often while also nearly fixing it.
This change is a small fixup to CL 199840 that resolves the known bug
and prevents it from causing an additional regression in Go 1.14.
Part 1
The intention in CL 199840 was to return the same error that 'go list'
reported when the package wasn't located, so an early return was added.
However, to determine whether the package was located or not, p.Dir was
unintentionally checked instead of dir.
p is initialized to &Package{ImportPath: path} at top of Context.Import,
and its Dir field is never set before that line in importGo is reached.
So return errors.New(errStr) was always executed whenever errStr != "".
Originally, in CL 125296, the "go list" invocation did not include an
'-e' flag, so it would return a non-zero exit code on packages where
build constraints exclude all Go files, and importGo would return an
error like "go/build: importGo import/path: unexpected output: ...".
CL 199840 added an '-e' flag to the "go list" invocation, but checking
the wrong dir variable caused partial package information to never get
populated, and thus issue #31603 continued to occur, although with a
different error message (which ironically included the location of the
package that was supposedly "not found").
Now that the right dir is checked, issue #31603 is fixed.
Part 2
importGo checks whether it can use the go command to find the directory
of a package. In Go 1.13.x and earlier, one of the conditions to use the
go command was that the source directory must be provided.
CL 203820 made a change such that knowing the source directory was
no longer required:
// To invoke the go command,
-// we must know the source directory,
// ...
That meant build.Import invocations where srcDir is the empty string:
build.Import(path, "", build.FindOnly)
Started using the go command to find the directory of the package, and
started to run into issue #31603 as well. That's the #37153 regression.
Since this change fixes issue #31603, it also fixes issue #37153.
Part 3
There is one more thing. Delete the debugImportGo constant, it's unused.
Updates #26504 (CL 125296)
Updates #34752 (CL 199840)
Updates #34860 (CL 203820)
Fixes #31603
Fixes #37153
Change-Id: Iaa7dcc45ba0f708a978950c75fa4c836b87006f4
Reviewed-on: https://go-review.googlesource.com/c/go/+/218817
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
golang/go#26504 using build.Import is not reliable ... TODO: must compile resources into go binary
go/build's Import and Context.Import can't find import paths in modules. We've been assuming this was unfixable and that people should move to golang.org/x/tools/go/packages. But I think we can and should fix this for Go 1.11.
I've been thinking about the problem wrong. I assumed people were using the export data from GOPATH/pkg, because that would be the only way to handle large programs efficiently. And I assumed that we really needed to get all the information out of the go command in one shot, instead of asking for every package in sequence. But it sounds like most tools actually just use the source importer, slow as that may be in general, since they are working with relatively small code bases.
It occurred to me this morning that we can keep the source importer working as long as we make package go/build's Context.Import invoke the go command to ask what directory to look in, for each package.
Using the go command this way will have a pseudo-quadratic behavior where you ask the go command about the top-level package, it loads everything it needs to know about that package (including info about transitive dependencies), and then reports only the directory of the top-level package. Then this repeats for each dependency, which in turn reloads all the information about the dependency's dependencies, hence the pseudo-quadratic (depending on the shape of the import graph). But if people are OK with the source importer, then that may be a signal that the package graph is small enough not to worry about this. And in any event, "a little slow" is certainly better than "not working at all", so it's a better stopgap for tools that aren't ready to convert to golang.org/x/tools/go/packages.
We can limit the go command invocation to happen only when:
Those limitations should mean that invoking the go command only happens when is really needed, so that existing uses will be completely unaffected and still use the non-go-command code paths. The standard library constraint is not necessary for correctness but avoids unnecessary invocations to ask about the standard library, which can still be found by the old code.
In the long term we still want to move people to golang.org/x/tools/go/packages, but this should provide a much smoother transition.
Would fix most of #24661, magefile/mage#124, and probably many other issues, at least provisionally.
The text was updated successfully, but these errors were encountered: