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: creating v2+ modules has lower success rate than it could #31543

Open
thepudds opened this issue Apr 18, 2019 · 5 comments
Open

cmd/go: creating v2+ modules has lower success rate than it could #31543

thepudds opened this issue Apr 18, 2019 · 5 comments
Labels
GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@thepudds
Copy link
Contributor

thepudds commented Apr 18, 2019

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

go 1.12.4

Does this issue reproduce with the latest release?

Yes, including tip.

What did you do?

Observed many problems in go.mod files for repositories with v2+ semver tags.

What did you expect to see?

A smaller rate of problems.

Some concrete issues

"go.mod files that I happen to examine" is not an unbiased random sample, but from what I have seen, the failure rate is not low when someone from the broader community opts in to modules for a pre-existing v2+ set of packages.

Issue 1

Of the various go.mod files I have looked at in repos with v2+ semver tags over the last several months, I estimate more than 50% of those go.mod files are incorrect due to missing the required /vN at the end of the module path.

Three samples I happened to see in the last few days:

All of those are missing the required /v2 in the module statement.

Issue 2

Even when someone does know to add a /vN to their module statement, they might not know to update their own import statements in their .go code within the module to add the /vN, or might miss some import statements. The end result in that case is a module accidentally depending on the v1 version of itself.

Issue 3

Many modules created from v2+ repositories are not following the best practice outlined in #25967 (comment) of incrementing the major version when adopting modules for a pre-existing v2+ repository. There can be a fairly narrow set of cases where it is not desirable to do so (for example, if you are pursuing a more sophisticated forwarding or type aliasing solution). However, based on what I have observed, I would certainly guess in the large majority of cases I have seen it was not after considered deliberation by the module author of the pros and cons of doing so, but rather more often due to the module author not knowing it is considered desirable in most circumstances.

Some likely contributing factors

Factor 1

go mod init seems to complete successfully for a v2+ repository that has not yet opted in to modules, but produces the wrong result.

Sample:

GO111MODULE=off go get -d github.com/pierrec/lz4
cd $GOPATH/src/github.com/pierrec/lz4
git describe
# output: v2.1.2
rm go.mod
go mod init
head -1 go.mod
# output: module github.com/pierrec/lz4

At first glance, the go.mod file seems to have been created properly or at least there is no error or warning, but the result is missing the required /v2 in the module statement given that repository is tagged v2.1.2.

I would guess a healthy percentage of gophers might trust the resulting go.mod created by go mod init.

Factor 2

go get does not complain if you do go get foo without a /vN for repository foo that has v2+ semver tags but foo does not have /vN in the module statement in foo's go.mod.

If someone incorrectly creates a go.mod without a required /vN in the module statement (e.g., manually, or after hitting Factor 1 above) and then does one of several permutations of go get foo (again without the /vN), they do not get a warning or error, and can move forward thinking their v2+ module has been created successfully after doing basic tests.

Here is a sample. For this lz4 repo:

  • v2.1.1 has an incorrect go.mod file missing /v2 in the module statement
  • 315a67e90e41 is the commit for v2.1.1
  • v2.0.5 is the tag immediately before the "bad" go.mod was introduced in v2.0.6.

Sample results:

# allows "bad" version:
require github.com/pierrec/lz4 v2.1.1+incompatible
 result github.com/pierrec/lz4 v2.1.1+incompatible                

# allows "bad" version converted to pseudo-version:
require github.com/pierrec/lz4 v2.1.1
 result github.com/pierrec/lz4 v0.0.0-20190327172049-315a67e90e41 

# silently gives you version just before first "bad" version:
require github.com/pierrec/lz4 latest
 result github.com/pierrec/lz4 v2.0.5+incompatible                

You get the same results if you specify require in a consumer go.mod, or if you do go get with the same version string.

On the other hand, if you use /v2 in the go get or require, it reports an error:

# error if asked for v2.1.1 using /v2
require github.com/pierrec/lz4/v2 v2.1.1
go: github.com/pierrec/lz4/v2@v2.1.1: 
 go.mod has non-.../v2 module path "github.com/pierrec/lz4" (and .../v2/go.mod does not exist) at revision v2.1.1
go: error loading module requirements

# error if asked for latest using /v2
require github.com/pierrec/lz4/v2 latest
go: github.com/pierrec/lz4/v2@v2.1.2: 
 go.mod has non-.../v2 module path "github.com/pierrec/lz4" (and .../v2/go.mod does not exist) at revision v2.1.2
go: error loading module requirements

However, accidentally doing go get foo for a v2+ module (and not knowing it should be go get foo/v2) is likely somewhat correlated with not knowing to create the go.mod with module foo/v2 in the first place.

Separately, that is not the easiest to understand error if you are not steeped in modules, but it is at least an error.

Sample test that generates results above:

cd $(mktemp -d)
export GOPATH=$(mktemp -d)
echo 'package "tempmod"' > tempmod.go

for ver in ' v2.1.1+incompatible' ' v2.1.1' ' latest' '/v2 v2.1.1' '/v2 latest'; do
  echo 'module tempmod' > go.mod
  echo "require github.com/pierrec/lz4$ver" | tee --append go.mod
  echo 'result:'
  go list -m all
  echo
done

Factor 3

#25967 (comment) is not widely known and is likely undercommunicated (e.g., as mentioned by Bryan in #27009 (comment)). (edit: sorry, corrected first link here).

Factor 4

More generally, how to properly create modules has likely been undercommunicated across the various channels available to the core Go team, including the golang.org blog, twitter, golang-nuts, etc.

Suggestions

Given time before 1.13 is short, I suggest aiming for some cheap but useful improvements.

Suggestion 1

It is probably not advisable to treat something like go get github.com/pierrec/lz4@v2.1.1 as an error even though v2.1.1 has an incorrect go.mod. It is debatable what 1.11 could or should have done in that scenario, but it is likely too late to treat as an error.

However, it seems that at least issuing a warning would be advisable. In particular, of the three variations of require or go get that are are silent in Factor 2 above, the most common is likely go get foo or go get foo@latest. It would very likely help nudge the community in the right direction to make go get foo or go get foo@latest issue a warning if the latest version of foo has v2+ semver tags but has a go.mod file that is missing a /vN. Presumably the cmd/go code is already reading and rejecting that latest go.mod, so perhaps this could be a small change. That would likely cause an author to notice they have a go.mod file that is bad in this way, or at least trigger a consumer filing an issue so that the author becomes aware after releasing a bad go.mod file.

Suggestion 2

Make the error from go mod init (without any arguments) much clearer. This would help in the case when someone has cloned their repo and is outside of their traditional GOPATH. It could say something along the lines of:

$ go mod init
go: cannot determine module path for source directory /tmp/foo (outside GOPATH, module path must be specified)
    example usage:
       'go mod init example.com/my/repo' to initialize a v0 or v1 module
       'go mod init example.com/my/repo/v2' to initialize a v2 module
    
    see https://golang.org/s/go-mod-init-faq for more details.

A side benefit is that message also helps people who don't know what to provide to go mod init (which is a healthy count of gophers), as well as help hint to people the most common module path is the repo root. (If someone wants to have a multi-module repo or place a go.mod outside of the repo root, that person already has a need to get deeper with modules than just reading an error message from go mod init, and the FAQ link could cover some of that either directly or through pointers).

A better solution would be to make go mod init aware of the git tags so that the proper /vN is applied by default in the module statement. However, I would guess there is no time for that for Go 1.13 (unless of course some more people could help polish modules in the small amount of time before freeze).

Suggestion 3

Re-open #27248 (including see comment #27248 (comment)). Aim for a golang.org/x utility that is usable by the community prior to Go 1.13 going GA that can properly add and adjust the /vN in go.mod files and .go code. A golang.org/x utility could be done after the freeze but before 1.13, as far as I understand?

This would help on multiple fronts outlined here, including helping people properly update their own go.mod, and cut down on incidents of a module accidentally depending on the v1 version of itself due to not updating import statements, as well as generally reduce the transitional pain of Semantic Import Versioning.

Suggestion 4

There are various suggestions in the go release issue that would help here, but it seems most or all of those won't be happening for 1.13, and also go release will not catch everything, including in the near-term while people might not be using go release yet).

One question would be if go release could start out as a golang.org/x utility in order to allow for iteration during the 1.13 freeze? (edit: added this as a separate suggestion in #26420 (comment)).

Suggestion 5

Increase energy on blogging, twitter PSAs or reminders, golang-nuts PSAs or reminders, etc., as well as ideally a increased push on smaller polish items in modules like better error messages.

@dmitshur dmitshur added the GoCommand cmd/go label Apr 18, 2019
@dmitshur dmitshur added this to the Go1.13 milestone Apr 18, 2019
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 18, 2019
@gopherbot
Copy link

Change https://golang.org/cl/173318 mentions this issue: cmd/go: expand cannot determine module path error

@tbpg
Copy link
Contributor

tbpg commented Apr 22, 2019

Hi @thepudds. I sent https://golang.org/cl/173318 to update the go mod init error with your second suggestion. I left out checking the current repo tags/remotes to try to guess at the module path/version for the sake of time.

Also, @jayconrod pointed out we need to be careful with modules not at the repo root.

gopherbot pushed a commit that referenced this issue Apr 22, 2019
See suggestion 2 of #31543 by thepudds.

We may want to expand 'go help mod init' in the future to document what
the module path should look like.

Change-Id: Ia559fa96fda871e011d53f42a803175abc512202
Reviewed-on: https://go-review.googlesource.com/c/go/+/173318
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/173721 mentions this issue: cmd/go: warn if falling back to a pseudo-version

@gopherbot
Copy link

Change https://golang.org/cl/174180 mentions this issue: cmd/go: error if mismatched major versions causes use of pseudo-version

@RoarkeRandall
Copy link

RoarkeRandall commented Jul 22, 2019

Just giving my experience. It wasn't immediately clear to me that semantic import version was required for v2 and above, and caused a bit of a headache to debug why I was getting psuedo versions in the go.mod file. I believe we fell into the Factor 2 section. Some more clear warnings or error logs would have been of great help.

Additionally, I think many teams will want to continue committing to master without using subdirectories. Currently that "option" is listed under the "Branching" option as a note, and implies that issues may arise from it. I can see this being a major hinderance to people adopting go modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go 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

8 participants
@rsc @andybons @dmitshur @RoarkeRandall @gopherbot @thepudds @tbpg and others