Skip to content

cmd/dist, x/build/cmd/golangbuild: switch to a devel version format that satisfies go/version.Lang and go/version.IsValid in more cases #73372

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

Closed
dmitshur opened this issue Apr 14, 2025 · 10 comments
Assignees
Labels
Builders x/build issues (builders, bots, dashboards) FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. ToolProposal Issues describing a requested change to a Go tool or command-line program.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Apr 14, 2025

golangbuild currently generates VERSION files with strings like this, a behavior that the Go build system had for a while:

devel <commit>
devel <change>/<patchset>

Which is broadly based on make.bash's auto-detection logic used to construct a version when a VERSION file specifying it isn't present, which generates version strings in a similar format (the "go1.X-" prefix being new as of CL 264938):

devel go1.X-<short commit> <date>

That format satisfies the property that a "devel" substring is present, but not the property that go/version.IsValid is true and go/version.Lang works on it.

As motivated in #73369 (comment), I suggest we change to the following format that have all of the aforementioned properties:

go1.X-devel_<commit>
go1.X-devel_<change>_<patchset>

Where go1.X corresponds to the language version specified in internal/goversion.Version, something golangbuild has easy access to. Something to consider including .0 like go1.X.0-devel_..., but given the first pre-release version (e.g., go1.25rc1) leaves it out, maybe its fine to leave it out too. Since this is just a development version which isn't tagged or published anywhere, changing it if we find a need to shouldn't be too expensive.

There might be test cases or other code that relies on the current format, such as checking for "devel" prefix (or its absence), that'd need to be updated as part of this.

golangbuild.maybeUpdateVersionFile diff
diff --git a/go/src/infra/experimental/golangbuild/buildmode.go b/go/src/infra/experimental/golangbuild/buildmode.go
index 0c1bcf13f5..0452c72822 100644
--- a/go/src/infra/experimental/golangbuild/buildmode.go
+++ b/go/src/infra/experimental/golangbuild/buildmode.go
@@ -123,8 +123,9 @@ func getGo(ctx context.Context, spec *buildSpec, goName, goroot string, goSrc *s
 //   - If the input property "version_file" is present, it always overwrites
 //     the VERSION file with that value.
 //   - If no VERSION file is present or the VERSION file is empty, then the
-//     VERSION file is written with contents `devel <commit>` or
-//     `devel <change>/<patchset>` (existing behavior).
+//     VERSION file is written with contents `go1.X-devel_<commit>` or
+//     `go1.X-devel_<change>_<patchset>` (this matches the version that
+//     make.bash auto-infers from git, other than <date> being left out).
 //   - If a VERSION file is present AND the first line matches `go1.X.Y`,
 //     then only the first line is kept, and we append `-devel_<commit>` or
 //     `-devel_<change>_<patchset>` to the version.

CC @golang/release, @prattmic.

@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 14, 2025
@dmitshur dmitshur added this to the Unreleased milestone Apr 14, 2025
@gabyhelp gabyhelp added the ToolProposal Issues describing a requested change to a Go tool or command-line program. label Apr 14, 2025
@prattmic
Copy link
Member

I am perfectly happy with the go1.X-devel... format, but I feel fairly strongly that golangbuild should generate a version in the same format used by cmd/dist (https://cs.opensource.google/go/go/+/master:src/cmd/dist/build.go;l=439;drc=b16c04f43993436f24b1e4155a4652193eb1b90c) for standard builds [1].

So this change to golangbuild sounds good if we make the same change to cmd/dist, otherwise it seems we should change golangbuild to be in line with dist.

[1] Why does golangbuild need to generate a VERSION file at all?

@dmitshur
Copy link
Contributor Author

dmitshur commented Apr 15, 2025

Yes, agreed, we should apply this to cmd/dist too. Retitled to capture that too.

As for why golangbuild (and coordinator, previously) generates a VERSION file, it does it for a few related reasons:

  • cmd/dist's findgoversion logic that creates a devel version with the commit hash can only work if 1) .git directory is present, and 2) a git binary is available. This might not be the case for all builders; they might opt to test a GOROOT tree without the .git directory (if that's faster) and some builders may not have a git binary in PATH.

  • Next, it's meant to be a (probably small) optimization: the build system already did the work of figuring out what the commit hash (or change ID/patch set number) it's testing, so it can more quickly write that information down into the VERSION file to prevent the somewhat-expensive findgoversion logic from needing to re-compute the same thing.

  • Finally, sometimes the build system has more information readily available, like knowing that what the CL number and patch set is for pre-submit runs. Another aspect is that the current release process leaves a stale VERSION file behind on release branches, which would be more misleading if the build system re-used as-is.

@dmitshur dmitshur changed the title x/build/cmd/golangbuild: switch to generating a valid (per go/version.IsValid) version string cmd/dist, x/build/cmd/golangbuild: switch to a devel version format that satisfies go/version.Lang and go/version.IsValid in more cases Apr 15, 2025
@xnox
Copy link
Contributor

xnox commented Apr 24, 2025

There is also need to identify downstream golang toolchains, their vendor, and patch level. In a way that is encoded in the binaries built by those toolchains. And then distributed to another party and be able to link it all back.
See the dismissed proposal at #73194 which talks about that. Partially related.

I wonder, if it is best to keep the top level version as short as possible, and still semver compliant. And have more verbose and structured information about the toolchain in the buildinfo, which propagates to the binaries.

$ go version -m somegobinary 
somegobinary: go1.25-devel
...
	build	toolchain.vcs=git
	build	toolchain.vcs.revision=1c075e12cf123600b57cfb8bb66b9aa107a37e08
	build	toolchain.vcs.time=2025-04-11T19:28:28Z
	build	toolchain.vcs.modified=false
	build	toolchain.stable=false (or .released or .devel or .snapshot or whatever name makes most sense)

Similarly this would be very useful for downstreams to embed toolchain.vendor and toolchain.vendor-version. Very often many parties are involved as to who creates the golang toolchain, who builds the binaries, by whom/where they are deployed. If one uses a linux distribution golang fork with backported CVEs today, builds go binaires, puts them into container, and pushes it to dockerhub or as a UBI, and somebody else deploys them, it is impossible to tell from the go binary alone which golang was used and if it was 1.23.3 from upstream, or a particular linux vendor, which vendor, and if it has backported CVE fixes.

Not too dissimilar with being able to identify snapshot builds, and pre-release builds.

If you can, please consider adding structured data about the toolchain to the buildinfo that persists in binaries beyond the top level "go1.x" string.

@dmitshur
Copy link
Contributor Author

dmitshur commented Apr 24, 2025

@xnox Including additional fields about the toolchain and its possible modifications in the build information (in addition to the go version string) like that is an interesting idea. It has some connection with the idea of having go mod verify also verify the toolchain against those that can be downloaded, which came up but I'm not sure if it has a tracking issue. However, I suspect you'll run into limitations of how useful it can be: many installed toolchains will not have VCS information, and if they do, toolchain.vcs.revision doesn't communicate much if the commit hash is an unpublished local commit. There may be more open questions, and it's out of scope for this issue, so I suggest you file a separate issue for it if you'd like to pursue it further.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/667955 mentions this issue: cmd/dist: add "devel" substring check to isRelease computation

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/668015 mentions this issue: cmd/dist: move "devel" substring in git-inferred development version

gopherbot pushed a commit that referenced this issue Apr 25, 2025
Non-release versions that are built from source without a VERSION file
specifying any particular version end up with a development version like
"devel go1.25-67e0681aef Thu Apr 24 12:17:27 2025 -0700". Right now
those versions are correctly determined to be non-release because they
don't have a "go" prefix, instead they have a "devel " prefix.

In preparation of being able to move the "devel" substring, add a check
that said substring isn't present anywhere, since it is certain not to
be included in any released Go version we publish at https://go.dev/dl/.

For #73372.

Change-Id: Ia3e0d03b5723d4034d6270c3a2224f8dfae380e9
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/667955
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
@dmitshur dmitshur self-assigned this Apr 25, 2025
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 25, 2025
@dmitshur dmitshur moved this to In Progress in Go Release Apr 25, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/668355 mentions this issue: cmd/go, cmd/internal/objabi: detect "devel" version by substring

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/669016 mentions this issue: go/analysis/unitchecker: update TestVetStdlib's skip for "devel" move

gopherbot pushed a commit to golang/tools that referenced this issue Apr 30, 2025
The intention of TestVetStdlib is to run and potentially fail only at
Go tip, where standard library code can be relatively easily modified.
The current skip logic relies on the property that the "devel" string,
something that we aim to include in all non-release versions, happens
to be a prefix only at Go tip. That won't be the case after the issue
go.dev/issue/73372 is resolved, so update the skip to keep working as
originally intended.

For golang/go#73372.

Change-Id: I122439a590782f86fc3b3a8ba4b3adbfee1e591f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/669016
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit that referenced this issue Apr 30, 2025
These were the remaining instances in the main Go repo I found where a
Go version like "devel go1.25-9ce47e66e8 Wed Mar 26 03:48:50 2025 -0700"
is considered to be a development version rather than a release version,
but the version "go1.25-devel_9ce47e66e8 Wed Mar 26 03:48:50 2025 -0700"
is not.

Update this in preparation of the move of "devel" from front to middle.

For #73372.
For #73369.

Change-Id: If5442ecb0751c08b3a1b4d1148193e501700b956
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/668355
Reviewed-by: Michael Matloob <matloob@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@dmitshur dmitshur modified the milestones: Unreleased, Go1.25 Apr 30, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Release Apr 30, 2025
@mvdan
Copy link
Member

mvdan commented May 2, 2025

I was struggling with this just last week - thanks so much for this change!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. ToolProposal Issues describing a requested change to a Go tool or command-line program.
Projects
Status: Done
Development

No branches or pull requests

6 participants