cmd/go: unnecessarily multiple possible pseudo versions for one commit #27173
Comments
|
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 |
hajimehoshi
changed the title
|
OK, so I fixed the issue title. Thanks! |
bcmills
added
the
NeedsInvestigation
|
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 |
|
@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 @bcmills Would it be reasonable for CC @rogpeppe |
As far as I remember, correct. Let me double-check later. |
|
@thepudds, I don't think We also can't reject versions that are arbitrarily below that implied by parent tags. The same commit may be (for example) tagged as 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. |
|
Change https://golang.org/cl/181881 mentions this issue: |
|
Change https://golang.org/cl/182178 mentions this issue: |
|
Here is a sample of affected paths, derived from https://index.golang.org: |
|
Unfortunately, I don't have a reverse-index of |
Signed-off-by: Tonis Tiigi <[email protected]>
|
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 For example: 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. |
|
@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. |
|
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 If so, it means a "rare" problem on a per 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 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 Here is an example of it failing with that CL when trying to get the current version of a reasonably popular utility: That is 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 Finally, sorry in advance if I am not understanding the impact of this change or when it triggers. |
|
@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: 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? |
Yes. If it were to apply only to indirect dependencies, then it would be possible to “pin” an erroneous pseudo-version using a
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 If 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.
That seems likely to me. |
|
As for the handful of |
|
@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 |
|
Of the remaining invalid base revisions:
|
|
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 The skewed |
|
This seems to at least attempt to use the 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. |
cc @sttts |
Previously, when we resolved a commit hash (not a complete version), we always checked the contents of the module cache for any pseudo-version matching that commit. However, there are many possible names for a given commit. Generally the semantically-highest valid name is the best, and that may change over time as new tags are added, so if we are able to fetch a better name from upstream we should do so. Otherwise, we should fall back to the highest appropriate name found in the cache. Fixes #27171 Updates #27173 Change-Id: Ib5c7d99eb463af84674e969813039cbbee7e395b Reviewed-on: https://go-review.googlesource.com/c/go/+/182178 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jay Conrod <[email protected]> Reviewed-by: roger peppe <[email protected]>
|
Grepping through my bash history I see the following that line up: I believe I was in a These are the go installs I did before and after that time so I was running go1.12.1 at the time (for above My My timezone is: Just for reference this line appears in my bash history and lines up with one of the timestamps: It was run in the module directory and I have a precommit hook that runs I'm happy to provide any more information if needed. |
|
Confirmed that the |
|
Looks like the |
|
@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.) |
|
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. Methodology
FindingsI 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 There were 12 modules with new validation errors due to this CL. I was happy not to see any problems with invalid |
|
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 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: mod graph says: gotip is |
Probably not a surprise, but cloning docker, then doing 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: Those steps pass with Go 1.12.6. |
|
Change https://golang.org/cl/183618 mentions this issue: |
|
Change https://golang.org/cl/183619 mentions this issue: |
|
@thepudds, I don't know why
As far as I can tell, because |
…” error If we have found a repository at the requested version but it does not contain a go.mod file in an appropriate subdirectory, then the module with the given path does not exist at that version. Therefore, we should report it with an error equivalent to os.ErrNotExist so that modload.Query will continue to check other possible module paths. Updates #27173 Change-Id: Ica73f4bb97f58e611a7f7d38183ee52fef5ee69a Reviewed-on: https://go-review.googlesource.com/c/go/+/183618 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jay Conrod <[email protected]>
CL 181881 added structured error types for direct fetches. Use those same structured errors to format proxy errors consistently. Also ensure that an empty @v/list is treated as equivalent to the module not existing at all. Updates #27173 Updates #32715 Change-Id: I203fd8259bc4f28b3389745f1a1fde936b0fa24d Reviewed-on: https://go-review.googlesource.com/c/go/+/183619 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jay Conrod <[email protected]>
|
Change https://golang.org/cl/184720 mentions this issue: |
Do not allow a pseudo-version derived from a canonical tag to refer to the same revision as the tag itself. It's unnecessary (because canonical tags already have a total ordering) and confusing (the pseudo-version appears to come after the tag, but actually refers to the exact same revision). Updates #32879 Updates #27173 Change-Id: I02befedbe89c8819bdd93e470783ce63fc813193 Reviewed-on: https://go-review.googlesource.com/c/go/+/184720 Run-TryBot: Bryan C. Mills <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jay Conrod <[email protected]>
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.The text was updated successfully, but these errors were encountered: