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: stamp git/vcs current HEAD hash/commit hash/dirty bit in binaries #37475

Closed
bradfitz opened this issue Feb 26, 2020 · 51 comments
Closed

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Feb 26, 2020

(Related but different than #35667)

cmd/go currently embeds all the module dep information in binaries and it's readable with e.g. https://godoc.org/rsc.io/goversion/version but it does not include any information about the top-level module's version.

I propose that cmd/go look at {git,svn,etc} state and include in the binary:

  • HEAD commit time
  • HEAD hash
  • dirty bit (if there are uncommitted changes)

Currently many projects do this by hand with a build-program.sh and stamping it manually with --ldflags=-X foo=bar, but that means programs built the normal Go way lack that information, and people end up with non-portable (shell, often) build scripts.

I've hit this enough times with my own projects that it's actively frustrating me. It's worse when programs are clients that want to report their version number to a server (which might want to do analytics, build horizon enforcement, protocol version negotiation, etc) and then can't. There are alternative ways to do all that, but they're tedious.

Mostly I'm concerned that people have bespoke, often non-portable build scripts.

Note, July 15, 2021: This proposal was accepted but has not yet been implemented. Per discussion in #37693, when it does get implemented, we need to include a flag to turn this behavior off. - rsc

@gopherbot gopherbot added this to the Proposal milestone Feb 26, 2020
@rsc rsc added this to Incoming in Proposals (old) Feb 26, 2020
@rsc rsc moved this from Incoming to Active in Proposals (old) Feb 26, 2020
@bcmills
Copy link
Contributor

bcmills commented Feb 26, 2020

This may be a duplicate of #29814.

bradfitz added a commit to tailscale/tailscale that referenced this issue Feb 28, 2020
Maybe we'll auto-bump this with a bot over time.

See golang/go#37475 & golang/go#29814

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

We should figure out exactly what we want to record.
The other modules are all versions or pseudo-versions.
Should this one be too?
Can we find that quickly enough to be reasonable to run during every 'go build'?
(We can skip it during 'go test' like we skip dwarf.)
The go command already has code to turn a commit hash into a version; we should probably just use that same code and add a +modified if the working directory is modified.

It would be helpful to time how much overhead this would be in 'go build'.

@bcmills, do you have any numbers about how much time this would add?

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

BTW I agree it's a duplicate of #29814 but I'll keep using this one because it is marked as a proposal and already appeared in the minutes.

@bcmills
Copy link
Contributor

bcmills commented Mar 4, 2020

It would be helpful to time how much overhead this would be in 'go build'.

git status --porcelain=v2 in the go repository is around 50ms for me, and git log -n 1 is around 25ms.

go install cmd/go for me is 1.6s dirty, and 140ms clean. So assuming that we can run the VCS commands in parallel with builds for non-main packages, the latency hit should be negligible.

CC @jayconrod @matloob

@bcmills
Copy link
Contributor

bcmills commented Mar 4, 2020

Hmm, I realized that I didn't account for checking tags in the above calculations. Still, I expect those costs will be order-of-magnitude similar to any other git command.

@dottedmag
Copy link
Contributor

I'd like to point out a (maybe small) problem with this approach: changing the version of source code, but not the code itself will cause the binary to change.

Let me explain the use-case I have that will be broken by this change:

  • A number of binaries are built from a monorepo, each binary is packaged in Docker, and a Helm chart is generated for every binary. This chart includes Docker image ID, not the tag.
  • These Helm charts are installed to Kubernetes

If the Helm chart contents and the binaries don't change, then no upgrades are performed by Kubernetes.

If the version of the checkout is stamped into every Go binary, then this scheme crumbles and:

  • either a version needs to be purged from every binary before packaging it into a Docker image,
  • or some crazy scheme has to be invented: remove the version from the binary, take a checksum, check that there is no Docker image with this version as a label published in the registry, publish the image and label it, use this label in Helm charts.

@bradfitz
Copy link
Contributor Author

bradfitz commented Mar 5, 2020

@dottedmag, what if we made it conditional on importing a new package, say runtime/version, containing the accessor funcs to get at the info? Then if you don't use it, no change in behavior.

Would that work for your use case?

@mark-rushakoff
Copy link
Contributor

I'm in a practically identical case to @dottedmag.

Inevitably, somewhere in the monorepo we will (perhaps unintentionally) bring in a dependency that depends on a magic package that breaks deterministic builds.

I think for most common cases, having this proposal enabled by default would be preferable. For my use case, I would be satisfied if there was a documented way to opt out of it. We already are using -ldflags=-buildid= to force a consistent build ID as part of deterministic binaries, so another esoteric flag to opt out of recording VCS state would be completely acceptable.

@dottedmag
Copy link
Contributor

@dottedmag, what if we made it conditional on importing a new package, say runtime/version, containing the accessor funcs to get at the info? Then if you don't use it, no change in behavior.

I agree with @mark-rushakoff: relying on imports will be brittle unless this import is considered only for main packages.

It's not an author of some recursively included library, but a builder of a final binary who in a position to decide whether to put versioning information into the binary or not.

@bcmills
Copy link
Contributor

bcmills commented Mar 5, 2020

@dottedmag, note that many functionally-equivalent builds will already produce slightly different binaries due to the version-stamping for runtime/debug.ReadBuildInfo. The version stamps will change with each change to the corresponding module versions, even if the contents of the specific imported packages are the same.

This proposal would case more of the same sort of version churn, but it is fundamentally the same churn.

That suggests that we may want to provide an option to disable version stamping in general. IMO, that should be a separate proposal.

@dottedmag
Copy link
Contributor

True. In practice, it is not a problem as changing the versions of dependencies nearly always changes the code of dependencies — nobody is updating versions of dependencies endlessly for no reason, usually, they only get updated to get a new feature or a bugfix.

Filed #37693.

@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

The discussion above about reproducible builds sounds like it would be satisfied by having the version embedded by default but also having an opt-out command-line flag; no special package needed.

Do I have that right, @dottedmag and @mark-rushakoff?

@mark-rushakoff
Copy link
Contributor

Yes, I think a flag to opt out of embedding version details would suffice for reproducible builds.

It would be nice if there was a single flag like -reproducible to omit version details and to set a fixed build ID, but it is probably fine if those remain separate concerns.

I don't care about reproducible builds when I'm at the command line building something for my own use; I care about reproducible builds when I am writing build scripts that run as part of a CI/CD pipeline, so it is not a big deal if I need to look up the whole collection of settings to make those builds reproducible.

@dottedmag
Copy link
Contributor

@rsc Correct.

@rsc
Copy link
Contributor

rsc commented Apr 1, 2020

OK, it sounds like everyone agrees about doing this by default, with a flag to turn it off.
It is unclear exactly how fast it will be to ask git/etc what we need to know, but that operation can overlap with the entire build, including the link. Right now if I build helloworld I get:

/Users/rsc/go/pkg/tool/darwin_amd64/link -o $WORK/b001/exe/a.out -importcfg $WORK/b001/importcfg.link -buildmode=exe -buildid=SqhXBrEZODPkt1gG-6fj/H70nrokRrHQB9NgKyehx/mN9hM1-9avNnPeQfRwgY/SqhXBrEZODPkt1gG-6fj -extld=clang $WORK/b001/_pkg_.a
/Users/rsc/go/pkg/tool/darwin_amd64/buildid -w $WORK/b001/exe/a.out # internal

That buildid step could install the git version info too. I'm confident git will be faster than the link.

Based on the discussion, then, this seems like a likely accept, although we may not be able to implement it until the next release (Go 1.16).

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Apr 1, 2020
@seankhliao
Copy link
Member

Will this include just the commit hash or also a (any?) version tag

@liggitt
Copy link
Contributor

liggitt commented Apr 1, 2020

Version tags introduce many sharp edges, at least for git… since you can have a git repo cloned without having fetched all tags, or can have different local tags, or can add a tag to a SHA at any point in time, using the nearest tag (e.g. git describe or equivalent) would mean that a build of the same SHA could result in different embedded versions for different people (or for the same person at different times). Further, if the closest tagged commit has multiple tags associated with it, the version could be ambiguous (git describe has particularly unpredictable behavior in this case).

thaJeztah added a commit to thaJeztah/runc that referenced this issue Dec 2, 2021
This allows building runc without passing the VERSION build-arg, but requires
go 1.16 or up.

Unfortunately, there's no easy way to get the git-commit from the filesystem
(possibly through `.git/logs/HEAD`, which is a large file); a proposal has been
accepted to add git information ([1]), which will be included in go1.18.

For users building through `go install github.com/opencontainers/runc@version`),
we may be able to use runtime/debug.BuildInfo() ([2]). BuildInfo provides the
module version and checksum, but only when building using `go install`. When
building from a local module (git clone), the version is alway set to `(devel)`,
see [3].

We could consider add module (optional) if available, e.g.:

    // Print module information if available. When built from local source,
    // (using git clone), module version is not available, and version is
    // set to "(devel)"; see golang/go#29814, and golang/go#37475.
    if bi, ok := debug.ReadBuildInfo(); ok && bi.Main.Version != "(devel)" {
        v = append(v, "module version: "+bi.Main.Version+", sum: "+bi.Main.Sum)
    }

With this patch (on go1.16):

    make
    go build -trimpath "-buildmode=pie"  -tags "seccomp" -ldflags "-X main.gitCommit=v1.0.0-133-g27d75cca " -o runc .

    ./runc --version
    runc version 1.0.0+dev

    commit: v1.0.0-133-g27d75cca
    spec: 1.0.2-dev
    go: go1.16.7
    libseccomp: 2.3.3

[1]: golang/go#37475
[2]: https://pkg.go.dev/runtime/debug#BuildInfo
[3]: golang/go#29814

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@gopherbot
Copy link

Change https://golang.org/cl/373875 mentions this issue: doc/go1.18: add section for runtime/debug changes

gopherbot pushed a commit that referenced this issue Dec 23, 2021
For #37475
For #47694

Change-Id: If8c1f1b756daf32648110f1a669b2ea60f797a24
Reviewed-on: https://go-review.googlesource.com/c/go/+/373875
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
josharian added a commit to tailscale/go that referenced this issue Feb 7, 2022
Temporary fix until golang#37475 is done.

(cherry picked from commit ba89304)

Change-Id: I204f03bc62c6f2295a8bc66d3f0bb2132153bc27
@gopherbot
Copy link

Change https://go.dev/cl/391803 mentions this issue: debug/buildinfo: use testenv.GoToolPath instead of resolving "go" from $PATH

@gopherbot
Copy link

Change https://go.dev/cl/391809 mentions this issue: cmd/go: stamp build settings for binaries in cmd

gopherbot pushed a commit that referenced this issue Mar 14, 2022
… "go" from $PATH

Updates #37475.

Change-Id: I8c3237438da3e9521ce3be26a0b5d5ca36944b17
Reviewed-on: https://go-review.googlesource.com/c/go/+/391803
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Trust: Daniel Martí <mvdan@mvdan.cc>
gopherbot pushed a commit that referenced this issue Mar 18, 2022
Also update cmd/dist to avoid setting gcflags and ldflags explicitly
when the set of flags to be set is empty (a verbose way of specifying
the default behavior).

Stamping was disabled for the Go standard library in CL 356014 due to
the cmd/dist flags causing cmd/go to (correctly) report the resulting
binaries as stale.

With cmd/dist fixed, we can also remove the special case in cmd/go,
which will allow tests of binaries in 'cmd' to read the build info
embedded in the test binary. That build info may be useful to
determine (say) whether runtime.GOROOT ought to work without GOROOT
set in the environment.

For #51483
Updates #37475

Change-Id: I64d04f5990190094eb6c0522db829d3bdfa50ef3
Reviewed-on: https://go-review.googlesource.com/c/go/+/391809
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
thaJeztah added a commit to thaJeztah/runc that referenced this issue Mar 21, 2022
This allows building runc without passing the VERSION build-arg, but requires
go 1.16 or up.

Unfortunately, there's no easy way to get the git-commit from the filesystem
(possibly through `.git/logs/HEAD`, which is a large file); a proposal has been
accepted to add git information ([1]), which will be included in go1.18.

For users building through `go install github.com/opencontainers/runc@version`),
we may be able to use runtime/debug.BuildInfo() ([2]). BuildInfo provides the
module version and checksum, but only when building using `go install`. When
building from a local module (git clone), the version is alway set to `(devel)`,
see [3].

We could consider add module (optional) if available, e.g.:

    // Print module information if available. When built from local source,
    // (using git clone), module version is not available, and version is
    // set to "(devel)"; see golang/go#29814, and golang/go#37475.
    if bi, ok := debug.ReadBuildInfo(); ok && bi.Main.Version != "(devel)" {
        v = append(v, "module version: "+bi.Main.Version+", sum: "+bi.Main.Sum)
    }

With this patch (on go1.16):

    make
    go build -trimpath "-buildmode=pie"  -tags "seccomp" -ldflags "-X main.gitCommit=v1.0.0-133-g27d75cca " -o runc .

    ./runc --version
    runc version 1.0.0+dev

    commit: v1.0.0-133-g27d75cca
    spec: 1.0.2-dev
    go: go1.16.7
    libseccomp: 2.3.3

[1]: golang/go#37475
[2]: https://pkg.go.dev/runtime/debug#BuildInfo
[3]: golang/go#29814

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@andig
Copy link
Contributor

andig commented Jul 25, 2022

That seems like a reasonable motivation to include both the commit hash and semantic version, rather than just one or the other.

@bcmills I would read this as "both commit hash and tag" will be implemented, or rather are since the PR is merged. Looking at the buildinfo from 1.18, only the commit hash is present.

Is there further discussion or efforts for adding the (most recent) tag?

@seankhliao
Copy link
Member

@andig #50603

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests