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 · 13 comments

Comments

Projects
None yet
5 participants
@bcmills
Copy link
Member

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 this to the Go1.12 milestone Sep 27, 2018

@hyangah

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

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

This comment has been minimized.

Copy link

thepudds commented Oct 24, 2018

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

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link
Member 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.
@jayconrod

This comment has been minimized.

Copy link
Contributor

jayconrod commented Jan 18, 2019

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

This comment has been minimized.

Copy link
Member 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.