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: go mod init doesn't import nested module, tidy picks older version #33033

Open
rogpeppe opened this issue Jul 10, 2019 · 16 comments
Open
Labels
modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@rogpeppe
Copy link
Contributor

In this case, the glide.lock file depends on a version of github.com/hashicorp/consul that uses modules and submodules. Before go mod tidy, we are depending on v1.5.1, a commit with date 2019-05-22. After go mod tidy, the dependency has regressed to the latest available api submodule version, v1.1.0, a commit with date 2019-05-08.

This is a dependency regression which could potentially have broken code relying on new features added between the two commits, something that go mod tidy shouldn't be able to do.

I suspect that go mod tidy needs to use a pseudoversion commit in this case, perhaps github.com/hashicorp/consul/api v1.1.1-0.20190522201912-40cec98468b8.

% go version
go version devel +a05c132064 Wed Jul 10 15:52:04 2019 +0000 linux/amd64
% ls
glide.lock  main.go
% cat glide.lock
hash: 0a6384395a31012cdcb431685f7cbe2ab3e4fb82412f708c491a785002881ed0
updated: 2019-06-05T16:36:27.768346055+02:00
imports:
- name: github.com/hashicorp/consul
  version: 40cec98468b829e5cdaacb0629b3e23a028db688
  subpackages:
  - api
% cat main.go
package main
import _ "github.com/hashicorp/consul/api"

func main() {
}
% go mod init m
go: creating new go.mod: module m
go: copying requirements from glide.lock
% cat go.mod
module m

go 1.13

require github.com/hashicorp/consul v1.5.1
% go mod tidy
% cat go.mod
module m

go 1.13

require (
	github.com/hashicorp/consul/api v1.1.0
	github.com/hashicorp/go-msgpack v0.5.4 // indirect
	github.com/hashicorp/memberlist v0.1.4 // indirect
	golang.org/x/crypto v0.0.0-20190325154230-a5d413f7728c // indirect
	golang.org/x/net v0.0.0-20190403144856-b630fd6fe46b // indirect
	golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6 // indirect
	golang.org/x/sys v0.0.0-20190403152447-81d4e9dc473e // indirect
)
@av86743
Copy link

av86743 commented Jul 10, 2019

@rogpeppe

github.com/hashicorp/consul/api v1.1.1-0.20190522201912-40cec98468b8

.../api v1.5.1-... ?

@rogpeppe
Copy link
Contributor Author

@av86743 I don't think so, because the latest version of github.com/hashicorp/consul/api is v.1.1.0.
If the pseudo-version used v.1.5.1-... and a new release of github.com/hashicorp/consul/api was tagged, say v1.1.1, the new release would be considered older than the pseudo-version when it should actually be considered newer.

@rogpeppe
Copy link
Contributor Author

@bcmills @jayconrod

@av86743
Copy link

av86743 commented Jul 11, 2019

@av86743 I don't think so, because the latest version of github.com/hashicorp/consul/api is v.1.1.0.
If the pseudo-version used v.1.5.1-... and a new release of github.com/hashicorp/consul/api was tagged, say v1.1.1, the new release would be considered older than the pseudo-version when it should actually be considered newer.

@rogpeppe

I do not see logic of what you are saying.

.../consul/api is renamed as local subdirectory here in go.mod and its required version is 1.1.0 here in go.mod. However, being a subdirectory, consul/api does not have versioning on its own; it uses same versioning as enclosing consul repo. Which already has tag v1.1.1 - which has date and commit hash different from what you have specified.

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Jul 11, 2019

@av86743

However, being a subdirectory, consul/api does not have versioning on its own

I think you're missing the important fact that github.com/hashicorp/consul/api is a submodule of github.com/hashicorp/consul - the go.mod file is here - and thus has its own independent versions.

In fact, there are three submodules (api, internal and sdk) and each has its own set of tags:

% git tag | grep /
api/v1.0.0
api/v1.0.1
api/v1.1.0
internal/v0.1.0
sdk/v0.1.0
sdk/v0.1.1

@av86743
Copy link

av86743 commented Jul 11, 2019

@av86743

However, being a subdirectory, consul/api does not have versioning on its own

I think you're missing the important fact that github.com/hashicorp/consul/api is a submodule of github.com/hashicorp/consul - the go.mod file is here - and thus has its own independent versions.

@rogpeppe

My mistake. Thanks for explaining this to me. Conveniently, github viewer does not indicate submodules visually, and their versions are hiding on the bottom of the tag list.

I did see consul/api/go.mod, however its presence does not imply that consul/api has versioning of its own (via submodules, as you have explained.)

I do not see any references in description of Modules neither to git submodules nor to special way of treating git submodule versions. Should the latter be obvious? Are there any pointers at all (except the source code) which would explicitly resolve ambiguity of v1.1.0 in this case?

        github.com/hashicorp/consul/api v1.1.0

PS I have looked at git submodules and realized that .gitmodules for the project in question does not exist. That is, your submodules are not git submodules, but simply nested go modules. From where I infer that method of go versioning using git tags like${submodule_root}/vX.Y.Z must be entirely conventional; and not described anywhere either - or is it?

@av86743
Copy link

av86743 commented Jul 11, 2019

@rogpeppe

I apologize for the noise - format of tags for nested modules is given in the description of the Modules.

Admittedly, absence of any indication whether version belongs to the root module or submodule, does not make inspection of module dependencies any easier.

As for your example, you probably do not want to use future exact version, which go mod will likely mark later as erroneous, and use semver wildcard instead:

github.com/hashicorp/consul/api v1.1.x-0.20190522201912-40cec98468b8

@jayconrod
Copy link
Contributor

This seems like a problem with importing from glide.lock, rather than go mod tidy.

Before you run go mod tidy, only github.com/hashicorp/consul is required. github.com/hashicorp/consul/api is imported, but not required. Any build command will add a requirement on the latest version, which is is v1.1.0.

$ cat go.mod
module m

go 1.13

require github.com/hashicorp/consul v1.5.1

$ go list .
m

$ cat go.mod
module m

go 1.13

require (
	github.com/hashicorp/consul v1.5.1
	github.com/hashicorp/consul/api v1.1.0
)

go mod tidy will add the requirement on github.com/hashicorp/consul/api, but it will also remove any requirements on modules that aren't transitively imported, so github.com/hashicorp/consul is removed.

When go mod init imports from another package manager, it does a pretty simple translation. I don't think there's any package manager that supports Go nested modules, so the glide.lock file will just say the repository is required at tag v1.5.2, including everything in the api subdirectory.

Perhaps we can do something more sophisticated here. We could walk the import graph, figure out what version or commit each package should have been required at, then try to reverse-engineer a go.mod file that produces the same build list. This is close to what go get does now, so it's not infeasible (though I'm sure there are cases where MVS can't produce the same result), but it's a bit of work.

@jayconrod jayconrod changed the title cmd/go: go mod tidy can regress a dependency when it has a submodule cmd/go: go mod init doesn't import submodule, tidy picks older version Jul 11, 2019
@jayconrod jayconrod added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 11, 2019
@jayconrod jayconrod added this to the Go1.14 milestone Jul 11, 2019
@bcmills bcmills changed the title cmd/go: go mod init doesn't import submodule, tidy picks older version cmd/go: go mod init doesn't import nested module, tidy picks older version Jul 11, 2019
@bcmills
Copy link
Contributor

bcmills commented Jul 11, 2019

Perhaps we can do something more sophisticated here. We could walk the import graph, figure out what version or commit each package should have been required at, then try to reverse-engineer a go.mod file that produces the same build list.

In general, the module configuration converted from another dependency manager will often require adjustment anyway: for example, some dependency managers have operated at the package (rather than repo) granularity, and migrating those to modules ends up bumping the module version for all of those packages upward to the package with the highest requirement, which can end up pulling in breaking changes.

So it's probably more useful to view the converted go.mod as a “roughed-in” configuration rather than a high-fidelity equivalent, and given that, addressing this issue seems like it would be a lot of work (and a lot of complexity) in order to address a transitional problem — and hopefully a rare one even then.

@rogpeppe
Copy link
Contributor Author

In general, the module configuration converted from another dependency manager will often require adjustment anyway: for example, some dependency managers have operated at the package (rather than repo) granularity, and migrating those to modules ends up bumping the module version for all of those packages upward to the package with the highest requirement, which can end up pulling in breaking changes.

Moving dependencies forward in time is fine - that's something that's going to happen with MVS, and something that can't be avoided; semver compatibility is something we're explicitly buying into when we move to Go modules.

Moving dependencies back in time is a problem though. Moving back in time can remove fixes and features even when the publisher has taken care to respect semver requirements.

I feel strongly that as far as is possible, go mod init should not regress module versions. When migrating large amounts of code to using modules, manual inspection is error prone. If we have this guarantee, the inspection process is less important - there should be issues only if a repo hasn't respected semver.

For myself, I'm currently migrating over 70 different independent services to using modules. This kind of issue makes for a much more painful experience.

There is also the issue that it's not even possible to tell easily whether modules have regressed or not. I've written a little tool to check old and new resolved versions (which is the only reason I discovered this issue and some others), but it's pretty awkward to do.

@rogpeppe
Copy link
Contributor Author

@av86743

As for your example, you probably do not want to use future exact version, which go mod will likely mark later as erroneous, and use semver wildcard instead:

github.com/hashicorp/consul/api v1.1.x-0.20190522201912-40cec98468b8

There's no such thing as a "semver wildcard" in Go AFAIK. That's not a valid version.

@av86743
Copy link

av86743 commented Jul 12, 2019

@av86743

As for your example, you probably do not want to use future exact version, which go mod will likely mark later as erroneous, and use semver wildcard instead:
github.com/hashicorp/consul/api v1.1.x-0.20190522201912-40cec98468b8

There's no such thing as a "semver wildcard" in Go AFAIK. That's not a valid version.

@rogpeppe

Exactly. What you suggested does not make sense whatever way I am trying to turn it.

No need to bother about it, though. Migrations that you have on hand are more important.

@bcmills
Copy link
Contributor

bcmills commented Jul 12, 2019

@rogpeppe: as far as I can tell, there are several conditions that must all be met in order for the version to actually regress:

  1. The user code must have a legacy lockfile that specifies versions at the repo level.
  2. The dependency repo at the commit named in the lockfile must contain a nested module.
    • And the user code must import a package from within the nested module.
  3. At that commit, the module at the root of the repo must not require the nested module at an equivalent version (that is, a version for which the code within the nested module has the same behavior as at the named commit).
  4. The latest version of the nested module must be before the named commit.
    a. The nested module must have a release (or pre-release) tag.
    b. The named commit must be after the latest tag.
    • This implies that the user code is relying on behavior without a guarantee of stability.

It certainly is possible to meet those conditions, because you presumably would not have filed this issue otherwise. But I doubt that they co-occur often in practice.

@rogpeppe
Copy link
Contributor Author

It certainly is possible to meet those conditions, because you presumably would not have filed this issue otherwise. But I doubt that they co-occur often in practice.

One might not think so, but the consul/api package is imported by over 2000 packages, according to godoc.org, and some of the most popular pre-module revision control systems (glide, dep) specify versions at the repo lovel. So this might not be as uncommon as you might think, just because of the popularity of the module that exhibits this issue.

AFAICS this will happen to any module that uses a dependency using glide or dep that imports consul with a version specifier of ^1.0.0 or above.

@bcmills

This implies that the user code is relying on behavior without a guarantee of stability.

I think that's a questionable assertion. The code is importing the latest tagged version of consul in good faith. The latest tagged version is v1.5.1. From the pre-modules point-of-view, this version includes the api and sdk sub-packages, so it's hard to argue that you aren't guaranteed stability, unless the consul repo explicitly says that it won't allow non-module imports.

I agree that this whole situation is unfortunate, but I worry that it isn't as uncommon a scenario as you make out. If there was some solution to this that wasn't too hard, I'd still argue that it's worth doing.

@bcmills
Copy link
Contributor

bcmills commented Jul 15, 2019

the consul/api package is imported by over 2000 packages, according to godoc.org, and some of the most popular pre-module revision control systems (glide, dep) specify versions at the repo level.

If consul/api is the specific concern, then a simpler solution might be to ask the HashiCorp folks (@jefferai, @rboyer, and @freddygv, maybe?) to tag a new release of consul/api at (or after) the latest root-module release.

From the pre-modules point-of-view, this version includes the api and sdk sub-packages

IMO, the presence of a go.mod file should be prima facie evidence that the author of the module intends a module-mode interpretation. v1.5.1 includes a go.mod file, so if anything I would argue that any stronger expectation of stability is what would need to be more explicit.

I agree that this whole situation is unfortunate, but I worry that it isn't as uncommon a scenario as you make out. If there was some solution to this that wasn't too hard, I'd still argue that it's worth doing.

The hashicorp repos are the only ones I am aware of that are using nested modules for importable packages without requirement cycles between those modules. If you are aware of others, please do let me know. 🙂

@mkeeler
Copy link

mkeeler commented Aug 22, 2019

To shed a little more light on the situation here the Consul repo contains 3 modules.

github.com/hashicorp/consul
github.com/hashicorp/consul/api
github.com/hashicorp/consul/sdk

Both the api and sdk modules are nested under the root module. One big purpose we use the nested modules for is to limit the dependencies necessary for the Consul API client (the api module). Consul itself has many large dependencies that we don't want to require everyone who uses our API client to have to download and compile.

Our general strategy for modules regarding releases is:

  1. Tag the sdk module if it had updates and then update the api and root modules dependencies on it.
  2. Tag the api module if it had updates and then update the root modules dependency on it.
  3. Tag the root module.

The versions of the sdk and api modules do not correspond to the Consul version. There will be plenty of releases where neither of those two modules have updates and we aren't bumping the version. As of right now the api/v1.1.0 tag represents the latest API client and would be fully compatible with the v1.5.3 tag of the root module.

With regards to the nested module situation, it has been a source of pain for us since implementing it. So much so that we having been considering ways to automate away the nesting.

The root problem I see is that we want to be able to PR changes to our public facing API and the corresponding changes to the API client all at once. This means that both bits of code must live in the same repository (at least at the time of the PR). What we are trying to avoid is leaking all of the root modules dependencies to everyone who wants to pull in the API client.

One solution I have been thinking through is getting rid of the nested module in the same repo and instead at release time pushing the API client code to a secondary repository where the go.mod would live. This would:

  1. Maintain the development workflow of 1 PR to touch both the public API and its API client
  2. Allow a smaller set of dependencies for those pulling in just the API client
  3. Work around a handful of issues with various Go commands really not working quite right with nested modules (requiring replace directives in the root modules go.mod to prevent pulling the nested modules from github instead of using whats we already have, preventing issues with go mod vendor attempting to vendor our own nested modules and there are others)
  4. Be able to maintain better backwards compatibility with older versions of Go in the separate repo.

The downside is that many of our users would need to change the import path or now be required to pull in all of the root modules dependencies. I wrote a tool to automate fixing import paths though so the burden on our users would be minimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants