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

cmd/go: compile and link search paths not compatible with multiple GOPATH roots #14271

Closed
rsc opened this issue Feb 9, 2016 · 9 comments
Closed

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Feb 9, 2016

Here is a day 1 design incompatibility between GOPATH and cmd/go on the one side and cmd/compile's -I option and cmd/link's -L option on the other.

When invoking the compiler or linker, cmd/go must tell it where to find compiled packages. It does so by walking the full dependency graph and collecting the names of pkg directories involved and then passes that list as repeated -I (compiler) or -L (linker) options.

GOPATH defines a preference list. GOPATH=p1:p2 (p1;p2 on Windows) means first look in p1, then p2. It is also valid for both p1 and p2 to have a package foo: p1 wins. But until the fix for #14192, it was possible for cmd/go's walk of the dependency graph to produce go tool compile -I p2 -I p1 or go tool link -L p2 -L p1, causing the compiler or linker to find p2's foo (incorrect) instead of p1's foo. CL 19385 fixes this kind of preference inversion, making sure that the -I and -L options always respect GOPATH order.

But there is a second kind of possible preference inversion that arises because cmd/go looks for source code and the compiler and linker look for compiled packages. This inversion is harder to fix.

Suppose p1 has source code p1/src/foo, that p2 does not have p2/src/foo, and that p2 does have a stale p2/pkg/goos_goarch/foo.a.

Suppose p2 has source code p2/src/bar, that p1 does not have p1/src/bar, and that p1 does have a stale p1/pkg/goos_goarch/bar.a.

Suppose there is a package top that imports both foo and bar. cmd/go will correctly resolve the imports to the source directories p1/src/foo and p2/src/bar; go install top will first build p1/src/foo into p1/pkg/goos_goarch/foo.a and p2/src/bar into p2/pkg/goos_goarch/bar.a, and then it will invoke go tool compile -I p1 -I p2 top.go. To resolve import "foo", the compiler will correctly find p1/pkg/goos_goarch/foo.a. To resolve import "bar", the compiler will incorrectly find p1/pkg/goos_goarch/bar.a: it is supposed to be using p2/pkg/goos_goarch/bar.a. And swapping the -I options does not help: -I p2 -I p1 will resolve bar correctly but not foo (and contradicts GOPATH). CL 19385 includes this scenario as a disabled test, TestGoBuildGOPATHOrderBroken.

After the fix in CL 19385, only bar is needed in the example. Forcing GOPATH order (important for more common scenarios) contradicts what would make bar resolve correctly above. Including foo just makes clear that there's no solution even if you drop the GOPATH order constraint.

The fundamental problem is that, as designed, cmd/go assumes the GOPATH source directory search and the equivalent compiled package search will land on corresponding results. In this example they do not and cannot.

I see three possible resolutions:

  1. Do nothing. It's been like this for a long time and I'm aware of no actual reports of this problem. In practice, it would only arise if people use multiple GOPATH roots and move code from an earlier root to a later root. Multiple GOPATH roots are uncommon except with godep (I think gb avoids cmd/go entirely now), and godep is moving to the new vendoring convention. In pre-vendoring godep usage, the situation would only arise when devendoring a package: removing it from the _Godep/workspace/src without also removing it from _Godep/workspace/pkg would leave a stale package file behind. Also blowing away p1/pkg fixes the problem, which people in this rare situation probably know to do.
  2. Make cmd/go detect this situation and delete the stale compiled package bar.a. Specifically, the situation is having multiple GOPATH roots p1:p2:...:pN, where zzz resolves to pK/src/zzz and cmd/go wants to invoke the compiler with -I pJ -I pK, for some J < K. In this case, cmd/go would first have to delete pJ/pkg/goos_goarch/zzz.a, if it existed.
  3. Replace the -I and -L options in the compiler and linker with more fine-grained options -Z importpath=/path/to/pkg.a. This would result in O(# of imports) arguments to the compiler and O(total # of packages) arguments to the linker, instead of the current O(# of GOPATH roots), which might suggest passing the map in a file instead of on the command line.

(1) is not terribly satisfying. (2) seems reasonable behavior by cmd/go if viewed as “detecting a removed source directory and removing the corresponding package object.” (3) seems like unwarranted complexity.

On the other hand, if we ever turn the pkg/ tree into some kind of general package cache (see #4719), it will necessarily lose the current alignment with import paths and thus require something like (3). I also wonder whether alternate build systems like gb or bazel would be helped by having (3) available (/cc @davecheney, @hanwen). For example, my compiler support for vendoring was to add -importmap, a much more general compiler option, both because it kept vendor logic out of the compiler and because I expect that eventually bazel in large source repositories (like Google's) will need something like this to support multistep moves of Go packages from one location to another. In that case, -importmap is there for vendoring today and there for a need I know will arise in the future (because I've already seen the need come up and be worked around). If the authors of gb or bazel or some other build system made a case for why (3) would help them, I'd be more inclined to do it.

With the information I have right now, I'm inclined toward doing (2). In the common case of ≤ 1 GOPATH root, it would be no extra work, and when there are multiple GOPATH roots there are still no more removals of compiled packages than there were failed stats of source directories.

@rsc rsc self-assigned this Feb 9, 2016
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 9, 2016

CL https://golang.org/cl/19385 mentions this issue.

@hanwen

This comment has been minimized.

Copy link
Contributor

@hanwen hanwen commented Feb 9, 2016

Bazel sidesteps the Go tool, and manages its own caches, so this scenario isn't a problem for Bazel. Since all the compiles happen in a symlink tree (on Linux it additionally uses a namespace sandbox), there is no possibility of confusion or need of -L options. In fact, it just sets "-L . ":

https://github.com/bazelbuild/bazel/blob/master/tools/build_rules/go/def.bzl#L245

If you added a -importmap option to the linker, the rules could probably be adapted for that, but it doesn't bring any benefit, just a little extra work.

It's probably too late to change this, but why not have GOPATH just apply to source directories? The artifacts would go then into a pkg/PREFIX/ directory, where you can work all relevant settings (race detector, $GOPATH, target architecture, compiler version, etc.) into PREFIX.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Feb 9, 2016

If you added a -importmap option to the linker, the rules could probably be adapted for that, but it doesn't bring any benefit, just a little extra work.

To be clear, -importmap is about something completely different from this
issue (allowing import "x" and import "y" to resolve to the same package;
symlinks don't help, because you'd still get two copies in the final
binary).

It's probably too late to change this, but why not have GOPATH just apply to source directories? The artifacts would go then into a pkg/PREFIX/ directory, where you can work all relevant settings (race detector, $GOPATH, target architecture, compiler version, etc.) into PREFIX.

The original motivation for multiple GOPATH entries was to allow
GOPATH=$HOME/mygo:/usr/local/systemgo. You can't just have one pkg/
directory in that case: the user needs write access to update the packages
for $HOME/mygo's sources, but you want the systemgo packages read-only.

Russ

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Feb 9, 2016

I don't think (3) sounds too complicated. Plus I would love to see work towards #4719 in 1.7. (I don't like having to explain to people when to run "go install or "go test -i" occasionally, to make their "go builds" fast again.)

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Feb 9, 2016

gopherbot pushed a commit that referenced this issue Feb 9, 2016
Given GOPATH=p1:p2 and source code of just the right form,
the go command could previously end up invoking the compiler
with -I p2 -I p1 or the linker with -L p2 -L p1, so that
compiled packages in p2 incorrectly shadowed packages in p1.
If foo were in both p1 and p2 and the compilation of bar
were such that the -I and -L options were inverted in this way,
then

	GOPATH=p2 go install foo
	GOPATH=p1:p2 go install bar

would get the p2 copy of foo instead of the (expected) p1 copy of foo.

This manifested in real usage in a few different ways, but in all
the root cause was that the -I or -L option sequence did not
match GOPATH.

Make it match GOPATH.

Fixes #14176 (second report).
Fixes #14192.
Related but less common issue #14271 not fixed.

Change-Id: I9c0f69042bb2bf92c9fc370535da2c60a1187d30
Reviewed-on: https://go-review.googlesource.com/19385
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 5, 2017

CL https://golang.org/cl/44850 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 5, 2017

CL https://golang.org/cl/44851 mentions this issue.

gopherbot pushed a commit that referenced this issue Jun 6, 2017
Allows reading -importmap options from a file instead of putting
them all on the command line, and adds the ability to specify the
file location of specific packages. In effect, -importcfg is a generalization
of and supersedes -importmap, -importsuffix, and -I.
Of course, those flags will continue to be supported,
for compatibility with other tools.

Having this flag in Go 1.9 will let us try some experiments involving
package management without needing guinea pigs to build a
custom Go toolchain.

This flag also helps with #14271 at some later point.

For #20579.

Change-Id: If005dbc2b01d8fd16cbfd3687dfbe82499f4bc56
Reviewed-on: https://go-review.googlesource.com/44850
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Jun 6, 2017
Adds the ability to specify the file location of each imported package,
like in the -importcfg added to cmd/compile in a related CL.
In effect, -importcfg is a generalization of and supersedes -installsuffix
and -L. Of course, those flags will continue to be supported, for
compatibility with other tools.

Having this flag in Go 1.9 will let us try some experiments involving
package management without needing guinea pigs to build a custom
Go toolchain.

This flag also helps with #14271 at some later point.

For #20579.

Change-Id: Ie4c171bcd3aa2faa446ac340e36516f2f9853882
Reviewed-on: https://go-review.googlesource.com/44851
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 16, 2017

Change https://golang.org/cl/56279 mentions this issue: cmd/go: use -importcfg to invoke compiler, linker

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 5, 2017

Change https://golang.org/cl/61731 mentions this issue: cmd/go: add gccgo support for recent work

@gopherbot gopherbot closed this in 0be2d52 Sep 29, 2017
gopherbot pushed a commit that referenced this issue Sep 29, 2017
Implement importcfg on behalf of gccgo by writing out a
tree of symbolic links. In addition to keeping gccgo working
with the latest changes, this also fixes a precedence bug in
gccgo's cmd/go vendor support (the vendor equivalent of #14271).

Change-Id: I0e5645116e1c84c957936baf22e3126ba6b0d46e
Reviewed-on: https://go-review.googlesource.com/61731
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@golang golang locked and limited conversation to collaborators Sep 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.