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

go/build: Import does not find source directory for code in modules #26504

Closed
rsc opened this issue Jul 20, 2018 · 9 comments
Closed

go/build: Import does not find source directory for code in modules #26504

rsc opened this issue Jul 20, 2018 · 9 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Jul 20, 2018

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:

  • the AllowBinary and IgnoreVendor flags are not set,
  • Context describes the local file system (all the helper functions are nil),
  • srcDir is outside GOPATH/src (or GO111MODULE=on),
  • there's a go.mod in srcDir or above,
  • the release tags are unmodified from Default.Context,
  • and path does not name a standard library package.

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.

@rsc rsc added this to the Go1.11 milestone Jul 20, 2018
@rsc rsc self-assigned this Jul 20, 2018
@bcmills bcmills added modules NeedsFix The path to resolution is known, but the work has not been done. labels Jul 20, 2018
@bcmills
Copy link
Contributor

bcmills commented Jul 20, 2018

Maybe this is a silly question, but why do we need to shell out to the go command at all? (Can we move modload and its dependencies someplace where go/build can import them?)

Are tools invoking Import multiple times from the same binary? If so, it seems like we could avoid at least some of the quadratic behavior by using the -deps flag and caching all of the packages it returns in an in-process (and per-srcDir) cache.

@gopherbot
Copy link

Change https://golang.org/cl/125296 mentions this issue: go/build: invoke go command to find modules during Import, Context.Import

@rsc
Copy link
Contributor Author

rsc commented Jul 20, 2018

Maybe this is a silly question, but why do we need to shell out to the go command at all? (Can we move modload and its dependencies someplace where go/build can import them?)'

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 go get github.com/magefile/mage, we want that one binary to work no matter which version of Go I am using. I should be able to flip back and forth between, say, Go 1.10 and Go 1.11beta2, without having to maintain "mage for Go 1.10" vs "mage for Go 1.11beta2". Same for golint, etc. The fundamental mistake here is to allow tools to have a copy of the logic for where you get source code from. The last source layout change was the introduction of vendor directories and we ran into the same problem. There should be one canonical implementation - inside the go command. Other tools should invoke the go command to access this implementation. If you update to a new Go distribution, you get a new go command, and all the tools that invoke it automatically adapt to whatever has changed.

It matters a lot that there's one source of truth.

Are tools invoking Import multiple times from the same binary? If so, it seems like we could avoid at least some of the quadratic behavior by using the -deps flag and caching all of the packages it returns in an in-process (and per-srcDir) cache.

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.

@natefinch
Copy link
Contributor

without having to maintain "mage for Go 1.10" vs "mage for Go 1.11beta2"

Thank you! I was really worried about that.

@myitcv
Copy link
Member

myitcv commented Jul 23, 2018

If so, it seems like we could avoid at least some of the quadratic behavior by using the -deps flag and caching all of the packages it returns in an in-process (and per-srcDir) cache.

As another data point, some time ago I created the very much temporary https://github.com/myitcv/hybridimporter:

myitcv.io/hybridimporter is an implementation of go/types.ImporterFrom that uses non-stale package dependency targets where they exist, else falls back to a source-file based importer.

My experience with hybridimporter is that having even moderately complex import graphs pushes you rapidly towards wanting to cache the result of go list -deps. I take Russ' point about long running programs and stale data, but I'm not aware that go/packages has a "solution" for that either (https://go-review.googlesource.com/c/tools/+/116359/1/go/packages/doc.go#51) as yet.

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 go/build is.

@mvdan
Copy link
Member

mvdan commented Jul 23, 2018

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.

If you mean go/importer.For("source", nil), I think it has had more to do about correctness than size of input. For example, go/importer.For("gc", nil) has historically been a source of confusion for users if they forgot to go install the packages first.

@morlay
Copy link

morlay commented Jul 23, 2018

How about modifying dirname of go modules?

For example, $GOMOD_ROOT=$GOPATH/src/mod

From

$GOMOD_ROOT/
       golang.org/x/tools@v0.0.0-xxx/
           xxx.go

to

$GOMOD_ROOT/
       golang.org/x/tools@v0.0.0-xxx/
            src/golang.org/x/tools/go/loader/
                  xxx.go
            src/golang.org/x/tools/go/loader/v1/   # or version alias
                  xxx.go

Then we could just append GOPATHs by list of go.mod when GO111MODULE=on.
like GOPATH=$GOPATH:$GOMODROOT/golang.org/x/tools@v0.0.0-xxx.

@gopherbot
Copy link

Change https://golang.org/cl/183991 mentions this issue: cmd/doc: provide working directory to build.Import calls

gopherbot pushed a commit that referenced this issue Jun 28, 2019
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>
@gopherbot
Copy link

Change https://golang.org/cl/218817 mentions this issue: go/build: populate partial package information in importGo

gopherbot pushed a commit that referenced this issue Feb 12, 2020
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>
einthusan added a commit to paperboard/example that referenced this issue Jan 11, 2021
golang/go#26504
using build.Import is not reliable ...
TODO: must compile resources into go binary
@golang golang locked and limited conversation to collaborators Feb 9, 2021
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants