Closed hajimehoshi closed 5 years ago
bcmills
commented
6 years ago Note that, in general, a given commit can correspond to multiple tagged versions too: nothing stops you from, say, tagging the same commit as all of v1.8.5, v1.9.0-rc1, and v1.9.0.
hajimehoshi
commented
6 years ago OK, so I fixed the issue title. Thanks!
thepudds
commented
5 years ago I think https://golang.org/cl/174061 would help this as well (by making it deterministic by selecting the semantically highest tag when it first creates a pseudo-version).
However, is part of the complaint here that a human can manually enter different pseudo-versions, and/or that different go.mod files over time might have different pseudo-versions for the same commit if tags are added over time to a single commit?
thepudds
commented
5 years ago @hajimehoshi In your example that you reported here, is it the case that most of the pseudo-versions you tried had the "wrong" semver tag within them for that same commit 1c088dc8b6d3 you kept using, but then go mod tidy did not update those "wrong" pseudo-versions to have correct semver tag within the pseudoversion?
@bcmills Would it be reasonable for go mod tidy to be defined to reach back to the source for any pseudo-version found in go.mod to pick the semantically highest semver tag contained within that commit and update the corresponding pseudo-version within the go.mod? Or at least for go mod tidy to do that update if the pseudo-version found in go.mod has a "wrong" semver tag within it?
CC @rogpeppe
hajimehoshi
commented
5 years ago but then go mod tidy did not update those "wrong" pseudo-versions to have correct semver tag within the pseudoversion?
As far as I remember, correct. Let me double-check later.
bcmills
commented
5 years ago @thepudds, I don't think go mod tidy is the right place for that. (It shouldn't hit the network if the module graph and the package graph are already in alignment and the required modules are already in the local cache.)
We also can't reject versions that are arbitrarily below that implied by parent tags. The same commit may be (for example) tagged as v1.3.0 and v1.2.1 (if it is, say, released as v1.3.0 and subsequently determined to fix a regression in v1.2.0), so it should be possible to name a commit based on any tag it may logically succeed.
However, we probably should reject (that is, refuse to even resolve) pseudo-versions that are above that implied by the highest tag, since that would allow modules to forcibly downgrade dependencies by misrepresenting the version of an old commit.
gopherbot
commented
5 years ago Change https://golang.org/cl/181881 mentions this issue: cmd/go: validate pseudo-versions against module paths and revision metadata
gopherbot
commented
5 years ago Change https://golang.org/cl/182178 mentions this issue: cmd/go/internal/modfetch: re-resolve commit hashes in readDiskStat
bcmills
commented
5 years ago Here is a sample of affected paths, derived from https://index.golang.org:
bcmills
commented
5 years ago Unfortunately, I don't have a reverse-index of go.mod files in which (even a subset of) those versions appear.
marwan-at-work
commented
5 years ago Responding to @bcmills's slack question on how we usually write pseudo semvers in Go.mod files:
I never manually write a pseudo version, I usually do go get <some-pkg>@<full-long-sha> and let go convert a full long hash to a pseudo version
For example: go get github.com/pkg/errors@27936f6d90f9c8e1145f11ed52ffffbfdb9e0af7
and then my go.mod file will actually turn it into whatever pseudo semver.
It would be nice if people followed that convention and never wrote their own pseudo semvers...not sure if the Go command can help people towards that practice.
rogpeppe
commented
5 years ago @marwan-at-work That's what I do now, but originally I wasn't aware of that very convenient behaviour. I think that in principle it would be nice if the Go command failed on a pseudoversion with a mismatching date and suggest the alternative approach. My worry is that there are already lots of bad versions out there and this will cause people's builds to fail unexpectedly, giving more bad press to modules.
thepudds
commented
5 years ago Hi @bcmills, a couple questions.
Q1. From https://golang.org/cl/181881, it seems it will do a hard error if one of these types of previously allowed mistakes is found in a go.mod. Does that apply to go.mod files for both direct and indirect dependencies? I suspect the answer is yes.
If so, it means a "rare" problem on a per go.mod basis can end up being not so rare to experience when even a small-ish project can have a large count of dependencies in its module-level graph. Also, a single mistake by a single popular project could end up with wide impact, and that mistake could even happen a few months from now, if I am following.
It might make sense to be cautious here. It seems a warning in 1.13 would be less disruptive, and give more time to adapt.
Q2. Does that hard error also get triggered by any go.mod visited across the module-level dependency graph, including possibly being triggered by go.mod files from old versions that don't even end up being selected as a final version for the final build list?
If so, that increases the risk of experiencing a hard failure for something that used to work in Go 1.12, including if some indirect dependency might have fixed the problem several months ago but an old version of that go.mod still gets visited and triggers the error. That would mean the "hangover" of a mistake ends up lasting longer. That said, not sure if that applies here.
Here is an example of it failing with that CL when trying to get the current version of a reasonably popular utility:
$ go mod init tempod
$ export GOPROXY=direct
$ go get github.com/golangci/golangci-lint/...
go get: github.com/golangci/golangci-lint@v1.17.1 requires
github.com/go-critic/go-critic@v0.0.0-20181204210945-1df300866540:
pseudo-version does not match timestamp (2019-05-26T07:48:19Z)That is devel +13c08e5389 Fri Jun 14 17:40:17.
Here is where that change seemed to happen. In that particular case, I don't know why just the commit hash of the pseudoversion was changed in the go.mod, but my first guess would be sometimes people might do a hand edit where they felt it slightly more convenient to just update the hash (without deleting the whole pseudoversion first), or perhaps in some cases people don't know that they can do "module queries" in their go.mod (including specifying just a commit hash).
Finally, sorry in advance if I am not understanding the impact of this change or when it triggers.
thepudds
commented
5 years ago @bcmills also, skimming over your list just now, it seems there are cases where the timestamp is a dozen or so minutes off, such as:
golang.org/x/net 60506f45cf65
2019-06-03 09:10:49 +0000 UTC
2019-06-03 09:33:02 +0000 UTC
(and 64 more)That example is probably not from a human edit, I would guess? Do you have a sense of where that type of error might be coming from?
bcmills
commented
5 years ago it will do a hard error if one of these types of previously allowed mistakes is found in a
go.mod. Does that apply togo.modfiles for both direct and indirect dependencies?
Yes. If it were to apply only to indirect dependencies, then it would be possible to “pin” an erroneous pseudo-version using a replace directive, which I don't want to allow. (The owner of the module, not the consumer of that module, should determine its mapping of commits to versions.)
If so, that increases the risk of experiencing a hard failure for something that used to work in Go 1.12, including if some indirect dependency might have fixed the problem several months ago but an old version of that
go.modstill gets visited and triggers the error.
Yes. I'm actually pretty worried about that scenario, and it's something we'll need to watch closely during the beta. However, it should at least be possible to repair locally using a replace directive:
replace github.com/go-critic/go-critic v0.0.0-20181204210945-1df300866540 => v0.0.0-20190526074819-1df300866540If we find a small number of such dependencies in central modules, we could also consider a whitelist of invalid {module, version} pairs. That's the most reasonable mitigation I can think of at this point that doesn't also allow arbitrarily-bad version strings going forward.
perhaps in some cases people don't know that they can do "module queries" in their
go.mod(including specifying just a commit hash).
That seems likely to me.
bcmills
commented
5 years ago As for the handful of golang.org/x and similar modules with very large numbers of timestamp variations: I have no idea what's going on with those, and I hope that we can flush that problem out during the beta. My best guess is that someone, somewhere (maybe even the Go project itself!) has a buggy CI script that is attempting to produce its own pseudo-versions for specific commits (possibly the master commit?).
bcmills
commented
5 years ago @jayconrod suggested that I expand the search for affected versions by looking for pseudo-versions with the same timestamp but different commit hashes. Filtering out the v0.0.0-0. parse errors (for which I have no explanatory theory) and the v0.0.0-[…]+incompatible errors (which we have confirmed are a bug internal to the proxy), that expands the results as below.
bcmills
commented
5 years ago Of the remaining invalid base revisions:
Two come from replace directives.
github.com/census-instrumentation/opencensus-proto v0.1.0-0.20181214143942-ba49f56771b8 comes from github.com/pulumi/tf2pulumi.github.com/cloudwan/ginkgo v1.6.0-0.20190213151947-95174e8d10cd comes from a github.com/cloudwan/gohan.replace directives don't affect downstream modules, the damage from these is very limited.The bad versions of github.com/containerd/containerd and github.com/docker/docker are from manual edits in github.com/docker/buildx and github.com/moby/buildkit by one or a small number of authors (@tonistiigi in particular).
docker ecosystem. It's not obvious to me whether buildx or buildkit are central modules within that ecosystem, but if so we may need to whitelist the specific versions in question.The bad version of github.com/modsdemo/v0tags is due to manual testing by @leitzler.
I could not locate the remainder with a Google or GitHub search. I suspect they are dependencies of non-public modules.
gopkg.in/gin-gonic/gin.v1 v1.0.0-0.20170702092826-d459835d2b07github.com/netlify/netlify-commons v0.17.0-0.20190606095701-c4d04dcc126bgopkg.in/bluesuncorp/validator.v5 v5.0.0-0.20150523023324-07cbdd2e6dfdgithub.com/containers/libpod v1.4.0-0.20190614192453-4a450d55d95fgithub.com/google/wire v0.3.0-0.20190611223505-93b1ce745f8dbcmills
commented
5 years ago Many of the inconsistent revisions appear in what appear to be smaller-scale forks of larger modules. The long revisions in particular tend to appear in replace directives.
The skewed k8s.io dates in particular tend to be offset by 7 hours. That suggests a bug in a conversion between America/Los_Angeles and UTC, and I'm guessing has something to do with the peculiar way they're using replace directives (see kubernetes/kubernetes#63607; CC @liggitt).
thepudds
commented
5 years ago This seems to at least attempt to use the go command to get the version string for a SHA:
https://github.com/kubernetes/kubernetes/blob/master/hack/pin-dependency.sh#L75
But maybe that is not right, or maybe another script does it incorrectly.
liggitt
commented
5 years ago k8s.io/apimachinery 1f207b29b441
2019-05-13 18:25:58 +0000 UTC
2019-05-14 01:25:58 +0000 UTC
k8s.io/apimachinery 5bae42371a56
2019-04-30 14:11:24 +0000 UTC
2019-04-30 21:11:24 +0000 UTC
k8s.io/apimachinery 63a6072eb563
2019-06-02 11:36:12 +0000 UTC
2019-06-02 18:36:12 +0000 UTC
k8s.io/code-generator b1289fc74931
2018-11-28 19:10:24 +0000 UTC
2019-03-01 17:30:42 +0000 UTCcc @sttts
tmthrgd
commented
5 years ago tmthrgd.dev/go/ddns is my package, but I've since switched the import path over to go.tmthrgd.dev/ddns. It's a command line tool so I'm certain it never appeared in a go.mod file.
Grepping through my bash history I see the following that line up:
24154 [2019-04-30 10:07:18] GO111MODULE=on go install tmthrgd.dev/go/ddns
24158 [2019-04-30 10:07:46] go install tmthrgd.dev/go/ddnsI believe I was in a $GOPATH/src directory and at the time I had GOPROXY=https://proxy.golang.org set.
These are the go installs I did before and after that time so I was running go1.12.1 at the time (for above go is ~/sdk/go1.12.1/bin/go).
23689 [2019-03-21 16:53:18] go get golang.org/dl/go1.12.1
23690 [2019-03-21 16:53:26] go1.12.1 download
24178 [2019-04-30 15:32:49] go get golang.org/dl/go1.12.4
24180 [2019-04-30 15:33:07] go1.12.4 downloadMy go env should have looked like this:
$ GOPROXY=https://proxy.golang.org go1.12.1 env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/tom/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/tom/go"
GOPROXY="https://proxy.golang.org"
GORACE=""
GOROOT="/home/tom/sdk/go1.12.1"
GOTMPDIR=""
GOTOOLDIR="/home/tom/sdk/go1.12.1/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build145672369=/tmp/go-build -gno-record-gcc-switches"My timezone is:
$ date +"%Z %z"
ACST +0930Just for reference this line appears in my bash history and lines up with one of the timestamps:
24152 [2019-04-30 10:06:47] git commitIt was run in the module directory and I have a precommit hook that runs gofmt -l (edit: on individual changed .go files), though I don't know how or why that would affect anything.
I'm happy to provide any more information if needed.
bcmills
commented
5 years ago Confirmed that the v0.0.0-0. entries are due to a bug internal to the Go module proxy, introduced around June 3 and fixed around June 12 (part of the fix for #32461). Hopefully that's a short enough window that those invalid versions are not widespread.
bcmills
commented
5 years ago Looks like the k8s.io issue should be fixed in https://github.com/kubernetes/publishing-bot/issues/186.
bcmills
commented
5 years ago @tmthrgd, thanks for the info. Most likely the error is in some consumer of your module rather than your module itself. (We're still trying to figure out how to investigate the sources of these errors, at least when those sources are other open-source modules.)
jayconrod
commented
5 years ago I did some analysis on whether CL 181881 would break many projects. I found 12 modules out of 1531 analyzed that would be broken. This seems like a small enough number that I think we should proceed with the fix as it is.
https://proxy.golang.org/<modpath>/@latest to tell me the latest version, then https://proxy.golang.org/<modpath>/@v/<version>.mod to get this. If @latest was not available, I used the last value in https://proxy.golang.org/<modpath>/@v/list for the version.go list -m all with CL 181881 and with Go 1.12.6 for comparison. Both commands were run with GOPROXY=direct. A different GOPATH was used for each Go version, but each GOPATH was shared with all modules to avoid hitting GitHub too hard (I already got rate limited a bit).I didn't find any modules where the build list was constructed successfully but with differences in both versions of Go.
There were 2 modules where the latest version has an incorrect module path. For example in gopkg.in/gin-gonic/gin.v1@v1.4.0, the module path is github.com/gin-gonic/gin. I skipped these, since the correct name of the module was covered already.
There were 12 modules with new validation errors due to this CL.
github.com/appscode/go
go: github.com/flosch/pongo2@v0.0.0-20181225140029-79872a7b2769 requires
github.com/go-check/check@v1.0.0-20180628173108-788fd7840127: invalid pseudo-version: major version without preceding tag must be v0, not v1
github.com/cloudflare/cloudflare-go
go: golang.org/x/lint@v0.0.0-20190511005446-959b441ac422: invalid pseudo-version: does not match version-control timestamp (2019-04-09T20:28:23Z)
github.com/dubbo/go-for-apache-dubbo
go: github.com/dubbogo/getty@v0.0.0-20190523180329-bdf5e640ea53: invalid pseudo-version: does not match version-control timestamp (2019-05-23T10:03:29Z)
github.com/flosch/pongo2
go: github.com/go-check/check@v1.0.0-20180628173108-788fd7840127: invalid pseudo-version: major version without preceding tag must be v0, not v1
github.com/go-interpreter/wagon
go: github.com/twitchyliquid64/golang-asm@v0.0.0-20190315094337-365674df15fc: invalid pseudo-version: does not match version-control timestamp (2019-01-26T20:37:39Z)
github.com/jhump/protoreflect
go: google.golang.org/genproto@v0.0.0-20170818100345-ee236bd376b0: invalid pseudo-version: does not match version-control timestamp (2017-08-18T01:03:45Z)
github.com/lightninglabs/neutrino
go: github.com/btcsuite/btcwallet@v0.0.0-20190319010515-89ab2044f962 requires
github.com/lightninglabs/neutrino@v0.0.0-20190313035638-e1ad4c33fb18 requires
github.com/btcsuite/btcwallet@v0.0.0-20190313032608-acf3b04b0273 requires
github.com/lightninglabs/neutrino@v0.0.0-20190213031021-ae4583a89cfb requires
github.com/btcsuite/btcwallet@v0.0.0-20180904010540-284e2e0e696e33d5be388f7f3d9a26db703e0c06: invalid pseudo-version: revision is longer than canonical (284e2e0e696e)
github.com/ltcsuite/ltcd
go: github.com/ltcsuite/ltcutil@v0.0.0-20190507082654-23cdfa9fcc3d: invalid pseudo-version: does not match version-control timestamp (2019-05-07T13:33:22Z)
github.com/tonistiigi/fsutil
go: golang.org/x/crypto@v0.0.0-20190129210102-0709b304e793: invalid pseudo-version: does not match version-control timestamp (2018-09-04T16:38:35Z)
gomodules.xyz/cert
go: github.com/appscode/go@v0.0.0-20190424183524-60025f1135c9 requires
github.com/flosch/pongo2@v0.0.0-20181225140029-79872a7b2769 requires
github.com/go-check/check@v1.0.0-20180628173108-788fd7840127: invalid pseudo-version: major version without preceding tag must be v0, not v1
kmodules.xyz/client-go
go: github.com/appscode/go@v0.0.0-20190424183524-60025f1135c9 requires
github.com/flosch/pongo2@v0.0.0-20181225140029-79872a7b2769 requires
github.com/go-check/check@v1.0.0-20180628173108-788fd7840127: invalid pseudo-version: major version without preceding tag must be v0, not v1
kmodules.xyz/offshoot-api
go: kmodules.xyz/client-go@v0.0.0-20190524133821-9c8a87771aea requires
github.com/appscode/go@v0.0.0-20190424183524-60025f1135c9 requires
github.com/flosch/pongo2@v0.0.0-20181225140029-79872a7b2769 requires
github.com/go-check/check@v1.0.0-20180628173108-788fd7840127: invalid pseudo-version: major version without preceding tag must be v0, not v1I was happy not to see any problems with invalid +incompatible versions. Several modules require github.com/pierrec/lz4 v2.0.5+incompatible, which is the last valid version.
thepudds
commented
5 years ago Maybe this is unrelated, but posting here for now in case it is related.
Getting k8s.io/kubernetes at master then k8s.io/api at master fails with gotip complaining about unknown revision v0.0.0, but same steps work with Go 1.12.6 without error.
go get golang.org/dl/gotip && gotip download
export GOPROXY=direct
export GOSUMDB=off
export GOPATH=$(mktemp -d)
cd $(mktemp -d)
gotip mod init m
gotip get -v -d k8s.io/kubernetes@master # current master: 8c3b7d7679cc
gotip get -v -d k8s.io/api@master # current master: dcce3486da33which fails with:
go: extracting k8s.io/api v0.0.0-20190620073856-dcce3486da33
go: downloading k8s.io/api v0.0.0
go get k8s.io/api@master: unknown revision v0.0.0mod graph says:
gotip mod graph | egrep 'k8s.io/api@v0.0.0$'
k8s.io/kubernetes@v1.16.0-alpha.0.0.20190623232353-8c3b7d7679cc k8s.io/api@v0.0.0gotip is devel +c290cb6 Sun Jun 23 22:20:39 2019 +0000 linux/amd64.
thepudds
commented
5 years ago The bad versions of github.com/containerd/containerd and github.com/docker/docker [...]
Those are concerning because of the large number of modules with transitive dependencies within the docker ecosystem. It's not obvious to me whether buildx or buildkit are central modules within that ecosystem, but if so we may need to whitelist the specific versions in question.
Probably not a surprise, but cloning docker, then doing mod init followed by mod tidy fails with tip:
export GOPROXY=direct
export GOSUMDB=off
export GOPATH=$(mktemp -d)
git clone --depth=1 https://github.com/docker/docker
cd docker
gotip mod init
gotip mod tidywhich fails with:
go: github.com/moby/buildkit@v0.5.2-0.20190611004307-c24275065aca requires
github.com/containerd/containerd@v1.3.0-0.20190426060238-3a3f0aac8819:
invalid pseudo-version: version before v1.3.0 would have negative patch numberThose steps pass with Go 1.12.6.
gopherbot
commented
5 years ago Change https://golang.org/cl/183618 mentions this issue: cmd/go/internal/modfetch: treat a missing go.mod file as a “not exist” error
gopherbot
commented
5 years ago Change https://golang.org/cl/183619 mentions this issue: cmd/go/internal/modfetch: return structured errors from proxy operations
bcmills
commented
5 years ago @thepudds, I don't know why go1.12.6 doesn't detect the error in k8s.io/kubernetes, but gotip is correct to fail that sequence.
k8s.io/kubernetes requires k8s.io/api v0.0.0 here. There is no such version of k8s.io/api tagged, and because the repo doesn't have any semver-style tags, v0.0.0- is the correct pseudo-version prefix for its commits.
As far as I can tell, because k8s.io/kubernetes relies so heavily on replace directives to pin its dependencies it can only be built in module mode if it is the main module, or if the main module replicates its replace directives more-or-less exactly.
gopherbot
commented
5 years ago Change https://golang.org/cl/184720 mentions this issue: cmd/go: tighten the check for pseudo-version base tags
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?What did you do?
Create a project repository with a main.go importing
github.com/hajimehoshi/ebiten, and created go.mod. I tested 5 types of go.mod:Run
go1.11rc2 mod tidywith each go.modWhat did you expect to see?
go mod tidyshould update the dependency version tov1.8.0-alpha.0.20180819111657-1c088dc8b6d3since there is a tagged commitv1.8.0-alphaWhat did you see instead?
In any cases,
go mod tidydidn't updatego.mod.I think especially the case of
v1.9.0-...seems dangerous since it might pollute the cache and when I release the version v1.9.0-*, the cache might not work well.