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: ensure that 'go mod tidy' and go get -u do not introduce ambiguous imports #27899

Open
bcmills opened this issue Sep 27, 2018 · 16 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 27, 2018

As noted in #27858 (comment), when a package is moved between two modules that share one of the complete module paths as a prefix, each needs to require some baseline version of the other, for two reasons:

  1. To ensure that go get -u doesn't drop any needed packages.

  2. To ensure that any third module that depends on both of the common-prefix modules will always end up in a configuration that provides only one copy of each package.

It would be really unfortunate if moving a package meant that you could never run go mod tidy the involved modules again, or that you have to apply some manual edit every time you run it.

We should either modify go mod tidy to avoid dropping requirements that are involved in a cycle with the main module, or add some explicit annotation comment to tell it not to drop an apparently-unnecessary requirement.

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. modules labels Sep 27, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 27, 2018
@hyangah
Copy link
Contributor

hyangah commented Oct 12, 2018

If two modules don't have actual dependency but require is needed for the second reason, there is no cycle to detect. Explicit annotation from the submodule (maybe #keep) seems easier.

@bcmills
Copy link
Contributor Author

bcmills commented Oct 15, 2018

@hyangah, I'm not sure what you mean? If you're changing module boundaries, there should always be a cycle. (Reason # 1 imposes a requirement edge in one direction, while # 2 imposes the other direction: I can't think of a situation where we would need to preserve the edge but not have a cycle.)

@hyangah
Copy link
Contributor

hyangah commented Oct 15, 2018

@bcmills my comment is about how to change go mod tidy to detect the (implicit) cycle automatically and prevents dropping the requirement (or even better, adding the implicit requirement automatically), not about the existence of such implicit cycle. The implicit cycle is not encoded in the import graph from .go files so the go mod tidy should go beyond the latest source code analysis. Moreover, the required version is the one that introduces the new module boundary, which is not automatically known yet until the commit to add the new boundary is checked in.

@thepudds
Copy link
Contributor

As noted in #27858 (comment), when a package is moved between a module and one of is submodules, the module and submodule each need to require some baseline version of the other
...
We should either modify go mod tidy to avoid dropping requirements that are involved in a cycle with the main module, or add some explicit annotation comment to tell it not to drop an apparently-unnecessary requirement.

Sorry if this is just noise, and I wouldn't suggest this as a long term solution, but adding a comment here in case it triggers some discussion and/or a better idea.

To avoid go mod tidy removing cyclic dependencies that were put in place to allow a module to be broken apart by moving packages between modules, would it work in 1.11 to do something like add a set of modmove.go files or similar that contain import statements describing the required cyclic dependencies such that a later go mod tidy does not remove the require directives?

I have not tested this on an actual repo, but if you wanted to take a module example.com/my/module/ and break out example.com/my/module/foo into a separate module with its own go.mod, then after manually added the necessary cyclic require statements to the go.mod files, you could then encode those dependencies in a set of new modmove.go file, via something like:

In the root of the repo:

# 1. create a modmove.go for the parent module:

mkdir modmove
cat <<EOF > modmove/modmove.go
// +build modmove

package modmove

import (
        _ "example.com/my/module/foo"
)
EOF

# 2. create a modmove.go for the nested module foo:

mkdir foo/modmove
cat <<EOF > foo/modmove/modmove.go
// +build modmove

package modmove

import (
        _ "example.com/my/module"
)
EOF

In theory, a later go mod tidy would not remove the manually added cyclic require statements.

This technique would be somewhat analogous to the tools.go approach for tool dependencies as described in #25922 (comment), but applied in a different way against a different problem.

(Side note: example above creates modmove subdirectories. That might not be required given the modmove build tag?)

@bcmills
Copy link
Contributor Author

bcmills commented Oct 24, 2018

@thepudds, that's a nice workaround, but it's slightly fragile: if the package that we put in the import line itself moves into or out of some other module with the same prefix, we could end up accidentally creating a cycle between the nested modules rather than the intended cycle with the parent module.

@thepudds
Copy link
Contributor

thepudds commented Oct 24, 2018

@bcmills, agreed -- it would take some care to set up initially, and then take some care to not mess up later if there are additional moves.

Hopefully the core go tooling has some additional logic targeting these types of scenarios in 1.12 or so such that something like this is no longer needed.

That said, I was trying to think if there was something that could work in 1.11 (aside from "never run go mod tidy again”, or needing to repeatedly re-apply a manual edit). Perhaps modmoved.go or similar could be viewed as a candidate temporary workaround for 1.11 if someone needs to split a module in 1.11.

@bcmills bcmills modified the milestones: Go1.12, Go1.13 Oct 25, 2018
@rsc
Copy link
Contributor

rsc commented Oct 25, 2018

I don't think we can a priori assume that every cycle should be kept.
If I'm working on A and I know B uses A, I might run 'go get B@master; go test B/...'. That will create a cycle but one that I probably want 'go mod tidy' to remove.

@bcmills
Copy link
Contributor Author

bcmills commented Nov 16, 2018

In https://golang.org/issue/27858#issuecomment-434713751, @jba suggests:

After a go get -u, the tool should see if any packages required by the main module are no longer in a module, and fetch those packages' modules in the usual way.

That would allow for a single require edge (to remove the duplicate copy) rather than a cycle.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 17, 2019

(CC @jayconrod)

@jba's suggestion solves the “missing package when moving out” direction (1), but not the “ambiguous import when moving in” direction (2).

But perhaps we could resolve that similarly: if we detect an ambiguous import, before we emit an error we could try to upgrade away all of the otherwise-unconstrained modules providing that package.

For example, if we execute

$ go get -m example.com/a/b@v1.5.0 other.example.com@v1.2.0

and we detect that, as a result, package example.com/a/b/c is provided by three modules (example.com/a, example.com/a/b, and example.com/a/b/c), then we could treat that as equivalent to

go get -m example.com/a/b@v1.5.0 other.example.com@v1.2.0 example.com/a@latest example.com/a/b/c@latest

@bcmills
Copy link
Contributor Author

bcmills commented Jan 17, 2019

(Note that an N-way collision is possible if N different modules are passed to go get, since they can each have an indirect requirement on a distinct module that provides the package.)

@bcmills bcmills changed the title cmd/go: do not remove cyclic requirements in 'go mod tidy' cmd/go: ensure that 'go mod tidy' does not introduce import collisions Jan 17, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Jan 17, 2019

To summarize, the options so far are:

  1. Change go mod tidy to preserve otherwise-unneeded requirements if one module is a prefix of the other.
  2. Change go get to try to resolve import collisions implicitly by upgrading the modules containing the colliding package(s).
  3. Require that maintainers annotate collisions using .go files with +build constraints so that go mod tidy won't remove the requirements.

@bcmills bcmills added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jan 17, 2019
@jayconrod
Copy link
Contributor

It seems like the issue is that we have version requirements that don't match import requirements, and go mod tidy only knows about import requirements.

There are other cases like this, too, right? From #25922, we need this for tool dependencies. Also, a more mundane examples: "I call a function in module A that returns a struct type defined in module B. I don't import packages from B directly, but I need a field added in B v1.2, and A only depends on v1.1".

I don't think go mod tidy or go get -u will be able to figure out when to remove and upgrade these in all cases. Even if they could, I don't think we should make them more magical: it's already hard for users to predict what they'll do. My preference would be for explicit annotation.

I like @hyangah's suggestion of having a // keep comment prevent tools from removing requirements, since the reasoning can be right there in the go.mod file. @thepudds suggestion of adding a tagged-out .go file with the necessary imports is logically equivalent and works without any new logic, but I think it's a little confusing to leave as an official long-term solution.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 22, 2019

From #25922, we need this for tool dependencies.

That case is a bit different: import collisions are, at least in principle, easy to detect, whereas tool dependencies are not.

“[…] I need a field added in B v1.2, and A only depends on v1.1"

In that case, go mod tidy would already preserve the dependency (with an // indirect comment). It only prunes out dependencies that do not provide any package in the transitive import graph.

@bcmills bcmills changed the title cmd/go: ensure that 'go mod tidy' does not introduce import collisions cmd/go: ensure that 'go mod tidy' and go get -u do not introduce ambiguous imports Jan 30, 2019
@bcmills bcmills modified the milestones: Go1.13, Go1.14 Apr 19, 2019
eliben added a commit to google/go-cloud that referenced this issue May 13, 2019
…#2057)

Updates #2048 - moves the go.mod file from samples/appengine to samples.

Note the replace gocloud.dev => ../ added to the go.mod. Without this, running go test ./... in samples/ results in many errors like:

can't load package: package gocloud.dev/samples/server: unknown import path "gocloud.dev/samples/server": ambiguous import: found gocloud.dev/samples/server in multiple modules:
	gocloud.dev/samples (/home/eliben/eli/go-cloud/samples/server)
	gocloud.dev v0.13.0 (/home/eliben/eli/go/pkg/mod/gocloud.dev@v0.13.0/samples/server)

The symptom and solution is explained by bcmills in ugorji/go#279 (which refers also to golang/go#27899). The new go.mod points to gocloud.dev v0.13, which also provides these packages - so the go command is confused - it sees the same package(s) provided by two different modules.

The ugorji/go solution was to use a pseudo-version pointing at an existing commit in the core module which removes the packages - this removes the ambiguity. In our case, there is no existing commit yet - so I'm using a replace line. The replace line should be unnecessary when we release a new CDK version.

This has interesting implications for #886 - we'll likely have to do the same when we split out providers to their own modules and retain replace lines until a new release.
@gopherbot
Copy link

Change https://golang.org/cl/196298 mentions this issue: cmd/go/internal/modload: use a structured error for 'ambiguous import'

gopherbot pushed a commit that referenced this issue Sep 19, 2019
This consolidates the construction of 'ambiguous import' errors to a
single location, ensuring consistency, and lays the groundwork for
automatic resolution in the future.

While we're at it, change "found" to "found package" to try to make
the cause of the error clearer.

Updates #32128
Updates #27899

Change-Id: I14a93593320e5c60d20b0eb686d0d5355763c30c
Reviewed-on: https://go-review.googlesource.com/c/go/+/196298
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Oct 14, 2020

perhaps we could resolve that similarly: if we detect an ambiguous import, before we emit an error we could try to upgrade away all of the otherwise-unconstrained modules providing that package.

This approach works for upgrades, but not downgrades.

Suppose that we have:

  • module example.com
    • at version v0.1.0, containing package example.com/foo
    • and at version v0.3.0, with that package removed.
  • module example.com/foo
    • without any version v0.1.0
    • and at version v0.2.0, containing package example.com/foo

If we start out by running go get example.com/foo@v0.1.0, then we must end up with

require example.com v0.1.0

because that is the only v0.1.0 providing the package.

If we then run go get example.com/foo@v0.2.0, then we can apply the “upgrade collisions away” rule to obtain

require (
	example.com v0.3.0
	example.com/foo v0.2.0
)

But now suppose that we want to get back to our initial state. If we run go get example.com/foo@v0.1.0 and only resolve conflicts upward, we will end up with the configuration

require (
	example.com v0.1.0
	example.com/foo v0.2.0
)

and an ambiguous import.

The user could perhaps resolve that by running go get example.com/foo@none && go get example.com/foo@v0.1.0, but that solution is not obvious — especially given that go get example.com/foo@v0.1.0 worked successfully the first time they ran it (in step 1).

@bcmills
Copy link
Contributor Author

bcmills commented Oct 14, 2020

One general approach might be:

  • If no module providing the package is being downgraded, try to upgrade away the collision(s).
  • If all modules providing the package are either being downgraded or remain at their previously-selected versions, try to downgrade away the collision(s).
  • If the modules providing the package include a mix of upgrades and downgrades, error out. (This condition should be exceedingly rare.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants