Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

rata
Copy link
Member

@rata rata commented Aug 15, 2024

We recently switched VERSION to be read from env vars (#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 #4370, we discussed to consider doing the same for
these variables too
(#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

@rata rata requested review from thaJeztah and kolyshkin August 15, 2024 14:38
@rata rata force-pushed the makefile-no-envs branch from f3cc0a4 to 99b3881 Compare August 15, 2024 14:39
@rata rata changed the title Makefile: Don't read COMMIT, BUILDTAG, EXTRA_BUILDTAGS from env vars Makefile: Don't read COMMIT, BUILDTAGS, EXTRA_BUILDTAGS from env vars Aug 15, 2024
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

🙈 needs a rebase

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 rata force-pushed the makefile-no-envs branch from 99b3881 to 767bc00 Compare August 24, 2024 12:50
@rata
Copy link
Member Author

rata commented Aug 24, 2024

Rebased now :)

@AkihiroSuda AkihiroSuda merged commit 346b818 into opencontainers:main Aug 24, 2024
42 checks passed
@lifubang lifubang mentioned this pull request Aug 31, 2024
@cpuguy83
Copy link
Contributor

Wait... this now breaks builds again.

@cpuguy83
Copy link
Contributor

There is no way to set a COMMIT except by carrying a patch to overwrite this.
It is removing something that has been in the Makefile for years.

Keep in mind that most of the time packagers do not have a .git to read from.

@tianon
Copy link
Member

tianon commented Dec 31, 2024

Doesn't make COMMIT=foo still work with this change?

@cpuguy83
Copy link
Contributor

Oh yes, you are right. I always forget that make args can set those vars even without ?=. So I guess no patch is needed.

@akhilerm
Copy link
Contributor

akhilerm commented Jan 1, 2025

Oh yes, you are right. I always forget that make args can set those vars even without ?=. So I guess no patch is needed.

No patch is needed. I am building internally using

make EXTRA_VERSION=${version_meta} EXTRA_BUILDTAGS=${extra_build_tags} COMMIT=${runc_commit}

@thaJeztah
Copy link
Member

Yeah, it's easy to confuse order;

This takes SOME_VAR from environment variables (which may, or may not work, depending on whether make is running in POSIX mode);

SOME_VAR=foo make some-target

This sets the make var;

make SOME_VAR=foo some-target

I got bit by that because macOS and Linux make differ, so now always default to the latter form 😅

@rata rata deleted the makefile-no-envs branch January 21, 2025 15:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants