Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION #4370

rata · 2024-08-08T10:16:07Z

This reverts commit 9d9273c. (PR #4270 and #4269) and adds a VERSION_EXTRA make variable (that is not read from env variables) to append a custom suffix for the version.

The reverted commit broke the build for several other projects (see comments here: #4270, after the merge) and we don't really need this to be able to set the version without changing the file.

With this commit reverted, we can still run:

make VERSION="1.2.3"

and it just works. It doesn't take it from an env variable, but that is what broke all the other projects (VERSION is just too generic as an env var, especially for a project like runc that is embedded in many others).

Also, to simplify just adding a custom suffix to the version, a VERSION_EXTRA is added. This seems to be what several users overriding the version need.

@akhilerm my understanding is that, without that commit, everything should just work. Let me know if this doesn't work for you.

For example:

$ make VERSION="hola"
rm -f libcontainer/dmz/binary/runc-dmz
go generate -tags "seccomp urfave_cli_no_docs " ./libcontainer/dmz
make[1]: Entering directory '/home/rodrigo/src/github.com/opencontainers/runc/libcontainer/dmz'
gcc -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc -static -o binary/runc-dmz _dmz.c
strip -gs binary/runc-dmz
make[1]: Leaving directory '/home/rodrigo/src/github.com/opencontainers/runc/libcontainer/dmz'
go build -trimpath "-buildmode=pie"  -tags "seccomp urfave_cli_no_docs " -ldflags "-X main.gitCommit=v1.2.0-rc.2-30-gba613c3d -X main.version=hola " -o runc .

$ ./runc --version
runc version hola
commit: v1.2.0-rc.2-30-gba613c3d
spec: 1.2.0
go: go1.22.5
libseccomp: 2.5.5

cpuguy83 · 2024-08-08T13:18:22Z

I'm off for a couple of weeks but saw this pop up.
I fear this will re-break my builds but need to test (which I won't be doing for a couple of weeks 🥰).

rata · 2024-08-08T13:32:05Z

@cpuguy83 I can leave this open for you to get back with the results.

Before this PR, the env var VERSION overrides the version runc prints in runc --version. My guess is that most people don't want to override it anyways and the fixes to workaround that don't rely on the env var being honored.

But there is only one way to know ;). Please just update here when you test and enjoy your holidays now! :)

thaJeztah · 2024-08-08T13:36:33Z

The make var vs env-var is indeed always a fun one (specifically with such generic names); does the same currently apply to COMMIT as well?

Perhaps namespaced (RUNC_VERSION, RUNC_COMMIT) ones could still work for env-vars (if environment-variables were the motivation)?

rata · 2024-08-09T11:01:25Z

@thaJeztah the commit said the change was done to be able to override the version, without changing the file. But that is possible without reading env vars, just using make VERSIOM="test", as I mentioned.

I don't see why we would want to still use env vars. Am I missing something?

kolyshkin · 2024-08-10T02:57:05Z

One thing I saw is official containerd packed for debian, which contains runc binary, has runc --version showing the version of containerd. I guess this happens because make takes VERSION from environment, which is not good.

I might be mistaken, but it seems to be that #4269 fixes those kind of weird issues. At the very least, we should move to RUNC_VERSION.

kolyshkin · 2024-08-10T03:01:44Z

Sorry I did not look deep enough. I though we're embedding VERSION file already, and this PR is about reverting that functionality. It appears that #3163 is still not merged.

Before 1.2.0 goes out we have to switch to one way or another. Maybe #3163 is the best (and those who want to change version should do so explicitly by writing the VERSION file).

akhilerm · 2024-08-10T08:05:38Z

@rata Using make VERSION=x.y.z without the commit fixes my use case. My bad, I should have first thought of doing it that way .

(and those who want to change version should do so explicitly by writing the VERSION file).

That means users will have to create an additional commit for the change in the VERSION file, else the build will be happening on a dirty git tree.

kolyshkin · 2024-08-11T17:45:37Z

(and those who want to change version should do so explicitly by writing the VERSION file).

That means users will have to create an additional commit for the change in the VERSION file, else the build will be happening on a dirty git tree.

If you're not changing anything, there's probably no need to change VERSION either. If you change something, you can change the VERSION file as well. Or am I missing anything? Can you tell me more about your use case?

akhilerm · 2024-08-11T17:53:56Z

If you're not changing anything, there's probably no need to change VERSION either. If you change something, you can change the VERSION file as well. Or am I missing anything? Can you tell me more about your use case?

In my case, there is no change to the source code, but want to append a build number along with the version string. eg: v1.1.13+build.N.

cpuguy83 · 2024-08-11T18:19:04Z

Yep, same we append the build number which is what we mess with it at all.

rata · 2024-08-12T08:26:23Z

Okay, so it makes sense to provide a way to override it without changing the source code.

The linked issues that were caused by reading the VERSION env var (https://github.com/containerd/containerd/pull/10556/files and https://github.com/docker/containerd-packaging/pull/385/files) should work fine with this revert too, no changes needed there. Those PRs were about unsetting the version var, so that is fine with this too.

Let's wait to @cpuguy83 to see if that work-flow will be broken by this or not. I understand @akhilerm use case is fine with this revert too.

lifubang · 2024-08-12T10:40:08Z

How about add a comment in README.md to tell users how to customize RUNC's version string? For example:

#### Version String Customize
1. Using make command options: make VERSION="1.1.13-build#1";
2. Using environment variable(If you confirm you can trust all the env vars): VERSION="1.1.13-build#1" make -e.

Though the users should know how to rewrite make vars, but in some moments we may forget these basic knowledge, so I think it will be better to record it in the document.

thaJeztah · 2024-08-12T10:50:11Z

I don't see why we would want to still use env vars. Am I missing something?

I don't think I personally need it, but I don't know if others expected it to work through env-vars, which could be (e.g.) when they build using a Dockerfile, and pass these as --build-arg.

The thing I noticed was that the problem we have with VERSION already could be the same for COMMIT, which also isn't prefixed with anything, so could easily be present for something else, and now result being consumed by runc as well;

docker build --progress=plain --no-cache -<<'EOF'
FROM golang:1.21
RUN apt-get update && apt-get install -y --no-install-recommends \
    dpkg-dev \
    gcc \
    libc6-dev \
    libseccomp-dev \
    pkg-config

WORKDIR /go/src/github.com/opencontainers/runc
ENV RUNC_VERSION=v1.1.13
ENV VERSION=1.99.0
ENV COMMIT=deadbeef
RUN <<EOT
  set -e
  git init .
  git remote add origin "https://github.com/opencontainers/runc.git"
  git fetch -q --depth 1 origin "${RUNC_VERSION}" +refs/tags/*:refs/tags/*
  git checkout -q FETCH_HEAD
  CGO_ENABLED=1 make static
  ./runc --version
EOT
EOF

Notice how both VERSION and COMMIT were picked up by the runc makefile;

#8 1.737 go build -trimpath -buildmode=pie  -tags "seccomp netgo osusergo" -ldflags "-X main.gitCommit=deadbeef -X main.version=1.99.0 -linkmode external -extldflags --static-pie " -o runc .
#8 6.972 runc version 1.99.0
#8 6.972 commit: deadbeef
#8 6.972 spec: 1.0.2-dev
#8 6.972 go: go1.21.13
#8 6.972 libseccomp: 2.5.4
#8 DONE 7.1s

So having a namespaced one (for env-vars at least) could be something to consider;

At the very least, we should move to RUNC_VERSION.

rata · 2024-08-12T11:45:11Z

@thaJeztah I understand we are reading env vars for more things. I agree COMMIT (and IMHO also BUILDTAGS) are candidates to either stop doing it or don't use such a generic name (a runc_ prefix as you suggest, or something).

I was trying to fix "the obvious", what broke people builds. The thing with COMMIT is that we are doing that for more than 3 years, I think we can decide if we want to fix that in another PR, as that might be less obvious what to do.

@lifubang I don't see the value of reading such a generic env var (VERSION) that broke the setup of several people. Why wouldn't we fix it? They can still override it without changing the source code.

lifubang · 2024-08-13T09:37:18Z

@lifubang I don't see the value of reading such a generic env var (VERSION) that broke the setup of several people. Why wouldn't we fix it? They can still override it without changing the source code.

Oh, what I mean is that besides this revert commit, I suggested to add a comment in readme.
I should say more clearly.

lifubang · 2024-08-13T09:44:21Z

Because Readme should provide the information to let users know which way is the best way we suggested to customize the version string in runc.

rata · 2024-08-13T11:34:59Z

Ohh, I see. I'm fine adding it to the README or some other docs, if this change looks fine code-wise :)

rata · 2024-08-13T14:45:30Z

@lifubang added. I ended up adding one of the options, as you can use env vars for the content with that way already (like make VERSION=$VERSION). PTAL

README.md

kolyshkin · 2024-08-14T02:32:52Z

According to what @cpuguy83 and @akhilerm said, we don't need VERSION at all. We need something like VERSION_EXTRA (or VERSION_SUFFIX etc.) as a way to append something to VERSION.

Changing the runc version is generally a bad thing as it might confuse users. Appending some arbitrary suffix is fine.

kolyshkin · 2024-08-14T02:58:29Z

@cpuguy83, @akhilerm, is something like this be sufficient for your use cases?

[kir@kir-tp1 runc]$ diff --git a/Makefile b/Makefile
index 94b9a3c3..f33e363c 100644
--- a/Makefile
+++ b/Makefile
@@ -18,7 +18,7 @@ BUILDTAGS ?= seccomp urfave_cli_no_docs
 BUILDTAGS += $(EXTRA_BUILDTAGS)
 
 COMMIT ?= $(shell git describe --dirty --long --always)
-VERSION ?= $(shell cat ./VERSION)
+VERSION := $(shell cat ./VERSION)$(RUNC_VERSION_EXTRA)
 LDFLAGS_COMMON := -X main.gitCommit=$(COMMIT) -X main.version=$(VERSION)
 
 GOARCH := $(shell $(GO) env GOARCH)
[kir@kir-tp1 runc]$ make RUNC_VERSION_EXTRA=.build-42 runc
rm -f libcontainer/dmz/binary/runc-dmz
go generate -tags "seccomp urfave_cli_no_docs " ./libcontainer/dmz
make[1]: Entering directory '/home/kir/git/runc/libcontainer/dmz'
gcc -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc -static -o binary/runc-dmz _dmz.c
strip -gs binary/runc-dmz
make[1]: Leaving directory '/home/kir/git/runc/libcontainer/dmz'
go build -trimpath "-buildmode=pie"  -tags "seccomp urfave_cli_no_docs " -ldflags "-X main.gitCommit=v1.2.0-rc.2-32-g9a2176c1-dirty -X main.version=1.2.0-rc.2+dev.build-42 " -o runc .
[kir@kir-tp1 runc]$ ./runc --version
runc version 1.2.0-rc.2+dev.build-42
commit: v1.2.0-rc.2-32-g9a2176c1-dirty
spec: 1.2.0
go: go1.22.5
libseccomp: 2.5.5

rata · 2024-08-15T09:17:40Z

@kolyshkin thanks, fixed, PTAL!

I left the VERSION_EXTRA as a make var, as I don't really see any benefit of it being an env var, and you can still use env vars for its value, like: make VERSION_EXTRA=$DEBIAN_PACKAGE_REVISION.

And IMHO there are potential drawbacks, like distros uses git or other SCMs for managing the package (that includes their files to build and all), so the env var "RUNC_VERSION" is not so clear what it means in that context: I wouldn't be suprised if some have that for the package version and "RUNC_UPSTREAM_VERSION" for the upstream part or whatever. I think it is limiting for no good reason.

kolyshkin

LGTM, thanks!

We should probably change COMMIT and BUILDTAGS from using ?= to := (as they are runc-specific), and keep GO, PREFIX, and GPG_KEYID (as they are not runc specific and can be used elsewhere).

@thaJeztah WDYT?

rata · 2024-08-15T09:21:44Z

@kolyshkin I was planning to open a different PR with that once this is merged. Those and also EXTRA_BUILDTAGS

thaJeztah

this change LGTM

I'd be open to (for future releases) further cleaning up some of this (which could still include prefixes used).

This change looks good to me though!

README.md

thaJeztah · 2024-08-15T13:57:10Z

For those interested in this topic; I once dug up some POSIX recommendations for these formats, which also had recommendations for "how to include package name"; linked some of them in containerd/containerd#6495 (comment).

Which basically drilled down to; the first line must have the version; additional lines are optional (and informational), and the first line to be formatted;

<name of tool> (package name and information) version

Some parts of those made a lot of sense; i.e. the actual version should be the last element on the first line of the output (so that tools can take the first line, split off the last whitespace-delimited element to get the version), but while looking at those, I also found that most tools don't stick to any standard, and output is all over the place.

rata · 2024-08-15T14:28:09Z

I'd like to wait for @cpuguy83 to confirm this doesn't re-break their setup. But I feel even if that is the case, this is the right move, 1.1.13 will probably break more users in the future the way it is (or some might had not realized yet that the output is wrong).

We recently switched VERSION to be read from env vars (opencontainers#4270). This broke several projects, as they were building runc and using a `VERSION` env var for, e.g. the containerd version. When fixing that in opencontainers#4370, we discussed to consider doing the same for these variables too (opencontainers#4370 (review)). Let's stop reading them from env vars, as it is very easy to do it by mistake (e.g. compile runc and define a COMMIT env var, not to override the commit shown in `runc --version`) and users that want can still override them if they want to. For example, with: make EXTRA_BUILDTAGS=runc_nodmz Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

kolyshkin · 2024-08-15T14:55:47Z

@kolyshkin I was planning to open a different PR with that once this is merged. Those and also EXTRA_BUILDTAGS

Now that you mention it, we have EXTRA_BUILDTAGS and EXTRA_FLAGS, so maybe this one should be EXTRA_VERSION for the sake of uniformity.

thaJeztah · 2024-08-15T15:02:54Z

Good catch; I'm good with EXTRA_VERSION if we want the consistency

rata · 2024-08-15T16:05:56Z

heh, I was in doubt, but this decides it. Updated to EXTRA_VERSION :)

Add this new make variable so users can specify build information without modifying the runc version nor the source code. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

thaJeztah

still LGTM

rata · 2024-08-16T13:38:31Z

Ouch, I wanted to wait for @cpuguy83 (#4370 (comment)). @cpuguy83 sorry it is merged already, please do test and update here how it goes when you are back :)

cpuguy83 · 2024-08-23T19:42:28Z

EXTRA_VERSION SGTM

We recently switched VERSION to be read from env vars (opencontainers#4270). This broke several projects, as they were building runc and using a `VERSION` env var for, e.g. the containerd version. When fixing that in opencontainers#4370, we discussed to consider doing the same for these variables too (opencontainers#4370 (review)). Let's stop reading them from env vars, as it is very easy to do it by mistake (e.g. compile runc and define a COMMIT env var, not to override the commit shown in `runc --version`) and users that want can still override them if they want to. For example, with: make EXTRA_BUILDTAGS=runc_nodmz Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

rata assigned kolyshkin and AkihiroSuda Aug 8, 2024

rata added backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 easy-to-review labels Aug 8, 2024

rata force-pushed the main branch from ba613c3 to 5c4f79d Compare August 13, 2024 14:44

rata requested a review from lifubang August 13, 2024 14:45

rata force-pushed the main branch from 5c4f79d to 624dd4e Compare August 13, 2024 16:53

kolyshkin mentioned this pull request Aug 13, 2024

cpu burst test failures on Ubuntu 24.04 #4373

Closed

kolyshkin reviewed Aug 14, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

rata force-pushed the main branch from 624dd4e to e0f0f40 Compare August 14, 2024 10:56

rata requested a review from kolyshkin August 15, 2024 09:12

kolyshkin approved these changes Aug 15, 2024

View reviewed changes

akhilerm approved these changes Aug 15, 2024

View reviewed changes

thaJeztah approved these changes Aug 15, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

rata mentioned this pull request Aug 15, 2024

Makefile: Don't read COMMIT, BUILDTAGS, EXTRA_BUILDTAGS from env vars #4380

Merged

rata force-pushed the main branch from 145c707 to d008a90 Compare August 15, 2024 16:04

Makefile: Add EXTRA_VERSION

cc2078c

Add this new make variable so users can specify build information without modifying the runc version nor the source code. Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>

rata force-pushed the main branch from d008a90 to cc2078c Compare August 15, 2024 16:06

rata added the impact/changelog label Aug 15, 2024

rata changed the title ~~Revert "allow overriding VERSION value in Makefile" and add VERSION_EXTRA~~ Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION Aug 15, 2024

thaJeztah approved these changes Aug 15, 2024

View reviewed changes

kolyshkin merged commit 376e875 into opencontainers:main Aug 16, 2024
40 checks passed

rata added backport/done/1.1 A PR in main branch which was backported to release-1.1 and removed backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 labels Aug 19, 2024

rata mentioned this pull request Aug 19, 2024

[1.1] Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION #4382

Merged

lifubang mentioned this pull request Aug 31, 2024

Release v1.2.0-rc.3 #4389

Closed

github-actions bot mentioned this pull request Sep 19, 2024

Bump runc from v1.1.13 to v1.1.14 kokyhm/kubespray_dependencies_bump_playground#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION #4370

Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION #4370

rata commented Aug 8, 2024 •

edited

Loading

cpuguy83 commented Aug 8, 2024

rata commented Aug 8, 2024 •

edited

Loading

thaJeztah commented Aug 8, 2024

rata commented Aug 9, 2024

kolyshkin commented Aug 10, 2024

kolyshkin commented Aug 10, 2024

akhilerm commented Aug 10, 2024

kolyshkin commented Aug 11, 2024

akhilerm commented Aug 11, 2024 •

edited

Loading

cpuguy83 commented Aug 11, 2024

rata commented Aug 12, 2024

lifubang commented Aug 12, 2024 •

edited

Loading

thaJeztah commented Aug 12, 2024

rata commented Aug 12, 2024

lifubang commented Aug 13, 2024

lifubang commented Aug 13, 2024

rata commented Aug 13, 2024

rata commented Aug 13, 2024

kolyshkin commented Aug 14, 2024

kolyshkin commented Aug 14, 2024 •

edited

Loading

rata commented Aug 15, 2024

kolyshkin left a comment

rata commented Aug 15, 2024

thaJeztah left a comment

thaJeztah commented Aug 15, 2024

rata commented Aug 15, 2024 •

edited

Loading

kolyshkin commented Aug 15, 2024

thaJeztah commented Aug 15, 2024

rata commented Aug 15, 2024

thaJeztah left a comment

rata commented Aug 16, 2024

cpuguy83 commented Aug 23, 2024

Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION #4370

Revert "allow overriding VERSION value in Makefile" and add EXTRA_VERSION #4370

Conversation

rata commented Aug 8, 2024 • edited Loading

cpuguy83 commented Aug 8, 2024

rata commented Aug 8, 2024 • edited Loading

thaJeztah commented Aug 8, 2024

rata commented Aug 9, 2024

kolyshkin commented Aug 10, 2024

kolyshkin commented Aug 10, 2024

akhilerm commented Aug 10, 2024

kolyshkin commented Aug 11, 2024

akhilerm commented Aug 11, 2024 • edited Loading

cpuguy83 commented Aug 11, 2024

rata commented Aug 12, 2024

lifubang commented Aug 12, 2024 • edited Loading

thaJeztah commented Aug 12, 2024

rata commented Aug 12, 2024

lifubang commented Aug 13, 2024

lifubang commented Aug 13, 2024

rata commented Aug 13, 2024

rata commented Aug 13, 2024

kolyshkin commented Aug 14, 2024

kolyshkin commented Aug 14, 2024 • edited Loading

rata commented Aug 15, 2024

kolyshkin left a comment

Choose a reason for hiding this comment

rata commented Aug 15, 2024

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Aug 15, 2024

rata commented Aug 15, 2024 • edited Loading

kolyshkin commented Aug 15, 2024

thaJeztah commented Aug 15, 2024

rata commented Aug 15, 2024

thaJeztah left a comment

Choose a reason for hiding this comment

rata commented Aug 16, 2024

cpuguy83 commented Aug 23, 2024

rata commented Aug 8, 2024 •

edited

Loading

rata commented Aug 8, 2024 •

edited

Loading

akhilerm commented Aug 11, 2024 •

edited

Loading

lifubang commented Aug 12, 2024 •

edited

Loading

kolyshkin commented Aug 14, 2024 •

edited

Loading

rata commented Aug 15, 2024 •

edited

Loading