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

Comments

Projects
None yet
4 participants
@rogpeppe
Copy link
Contributor

commented Jul 10, 2019

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
)

@rogpeppe rogpeppe added the modules label Jul 10, 2019

@av86743

This comment has been minimized.

Copy link

commented Jul 10, 2019

@rogpeppe

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

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

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@av86743

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

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 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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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.

@av86743

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

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

This comment has been minimized.

Copy link
Member

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. 🙂

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.