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: consider including Go's own version in 'go env' #41116

Closed
mvdan opened this issue Aug 28, 2020 · 22 comments
Closed

cmd/go: consider including Go's own version in 'go env' #41116

mvdan opened this issue Aug 28, 2020 · 22 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@mvdan
Copy link
Member

mvdan commented Aug 28, 2020

go env -json tells me a lot about the user's Go setup and environment, and in a format that's easy to parse. Unfortunately, go version is missing there, so if I want to fetch that I need a separate exec call.

I wonder if we could add it to go env, similar to other "not really an env var" lines like GOMOD, GOEXE, or GOHOSTARCH. For example:

$ go version
go version go1.15 linux/amd64
$ go env -json
[...]
    "GOVERSION": "go1.15",
[...]

It would not include the string prefix go version, since it's redundant, nor the linux/amd64 pair, since that's already as GOHOSTOS/GOHOSTARCH in go env.

I'm not making this a proposal for now, since the idea seems pretty simple.

/cc @bcmills @jayconrod @matloob for cmd/go

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go Tools This label describes issues relating to any tools in the x/tools repository. labels Aug 28, 2020
@ainar-g
Copy link
Contributor

ainar-g commented Aug 29, 2020

I'm very in favour of this. When I'm debugging builds on a remote machine or helping a colleague, I call go env and go version in tandem. Even the default issue template here on GitHub requires the output of both commands.

@mvdan
Copy link
Member Author

mvdan commented Aug 29, 2020

That's a good point; just having to call go env would be pretty handy when debugging problems on other machines.

@mvdan
Copy link
Member Author

mvdan commented Sep 23, 2020

On our last golang-tools call, @bcmills, @jayconrod, and @heschik raised a good point. The version reported by go version isn't the only interesting bit of information from the Go toolchain that tools would find useful. There's also the "Go language version" or major version used for build tags such as // +build go1.15. For the sake of clarity, let's call those GOVERSION and GOMAJORVERSION.

At the time of writing, if one runs Go 1.15.2, one could imagine:

$ go env
[...]
GOVERSION="go1.15.2"
GOMAJORVERSION="go1.15"

That seems simple enough. In the case of a stable release, they'll always share the first two semver components. However, it gets more interesting for devel builds. For example, with my current master build, as per src/internal/goversion/goversion.go showing const Version = 16:

$ go env
[...]
GOVERSION="devel +150bd4ffd4 Wed Sep 23 07:51:17 2020 +0000"
GOMAJORVERSION="go1.16"

We could bikeshed about the names, but it seems clear to me that both versions are useful to have, and they're clearly different.

@mvdan
Copy link
Member Author

mvdan commented Oct 21, 2020

It seems like everyone agrees this would be a good change, so I'll do a CL this week in the hopes to get it into 1.16. We can bikeshed about the var names on Gerrit.

@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

We shouldn't have two different variables. One should be enough, it should be GOVERSION, which should record the output of go version (with the 'go version' prefix and the goos/goarch suffix removed I assume).

There is a separate issue, which is that you can't tell from go version what the base version of Go is. Changing the output to do that seems fine to me. I think I would suggest changing the git hash to "go1.Version+Hash", as in:

go version devel go1.16+a8a337f1f4 Tue Oct 20 16:43:37 2020 -0400 darwin/amd64

@bcmills
Copy link
Contributor

bcmills commented Oct 21, 2020

We could close the not-quite-semver gap a little bit by using - instead of + to separate the hash: then it would read as a SemVer prefix with a pre-release suffix, which seems like not a terrible fit for the intended meaning.

@rsc
Copy link
Contributor

rsc commented Oct 22, 2020

I'm OK with '-' because the string means "before Go 1.16" not "after Go 1.16".
But I don't want to claim any interest in closing the semver gap. :-)

@heschi
Copy link
Contributor

heschi commented Oct 22, 2020

Just for the record, I don't think it reliably means either before or after. We do bump the version about the same time the tree reopens these days, but if you're building 1.16.1 before it's been tagged, it's definitely not before 1.16, right?

Adding the "base version" to the version string should work for gopls as long as it's enforced by dist as much as possible. It might be nice to validate the VERSION[.cache] file .

@mvdan
Copy link
Member Author

mvdan commented Oct 22, 2020

Do we agree to do both changes at once for 1.16? I feel like if we change the format of go env GOVERSION between 1.16 and 1.17, that would make the env var significantly less useful in 1.16.

@gopherbot
Copy link

Change https://golang.org/cl/264938 mentions this issue: cmd/dist: include "go1.x-" to devel go version strings

@dmitshur
Copy link
Contributor

dmitshur commented Oct 26, 2020

Making go version report more accurate version information for development versions is something that's been on my mind too, but I hadn't seen this issue.

@mvdan I wanted to ask if it's in scope of this issue to make it work for minor versions on release branches too?

For example, if Go is built at commit 8f14c1840d15233b7f3d08f0acf0b0559d465a56 (tip of release-branch.go1.15 as of this posting), go version will currently report go1.15.3, which is not true. Based on the pattern above, it could report:

go version devel go1.15.4-8f14c1840d15 Tue Oct 20 16:43:37 2020 -0400 darwin/amd64

Making that work may require doing something different, because unlike the main branch, release branches have a VERSION file committed. That's something that can be controlled on the side of release tooling, but need to consider people building from source. I wanted to ask since it may influence the overall direction.

@mvdan
Copy link
Member Author

mvdan commented Oct 26, 2020

Based on the pattern above, it could report:

go version devel go1.15.4-8f14c1840d15 Tue Oct 20 16:43:37 2020 -0400 darwin/amd64

I don't think so - it would report go version devel go1.15-8f14.... That is, the added string is equivalent to what is matched against build tags. It's always goX.Y, never goX.Y.Z. This is also how my CL is currently implemented. One could also call this the "Go language version", as it's what's used in the go X.Y line in a go.mod file.

release branches have a VERSION file committed

Thanks, I didn't know that (just like I didn't know that builders similarly had a hard-coded version in them). The question then is - should such a build result in go version go1.15.3, or go version devel go1.15-8f14...? I would argue the latter, because the former is certainly wrong. So I imagine that the simplest solution is to just not commit VERSION files in release branches, and rely on git tags for fully finished releases.

If the worry is that some people might build with a source tarball without the git directory present, then perhaps the VERSION file could be added right when the tag is done, then removed again. Or, as a similar mechanism to not pollute the release branch with such add and remove commits, the commit to add VERSION and tag the release could be done as a fork of the release branch.

@dmitshur
Copy link
Contributor

I see, so it's in scope of this issue to expose the major Go version but not minor.

I think that's fine as long as it leaves the door open to be able to also expose the minor version in the future. Those who are interested in just the Go language version can ignore the minor if it is added in the future (although code that just takes the whole string until the first - will break).

I can't remember what use case I had for exposing the minor version. I think it was about being exposing precise version information for informational purposes. But let me see if there are other good reasons for it.

@mvdan Are you okay with leaving the door open for exposing the minor (i.e., Go x.y.Z) version too in the future?

@mvdan
Copy link
Member Author

mvdan commented Oct 26, 2020

As far as I can tell, the minor version (or bugfix in semver) is only useful for the end user when they run a tagged release. If they're running a development source build, I don't think we should expose any minor version at all.

I'm not sure what you mean by leaving the door open, but I prefer not to expand the scope of the issue since there isn't a clear use case and it has no precedent that I can see. The precedents for exposing the major version are the build tag version and the language version, both of which alter how tools behave, but the minor version does not.

@dmitshur
Copy link
Contributor

I'm okay with not expanding scope for this issue. I think it will help to clarify what is in scope.

In #41116 (comment), Russ said:

One should be enough, it should be GOVERSION, which should record the output of go version (with the 'go version' prefix and the goos/goarch suffix removed I assume).

That suggests a hypothetical future Go 1.19.3 version would print:

$ go version
go version go1.19.3 linux/arm64

$ go env GOVERSION
go1.19.3

And a hypothetical future non-released version would print:

$ go version
go version {development version string} darwin/amd64

$ go env GOVERSION
{development version string}

And if the go version output for non-released versions changes in the future (something that is out of scope for this issue, if I understand correctly), then go env VERSION will print the new thing. Is that the plan you're suggesting @mvdan?

@mvdan
Copy link
Member Author

mvdan commented Oct 26, 2020

Yes, @dmitshur.

@dmitshur
Copy link
Contributor

That seems reasonable to me.

I think any development version string changes are better to leave to another cycle, since there's more discussion to be had. If there are concerns that it might become harder to change its format in the future, then for Go 1.16 you can consider making go env VERSION output be the empty string if it's not a release (or pre-release) version. That won't break anyone, since it's a new feature and no one can be relying on it yet.

@heschi
Copy link
Contributor

heschi commented Oct 26, 2020

@mvdan I think at this point you should probably just put the version in GOVERSION, whatever happens to be there. Don't worry about gopls; we can just continue to use the go list hackery we use today and pull from GOVERSION if/when it becomes usable.

@mvdan
Copy link
Member Author

mvdan commented Oct 26, 2020

I think any development version string changes are better to leave to another cycle, since there's more discussion to be had.

I realise that the freeze is just one week away, but I admit I don't agree that there's more discussion to be had. This issue is two months old, has involved all major stakeholders including cmd/go owners and Russ, and we've talked about this in detail in the two last golang-tools calls (you can see the notes and recordings at https://github.com/golang/go/wiki/golang-tools, if you're curious). I guess more discussion can't hurt, but it seems like we already had consensus for a solution in #41116 (comment) and the earlier comments.

I think at this point you should probably just put the version in GOVERSION, whatever happens to be there.

Indeed, this is my plan B, and part of why I wanted to do this issue in two CLs - so that at least go env GOVERSION could be merged if the version string format CL didn't make it. I'll work on the second CL tomorrow.

@dmitshur
Copy link
Contributor

To clarify, my comment was trying to answer your question of "Do we agree to do both changes at once for 1.16?" from 4 days ago, since I saw this issue was still in NeedsDecision state and I didn't see an answer to that here. I hadn't been aware of discussion outside of this issue, thanks for letting me know. It seems this issue should be moved to NeedsFix.

@mvdan
Copy link
Member Author

mvdan commented Oct 26, 2020

Indeed, I was trying to get some extra confirmation before the merges happened. I think we can safely move the issue to NeedsFix at least.

@mvdan mvdan added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 26, 2020
@gopherbot
Copy link

Change https://golang.org/cl/265637 mentions this issue: cmd/go: introduce the GOVERSION env variable

gopherbot pushed a commit that referenced this issue Apr 3, 2021
This way, "go version" will report the "base version" or major version
that the tool corresponds to. This is the same version number that is
matched against build tags such as "go1.14" or "!go1.16".

Obtaining this version being built is non-trivial, since we can't just
import a package or query git. The added comments document the chosen
mechanism, based on a regular expression. It was chosen over AST parsing
as it would add a significant amount of code without much gain, given
how simple the goversion.go file is.

For #41116.

Change-Id: I653ae935e27c13267f23898f89c84020dcd6e194
Reviewed-on: https://go-review.googlesource.com/c/go/+/264938
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Trust: Daniel Martí <mvdan@mvdan.cc>
Trust: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants