-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use go1.16 "embed" to set version from VERSION file #3163
base: main
Are you sure you want to change the base?
Conversation
(This is expected to fail on go1.15 in ci) |
👍 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 |
I'm a big fan of
I was considering that (have I can push a commit later today to see what it would look like, then we can decide. |
f09f5e5
to
f03bd33
Compare
@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 I also verified that overriding the version using 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
oh! good catch. hmm I guess the VERSION file has a newline; 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 |
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 |
f03bd33
to
58a6723
Compare
I know you looked this up too, but I wanted to record my research on Go's new
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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) |
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). |
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) |
LGTM, but needs rebase |
@thaJeztah can you rebase this one? I think we can merge it as soon as it's all green. |
@thaJeztah can you rebase this one? I think we can merge it as soon as it's all green. |
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>
58a6723
to
5baa9ec
Compare
Rebased 👍 Thanks for the ping! |
@@ -19,7 +19,7 @@ BUILDTAGS += $(EXTRA_BUILDTAGS) | |||
|
|||
COMMIT ?= $(shell git describe --dirty --long --always) | |||
VERSION ?= $(shell cat ./VERSION) | |||
LDFLAGS_COMMON := -X main.gitCommit=$(COMMIT) -X main.version=$(VERSION) | |||
LDFLAGS_COMMON := -X main.gitCommit=$(COMMIT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah it looks like VERSION
should be removed from the Makefile altogether.
Also, this needs another rebase.
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 beenaccepted 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
. Whenbuilding 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.:
With this patch (on go1.16):