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: module dependencies added from different go.mod files of single module at different versions #35884

Closed
nmiyake opened this issue Nov 27, 2019 · 6 comments

Comments

@nmiyake
Copy link

@nmiyake nmiyake commented Nov 27, 2019

What version of Go are you using (go version)?

go version go1.13.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

Created a module import graph that has an ambiguous import that is provided by two different version of the same dependent module and attempted to build.

Specifically:

  • Create repository "github.com/nmiyake/test-repo-c" with package "smile". This models a repository that does not yet use modules.
  • Create module "github.com/nmiyake/test-repo-b" that imports "github.com/nmiyake/test-repo-c" and tag v1.0.0
    • go.mod contains require github.com/nmiyake/test-repo-c v0.0.0-20191127215535-3c89b55e2851
  • Create module "github.com/nmiyake/test-repo-d" that imports "github.com/nmiyake/test-repo-b"
    • go.mod contains require github.com/nmiyake/test-repo-b v1.0.0
  • Create module "github.com/nmiyake/test-repo-c/smile" and tag "smile/v1.0.0"
  • Update module "github.com/nmiyake/test-repo-b" to consume the "github.com/nmiyake/test-repo-c/smile" module and tag v1.1.0
    • go.mod contains require github.com/nmiyake/test-repo-c/smile v1.0.0
  • Create module "github.com/nmiyake/test-repo-a" that imports "github.com/nmiyake/test-repo-b" and "github.com/nmiyake/test-repo-d"
    • go.mod contains require github.com/nmiyake/test-repo-b v1.1.0 and require github.com/nmiyake/test-repo-d v1.0.0
  • Run go build for test-repo-a

I created this scenario and pushed them to public repositories -- cloning and attempting to build https://github.com/nmiyake/test-repo-a demonstrates the issue.

Simplified textual representation:

B@1.0.0 -> C @ 0.0.0-2019…, imports package “C/smile”
B@1.1.0 -> C/smile @ v1.0.0
D@v1.0.0 -> B@v1.0.0
A -> B@v1.1.0
A -> D@v1.0.0

B@v1.1.0 imports module “C/smile”
D@v1.0.0 imports B@v1.0.0, which imports pseudo-module “C”, which has path “smile”

Attempting to build A reports import path conflict due to dependencies provided by B@v1.1.0 and B@v1.0.0.

What did you expect to see?

I expected this to succeed.

The module graph contains both B@v1.1.0 and B@v1.0.0, and together these cause an import path conflict. However, the module specification states that only a single major version of a module will be used within a build, so this import path conflict shouldn't matter -- ultimately the build will only use one version of B, so it's confusing to me that the build can fail due to conflicts between different version of the same module.

My expectation is that, when constructing the module graph, modules that have multiple different minor versions will be version-selected and then collapsed to a single one so that dependencies provided by multiple different versions of the same module aren't considered.

What did you see instead?

Build fails due to ambiguous import path:

/Volumes/git/go/pkg/mod/github.com/nmiyake/test-repo-b@v1.1.0/bar.go:1:21: ambiguous import: found github.com/nmiyake/test-repo-c/smile in multiple modules:
	github.com/nmiyake/test-repo-c v0.0.0-20191127215535-3c89b55e2851 (/Volumes/git/go/pkg/mod/github.com/nmiyake/test-repo-c@v0.0.0-20191127215535-3c89b55e2851/smile)
	github.com/nmiyake/test-repo-c/smile v1.0.0 (/Volumes/git/go/pkg/mod/github.com/nmiyake/test-repo-c/smile@v1.0.0)

It is possible to work around this by creating an empty module at "github.com/test-repo-c" and adding a "replace" directive:

replace github.com/nmiyake/test-repo-c => github.com/nmiyake/test-repo-c v1.0.0

However, all upstream modules that consume "test-repo-a" must add this as well, and it doesn't address the underlying issue of why it's possible for a conflict to occur due to dependencies provided by the same minor/patch version of a single module.

@nmiyake

This comment has been minimized.

Copy link
Author

@nmiyake nmiyake commented Nov 27, 2019

Although the sample reproduction is contrived, I ran into this issue in a real scenario when converting repositories with a circular dependency to modules (where both repos also pointed to a common repository where a submodule was later created).

Because minor versions aren't collapsed, the circular import made it such that the module was considering both its current dependencies and the dependencies of its previous version, which was very confusing to reason about.

If the underlying issue itself won't be fixed, it would be helpful to have documentation somewhere that notes that the module dependency graph will consider dependencies of all versions of dependent modules, including all different minor/patch versions of the same module.

@nmiyake nmiyake changed the title cmd/go: module dependencies added from different go.mod files of single module cmd/go: module dependencies added from different go.mod files of single module at different versions Nov 27, 2019
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Dec 2, 2019

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Dec 2, 2019

I think the issue here is that both modules github.com/nmiyake/test-repo-c and github.com/nmiyake/test-repo-c/smile are in the build list, and they both provide the package github.com/nmiyake/test-repo-c/smile at the selected versions. If you run go list -m all, you'll see a list of all the modules and selected versions that are part of the build.

github.com/nmiyake/test-repo-a v0....
github.com/nmiyake/test-repo-b v1.0.0
github.com/nmiyake/test-repo-c v0...
github.com/nmiyake/test-repo-c/smile v1.0.0
github.com/nmiyake/test-repo-d v0....

The requirement on github.com/nmiyake/test-repo-c may be surprising, but it's pulled into the module graph by the requirement in github.com/nmiyake/test-repo-d. go mod why -m github.com/nmiyake/test-repo-c should print a trace for that.

There are a couple ways to resolve this.

You could try eliminating the requirement on github.com/nmiyake/test-repo-c from the module graph by updating the requirement in the go.mod file of test-repo-d, then tagging a new version of test-repo-d and depending on a new version of that. It's hard to do this if several modules are involved or if any of those modules are outside your control though.

You could tag a new version of github.com/nmiyake/test-repo-c and require that from test-repo-a. The smile package won't be included in that version because smile/go.mod is present, and that "carves out" packages in that directory. The new version will be used since it's higher than pseudo-versions from before github.com/nmiyake/test-repo-c/smile was created.

Hope this helps. Closing since this is working as intended, but I'm happy to answer questions.

@jayconrod jayconrod closed this Dec 2, 2019
@nmiyake

This comment has been minimized.

Copy link
Author

@nmiyake nmiyake commented Dec 2, 2019

@jayconrod thanks for chiming in -- adding github.com/nmiyake/test-repo-c to the require block is an interesting approach I hadn't thought of that could resolve the issue without a replace (although would that require creating an unused import for it like for tools to ensure it doesn't get tidied away?).

It would be helpful for me to clear up this conceptual question, though:

  • As you said, "test-repo-c v0" is pulled in by the module graph for test-repo-d due to its dependency on test-repo-b@v1.0.0
  • However, "test-repo-a" explicitly has a require for "test-repo-b@v1.1.0" (and it does NOT pull in test-repo-c)
  • Isn't it true that only one major version of a module dependency is used in a given module build? I would expect that, since v1.1.0 is newer than v1.0.0 (and these are both modules), v1.1.0 will be used for the build.
  • If that's the case, then why do the dependencies of v1.0.0 come into play? There shouldn't be any import path conflicts in the set of modules that are actually used to build the end module, right? This was my question about "collapsing" -- it looks like the current behavior is to figure out conflicts based on the full module graph rather than the one where major versions are collapsed. If the analysis was done on this collapsed graph, I don't believe there would be an import path conflict.

If that's just the way things are then that's that, but this is the portion that I would specifically like to understand. That's the part that felt like a bug/incorrect behavior on my end without further knowledge -- since my understanding was that only one major version of a module was used per build, the fact that the dependencies of multiple different versions of the same module were being used in analysis seemed strange to me and was unexpected.

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Dec 2, 2019

adding github.com/nmiyake/test-repo-c to the require block is an interesting approach I hadn't thought of that could resolve the issue without a replace (although would that require creating an unused import for it like for tools to ensure it doesn't get tidied away?).

The requirement on a newer version of github.com/nmiyake/test-repo-c can be added anywhere that ensures you don't have a conflict. You might actually add that requirement to github.com/nmiyake/test-repo-c/smile. That ensures anyone requiring github.com/nmiyake/test-repo-c/smile will not get an old conflicting version of github.com/nmiyake/test-repo-c.

This comes up mostly when a single large module is split into submodules. For example, take a look at the go.mod file for cloud.google.com/go/logging. It requires cloud.google.com/go.

Isn't it true that only one major version of a module dependency is used in a given module build? I would expect that, since v1.1.0 is newer than v1.0.0 (and these are both modules), v1.1.0 will be used for the build.

f that's the case, then why do the dependencies of v1.0.0 come into play? There shouldn't be any import path conflicts in the set of modules that are actually used to build the end module, right? This was my question about "collapsing" -- it looks like the current behavior is to figure out conflicts based on the full module graph rather than the one where major versions are collapsed. If the analysis was done on this collapsed graph, I don't believe there would be an import path conflict.

That's true when it comes to loading and building packages, but not for reading go.mod files or applying requirements. Minimal version selection, the algorithm used to select module versions, may read requirements from multiple versions of a module. https://research.swtch.com/vgo-mvs is an extended description of that algorithm.

To summarize though, think of each version of each module in a directed graph. Requirements in go.mod files are the edges. MVS traverses the graph, starting at the main module (which has no version). It then returns a list of module versions (the build list) where each version is the maximum of the versions encountered on the traversal.

The go command will update requirements in go.mod if a newer version of a module is transitively required, so traversing multiple versions doesn't come up often, but it is part of the algorithm.

(Sorry this isn't documented somewhere more obvious by the way. I'm working on improving the documentation for 1.14 (#33637)).

@nmiyake

This comment has been minimized.

Copy link
Author

@nmiyake nmiyake commented Dec 2, 2019

Thanks! Yes, I think the main source of confusion for me was that import path conflicts could happen based on dependencies introduced by different versions of the same module -- I thought that these conflicts would only matter if they existed at the build list level, but it makes sense that this would be an error even at the module dependency construction level (and thus consolidating module versions to a single one at that point doesn't prevent the error).

I believe I fully understand the contracts now, and appreciate the tip for adding the requirement to the submodules to help prevent issues for other consumers. Thanks!

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