Use go1.16 "embed" to set version from VERSION file

[thaJeztah]

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), but not yet implemented.

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

[thaJeztah]

(This is expected to fail on go1.15 in ci)

[cyphar]

👍 I like this and it's just a simple as I thought when I suggested it the last time we futzed with the version string. Hopefully once we drop Go 1.15 support this is something we can use unconditionally. In the mean time, what if we made this a go1.16-gated thing so that go get users can get an accurate version for newer Go versions?

[thaJeztah]

I'm a big fan of //go:embed already. The only thing I'm missing is an option to compress the embedded files (at least, I don't think the embedded files are optimised in any way). For this particular use, it's perfect though.

In the mean time, what if we made this a go1.16-gated thing so that go get users can get an accurate version for newer Go versions?

I was considering that (have //+build go116 and //+build !go116 files), but thought it may be adding more complexity than it solves.

I can push a commit later today to see what it would look like, then we can decide.

[thaJeztah]

@kolyshkin @cyphar @AkihiroSuda ptal; I moved this one out of draft, now that we dropped Go < 1.16 on the master/main branch.

Given that this only affects the binary (i.e., requires the binary to be built with go 1.16+), I'm not sure if we need to implement a fallback for older Go versions; consumers of packages/modules provided in this repository should not be affected, but I did increment the go version in the go.mod (which currently only acts as a "recommendation", but not enforced).

I also verified that overriding the version using -X main.version is still possible (which could still be useful for distro-packagers who want to add a suffix to the version);

go build -trimpath "-buildmode=pie" -ldflags "-X main.gitCommit=v1.0.0-133-g27d75cca -X main.version=1.0.3~mydistro" -o runc .
./runc --version
runc version 1.0.3~mydistro
commit: v1.0.0-133-g27d75cca
spec: 1.0.2-dev
go: go1.17.3

[kolyshkin]

Nice! LGTM

[kolyshkin]

Actually, this adds an extra newline after the version string.

Before:

[kir@kir-rhat runc]$ ./runc --version
runc version 1.0.0+dev
commit: v1.0.0-415-g0c0ec3f5
spec: 1.0.2-dev
go: go1.17.3
libseccomp: 2.5.3

After:

[kir@kir-rhat runc]$ ./runc --version
runc version 1.0.0+dev

commit: v1.0.0-415-g0c0ec3f5-dirty
spec: 1.0.2-dev
go: go1.17.3
libseccomp: 2.5.3

I guess this needs to be fixed.

[thaJeztah]

oh! good catch. hmm I guess the VERSION file has a newline; we could remove it there, or when printing 🤔

[kolyshkin]

we could remove it there, or when printing

I vote for the latter. It's easy to re-introduce a newline to VERSION file. I guess a simple strings.TrimSpace(version) would do.

[thaJeztah]

Yes, I was a bit in doubt if the string would be used elsewhere, but I guess it's only for printing, and in that case trimming it seems indeed a good solution to me

[tianon]

I know you looked this up too, but I wanted to record my research on Go's new -buildvcs flag and how irritating it's going to be for us to use:

• it's included in BuildInfo.Settings (which is a slice of key-value pairs); the key isn't documented anywhere officially that I can find, but it does appear to be set in https://github.com/golang/go/blob/6673d5d701d224919d910e88a73b6f69472c9ed1/src/cmd/go/internal/load/pkg.go#L2427-L2435 (vcs.revision for the most interesting/useful-to-us value)

• BuildInfo.Settings was also new in 1.18: https://pkg.go.dev/runtime/debug@go1.17.8#BuildInfo

So to even consider using it, we'll have to have a go1.18+ file (or some fancy reflection that's probably not worth it IMO).

[kolyshkin]

LGTM

[thaJeztah]

Oh! Let me reply here as well on @tianon's comment (thanks!). So it turned out we were both looking at "how to use the go1.18 features" for the remaining bits; I started to work on a follow up (for when we no longer have to support go1.17); didn't get far yet, but the change in this PR should likely be good as a starting point (small steps)

[kolyshkin]

the change in this PR should likely be good as a starting point

Right, I see that what is here can be merged as is, and since the second part requires go >= 1.18 (which, practically, means we need to wait until go 1.19 is released), we can do that later (about 6 months later or so).

[tianon]

This looks pretty safe still, right? 👀

(even the docs for the fancy new thing I complained about are better now: https://pkg.go.dev/runtime/debug@go1.20.2#BuildSetting)

[AkihiroSuda]

LGTM, but needs rebase

[kolyshkin]

@thaJeztah can you rebase this one? I think we can merge it as soon as it's all green.

[kolyshkin]

@thaJeztah can you rebase this one? I think we can merge it as soon as it's all green.

[thaJeztah]

Rebased 👍 Thanks for the ping!

[akhilerm]

With this change will we still need the VERSION variable in Makefile. ?

Another concern is, the patch will make changes in 9d9273c ineffective. The use case was someone should be able to pass a custom version without editing the VERSION file.

[kolyshkin]

@thaJeztah it looks like VERSION should be removed from the Makefile altogether.

Also, this needs another rebase.