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

x/build: builders write a custom VERSION file with a simple "devel {hash}" format when testing Go #42221

Closed
mvdan opened this issue Oct 26, 2020 · 10 comments

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 26, 2020

See https://storage.googleapis.com/go-build-log/89055b21/android-amd64-emu_d4e8e53e.log. Instead of the usual format like devel +94887410d4 Sun Oct 25 07:51:58 2020 +0000 linux/amd64, they use a much simpler format via a VERSION file like devel 89055b21aff36834cb8d338420e0c553cbe5ab7a.

This is a problem because the builds will end up with an entirely different version format. This issue tracks why this custom format was added, and hopefully making the builders result in the same version that make.bash produces.

cc @dmitshur @heschik @bcmills @jayconrod

@gopherbot gopherbot added the Builders label Oct 26, 2020
@gopherbot gopherbot added this to the Unreleased milestone Oct 26, 2020
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 26, 2020

This is a problem because the builds will end up with an entirely different version format.

Why this is a problem?

I have a custom VERSION file on my GOROOT and it worked just fine.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Oct 26, 2020

See #41116, and in particular #41116 (comment).

I don't think there's anything wrong per se, but if tools want to start parsing information from go version or go env GOVERSION, we probably want to be careful with VERSION files containing arbitrary strings.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 26, 2020

Thanks. I agree that if tools want to parse Go version, it needs to be careful about arbitrary version strings. I don't think we want to disallow arbitrary version strings, though (it may not work for some tools, and that is probably fine).

@mvdan
Copy link
Member Author

@mvdan mvdan commented Oct 26, 2020

I personally don't have a horse in this race - I never create my own VERSION files, and I'm more than happy to have the tools I author simply error when a version string uses a custom format. The suggestion was by @heschik, so perhaps the need comes from gopls. cc @stamblerre

@heschik
Copy link
Contributor

@heschik heschik commented Oct 26, 2020

If we want to have a discussion about whether it's a good idea to require the major version I think that belongs on #41116, not here.

@mvdan
Copy link
Member Author

@mvdan mvdan commented Oct 26, 2020

It was @dmitshur who requested a new issue be opened for the builders :) I personally prefer keeping discussion in the original issue.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 26, 2020

This issue should be about determining what needs to be done on the builder side to support changes for #41116, if anything, and doing it. CC @golang/release.

There's more than one place in x/build that constructs a custom VERSION file, mostly related to debugging via gomote. I think those can be dealt with later.

The one that's relevant to testing commits is in buildgo.VersionTgz. It's used by cmd/coordinator in the buildStatus.writeGoSourceTo method, which has a lot of relevant information. From a relatively quick look, I don't see that the major Go version is computed there though.

@thanm
Copy link
Member

@thanm thanm commented Oct 27, 2020

Developers using toolstash also use custom version files in cases where you want to compare already-checked-in changes, see toolstash#hdr-Version_Skew. In case that matters.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Oct 27, 2020

Thanks @cherrymui and @thanm for helpful context.

After looking more into this, I left a comment on CL 264938 suggesting that it should not expect there to be any requirements met if there exists a VERSION file, since that file is allowed to have arbitrary content. It would be a problem not only on builders, but for users who have a custom VERSION file.

We may still want to change the custom VERSION file that builders write in the future, but I don't think we need to do anything now to unblock #41116 specifically.

@dmitshur dmitshur changed the title x/build: builders appear to use their own custom devel version string format x/build: builders write a custom VERSION file with a simple "devel {hash}" format when testing Go Oct 27, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 9, 2020

I don't think there's anything more to do here, so closing.

@dmitshur dmitshur closed this Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.