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: git tags with build metadata do not resolve #31713

Closed
aeneasr opened this issue Apr 27, 2019 · 13 comments
Closed

cmd/go: git tags with build metadata do not resolve #31713

aeneasr opened this issue Apr 27, 2019 · 13 comments
Assignees
Milestone

Comments

@aeneasr
Copy link

@aeneasr aeneasr commented Apr 27, 2019

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

$ go version
go version go1.12.4 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\foobar\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\workspace\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\foobar\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=D:\Temp\go-build415564098=/tmp/go-build -gno-record-gcc-switches

What did you do?

$ go get github.com/aeneasr/go-mod-buildmeta@v0.0.1-rc.1+buildmeta.1
go: finding github.com/aeneasr/go-mod-buildmeta v0.0.1-rc.1
go get github.com/aeneasr/go-mod-buildmeta@v0.0.1-rc.1+buildmeta.1: unknown revision v0.0.1-rc.1

$ go get github.com/aeneasr/go-mod-buildmeta@v0.0.2+buildmeta.1
go: finding github.com/aeneasr/go-mod-buildmeta v0.0.2
go get github.com/aeneasr/go-mod-buildmeta@v0.0.2+buildmeta.1: unknown revision v0.0.2
go get github.com/aeneasr/go-mod-buildmeta@v0.0.2+buildmeta.1
go: finding github.com/aeneasr/go-mod-buildmeta v0.0.2

$ go get github.com/aeneasr/go-mod-buildmeta@v0.0.2
go: finding github.com/aeneasr/go-mod-buildmeta v0.0.2
go get github.com/aeneasr/go-mod-buildmeta@v0.0.2: unknown revision v0.0.2

Despite all of those tags being available, it seems that tags build metadata is not resolving.

What did you expect to see?

I expected the dependencies to resolve.

What did you see instead?

The dependencies did not resolve.

@aeneasr aeneasr changed the title cmd/go: build metadata does not resolve when pre-release is provided cmd/go: git tags with build metadata do not resolve Apr 27, 2019
@aeneasr

This comment has been minimized.

Copy link
Author

@aeneasr aeneasr commented Apr 27, 2019

Package src/go/internal/semver/semver.go does seem to be able to resolve them just fine:

func TestCompareEqual(t *testing.T) {
	for _, tt := range [][]string{
		{"v0.0.1-rc.1+buildmeta.1", "v0.0.1-rc.1"},
	} {
		// go/internal/semver.Compare
		if cmp := Compare(tt[0], tt[1]); cmp != 0 {
			t.Errorf("Compare(%s, %s) = %v", tt[0], tt[1], cmp)
		}
	}
}
@aeneasr

This comment has been minimized.

Copy link
Author

@aeneasr aeneasr commented Apr 27, 2019

Potentially related: #28647

@jscook2345

This comment has been minimized.

Copy link

@jscook2345 jscook2345 commented May 2, 2019

I saw this today using centos6 in our CI. It turned out updating git from 1.7.1 to 1.8.3.1 (which is in centos7) fixed the problem. Maybe that (upgrading git) will help?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 7, 2019

@jscook2345: I can reproduce the observed behavior with git version 2.21.0.1020, so I don't think that's the problem.

(Also, if you check the go env output, @aeneasr is using Windows, not centos; I don't think many folks on Windows are still running that old of a git binary.)

@bcmills bcmills added this to the Go1.13 milestone May 7, 2019
@bcmills bcmills self-assigned this May 7, 2019
@aeneasr

This comment has been minimized.

Copy link
Author

@aeneasr aeneasr commented May 8, 2019

So this is not related to git - this doesn't work on multiple environments, including git version 2.21.0

@aeneasr

This comment has been minimized.

Copy link
Author

@aeneasr aeneasr commented May 8, 2019

There's also another issue we've discovered and which is currently being discussed on the Gopher slack. I don't have proof that those are related but I'm inclined to think it is.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 8, 2019

@aeneasr, not everyone who reads the issue tracker can or does use the Gopher slack. If there are relevant conversations happening there, please post a summary rather than (just) a link here.

@aeneasr

This comment has been minimized.

Copy link
Author

@aeneasr aeneasr commented May 10, 2019

From slack:

Megatrond [May 8th at 3:19 PM]
Hi, me and @aeneas have been trying to figure out why we’re having issues go get-ing some nested module in the ory/hydra repository on GitHub (see ory/hydra#1422); basically he’s added a sdk/go/hydra/go.mod in the repository and added a sdk/go/hydra/v0.0.1 tag, and I’m trying to do a go get github.com/ory/hydra/sdk/go/hydra@v0.0.1 in my end - and we’re seeing “unknown revision sdk/go/hydra/v0.0.1” as a result. We’ve also just tried go get github.com/ory/hydra/sdk/go/hydra@master which seems to somehow pull in an older commit which had a faulty dependency to gopkg.in/resty.v1 which has been corrected on master since October last year. Something is happening in the internals in go get here, as fetching it with go mod download or adding it to go.mod manually seems to work just fine. Anyone have any insights to whats happening here to help us troubleshoot this?

Megatrond [1 day ago]
After adding the sdk/go/hydra/v0.0.1 tag doing go mod download github.com/ory/hydra/sdk/go/hydra@v0.0.1 works just fine. Doing go get github.com/ory/hydra/sdk/go/hydra@v0.0.1 fails with the forementioned gopkg.in/resty.v1 dependency error that I don’t really see how can happen on a master checkout of the code base:

go: finding github.com/ory/hydra/sdk/go latest
go: finding github.com/ory/hydra/sdk latest
go: finding golang.org/x/oauth2 latest
go: finding golang.org/x/oauth2/clientcredentials latest
go: github.com/go-resty/resty@v1.12.0: parsing go.mod: unexpected module path "gopkg.in/resty.v1"
go: error loading module requirements

thepudds [3:25 PM]
You might need the tag prefix after the @ in the go get.

A little more here:
#30850 (comment)

Megatrond [3:27 PM]
whether I do go get github.com/ory/hydra/sdk/go/hydra@v0.0.1 or go get github.com/ory/hydra/sdk/go/hydra@sdk/go/hydra/v0.0.1 doesn’t seem to matter; it still fails on the gopkg.in/resty.v1 dependency - which I don’t think I understand where is coming from, as it’s no longer a dependency of Hydra as of last year - it just feels like somewhere it’s loading some old code, I just don’t understand the discrepancy between go mod download and go get here I think.

Aeneas Rekkas [3:30 PM]
I think the issue could be related to the tags using + - maybe the resolver breaks completely because of that: #31713

thepudds [3:45 PM]
Are you in the process of adding and removing tags?

Also, multiple multiple repositories are very, very subtle. If you haven’t already carefully read this, I would recommend doing so:
https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories

You can also try with tip, which might give better error messages in this case.
An easy way to get tip is go get golang.org/dl/gotip && gotip

Megatrond [3:53 PM]
tip gives the same errors as latest 1.12 - to my understanding it’s set up correctly at least, I made a test repo that mirrored the intended functionality of the setup in the hydra repository and it works just fine there so there is something with how go get is resolving things in the hydra repo that causes the issue. Again, go download works fine, adding require github.com/ory/hydra/sdk/go/hydra v0.0.1 to go.mod manually also works fine; it’s only go get struggling as far as I can tell

David Genest [4:38 PM]
you have a sdk/go/hydra@v0.0.1 tag in that repo is that wanted ?

Megatrond [4:52 PM]
yup

Tyler Bui-Palsulich [6:37 PM]
@megatrond I tried it out and got the same error as you. Sorry for the pain here.

Looking at the repo setup, I see there is a go.mod at the root of ory/hydra. I don't see a dependency of ory/hydra/sdk/go/hydra on a ory/hydra. So, I wonder if that's causing problems. See this sentence from the multi-module wiki page @thepudds linked, 'The way to get around this is to make the newly-added module depend on the module it was "carved out" from, at a version after which it was carved out.'

Tyler Bui-Palsulich [6:40 PM]
This is the section worth reading if you're adding a module to a multi-module repo: https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository

Megatrond [8:13 AM]
@tyler Bui-Palsulich myeah, but I think I did a replication of the same setup here and that works as expected with go get as well as go mod download (which ory/hydra also does): https://github.com/tanordheim/hydra-sdk-mod-test - i’m not sure what the concrete difference between those two repos are modulewise that should cause one to work and not the other

thepudds [10:04 AM]
@megatrond One difference is the full repository has more history.
Have you had a chance to read everything here:
https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories

Megatrond [10:06 AM]
I have, yeah, I just don’t quite see how the history affects it; according to those docs, a module starts at the root (presuming there’s a go.mod in the root) and contains the entire subtree excluding those that contains another go.mod; and versioning a submodule needs to be done by prefixing the version number by the relative path to the nested go.mod. That is the case for ory/hydra, I think - I might be missing something super obvious but unless i’m completely misunderstanding the docs it should be set up correctly

thepudds [10:07 AM]
Have you done this piece from there:
The way to get around this is to make the newly-added module depend on the module it was "carved out" from, at a version after which it was carved out.

Megatrond [10:08 AM]
I know it should ideally be its own repository, but it isn’t right now and it’s not my project - I’m just trying to make it work
hm, yeah, i just don’t know if that would solve anything and just cause the same problem that we were trying to avoid by making that nested module to begin with - thoughts, @aeneas?

thepudds [10:14 AM]
What problem are you trying to avoid by making that nested module?

Megatrond [10:15 AM]
we’re basically trying to see if there would be a way to just depend on a small set of generated code inside the ory/hydra repository that can be used for clients implementing their API - instead of having to pull down the whole ory/hydra module with all its dependencies since 99% of it is not needed

thepudds [10:16 AM]
There is a healthy chance export GOPROXY=https://proxy.golang.org would speed things up. (That is new and I think officially alpha status, I think: https://proxy.golang.org). You could alternatively try export GOPROXY=https://gocenter.io, which has been around longer.

Megatrond [10:16 AM]
they have a set of generated API clients generated from swagger-defs, client code implementing login/consent providers only really need those and nothing else from ory/hydra - I was trying to see if I could find a sensible way to speed up the CI pipelines for a project here as the Go module cache was close to 1GB when we depend on ory/hydra directly

vs. like 80 or so when I pull down the generated API client only

ideally they should probably be split out to its own repo and be pulled into the main repo as a dependency - I was just trying to see if there was a simpler way that didn’t require them to change as much on their end

thepudds [10:19 AM]
See here:
#29935 (comment)

And:
#29935 (comment)

Megatrond [10:19 AM]
yeah, I’ll give it a shot and see if it helps 🙂

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 13, 2019

The tricky thing about tags with metadata is that they can introduce ambiguity.

What happens if I tag one commit as v1.0.0-rc1+a, and a separate commit as v1.0.0-rc1+0? Which one represents the “real” v1.0.0-rc1, especially if neither of those commits is a parent of the other? (Per https://semver.org/#spec-item-10, “two versions that differ only in the build metadata, have the same precedence”.)

If we emit an error in case of ambiguity, then we'll have the unfortunate situation that an existing v1.0.0-rc1 can resolve successfully to v1.0.0-rc1+a, but then break when a v1.0.0-rc1+0 is added later.

So I think we should probably allow go get to use such tags (the same way we allow go get to use tags in general), but resolve them to unambiguous pseudo-versions.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 13, 2019

Change https://golang.org/cl/176905 mentions this issue: cmd/go: convert semver tags with metadata to pseudoversions

@thepudds

This comment has been minimized.

Copy link

@thepudds thepudds commented May 14, 2019

@bcmills @jayconrod Does this change mean any previously working go.mod files might stop working? (I think the answer is no, but not 100% sure).

Also, would it make sense to re-open this to track updating the documentation? Perhaps a sentence or so in one or both of these two sections might be sufficient:

https://golang.org/cmd/go/#hdr-Module_queries

If the revision is also tagged with a semantic version, the query evaluates to that semantic version. Otherwise the query evaluates to a pseudo-version for the commit.

https://golang.org/cmd/go/#hdr-Pseudo_versions

There are three pseudo-version forms:
[...]

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 14, 2019

@thepudds, the net effect of the fix is that we mostly treat non-canonical semver tags as non-semver tags (resolving them to pseudo-versions, which are what we actually write to the file).

We do use the non-canonical semver tag as the base version for the computed pseudo-version, but I think we already did that before anyway. (If you named the commit rather than the tag, you would have gotten the same pseudo-version as a result.)

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 14, 2019

I thought about how to update the documentation for this change, but (especially given the low prevalence of build metadata on source code in the wild) I don't think the marginal clarity of the documentation is worth the marginal distraction of mentioning it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.