Makefile: Handle unexpected empty var. values #14435

cevich · 2022-05-31T15:56:31Z

Substitution values built from $(shell ...) output can easily be empty
due to the shell's default pipefail behavior. This can also hide
non-zero exit codes, similarly resulting in empty values being set.
While not a perfect fix, the situation is improved by using the
err_if_empty function in all cases where empty values would be
unexpected. Also remove the definitions for GIT_BRANCH and
GIT_BRANCH_CLEAN which don't seem to actually be used anywhere
(including in code).

Does this PR introduce a user-facing change?

None

Makefile

contrib/cirrus/runner.sh

Fixes containers#14021 Substitution values built from `$(shell ...)` output can easily be empty due to the shell's default `pipefail` behavior. This can also hide non-zero exit codes, similarly resulting in empty values being set. While not a perfect fix, the situation is improved by using the `err_if_empty` function in all cases where empty values would be unexpected. Remove the definitions for `GIT_BRANCH` and `GIT_BRANCH_CLEAN` which don't seem to actually be used anywhere (including in code). Add a simple release-test to verify `podman info` outputs a non-empty value for "GitCommit". Signed-off-by: Chris Evich <[email protected]>

cevich · 2022-06-02T15:30:12Z

Final review-note: I'm a little nervous about build-targets used outside of CI. I did not check/test any of them 😨

edsantiago

This whole thing makes me super uncomfortable. I don't understand its ramifications. I don't understand why only some vars get the err_if_empty treatment.

I did run make with my .git subdirectory renamed, and confirmed that it still builds; but I don't know what else to test. I'd like to see the scope of this PR reduced to a reasonably small bare minimum, then give it a month or two (including releases and bodhi builds) before getting aggressive with TMPDIR and GOOS and _HLP_whatever. But that's just a preference, and I won't block because of it.

edsantiago · 2022-06-02T16:40:37Z

Makefile

 DATE_FMT = %s
 ifdef SOURCE_DATE_EPOCH
-	BUILD_INFO ?= $(shell date -u -d "@$(SOURCE_DATE_EPOCH)" "+$(DATE_FMT)" 2>/dev/null || date -u -r "$(SOURCE_DATE_EPOCH)" "+$(DATE_FMT)" 2>/dev/null || date -u "+$(DATE_FMT)")
+	BUILD_INFO ?= $(shell date -u -d "@$(call err_if_empty,SOURCE_DATE_EPOCH)" "+$(DATE_FMT)" 2>/dev/null || date -u -r "$(SOURCE_DATE_EPOCH)" "+$(DATE_FMT)" 2>/dev/null || date -u "+$(DATE_FMT)")


Given the ifdef just above, isn't this change unnecessary (and hence misleading)?

edsantiago · 2022-06-02T16:42:15Z

Makefile

+	$(if $(GIT_COMMIT),-X $(LIBPOD)/define.gitCommit=$(GIT_COMMIT),) \
+	$(if $(BUILD_INFO),-X $(LIBPOD)/define.buildInfo=$(BUILD_INFO),) \


Empty define.X= seems harmless, and has been working for many years, and is actually pretty clean and easy to read... so I'd be inclined to just revert this change, leaving it as it was

Okay, thanks. I was unsure of it as well.

edsantiago · 2022-06-02T16:57:22Z

Makefile

+	$(MAKE) podman-v$(call err_if_empty,RELEASE_NUMBER).msi
+podman-v%.msi: test/version/version podman-remote-windows podman-remote-windows-docs podman-winpath win-sshproxy


I have no way to review Windows code. @n1hility are you our Windows expert? Could you bless this?

Sure looks fine, will test

everything passes

FWIW: We do actually publish the latest test-builds of this MSI (among other binaries). It's available from https://api.cirrus-ci.com/v1/artifact/github/containers/podman/artifacts/binary/podman-<version>.msi Currently <version> is v4.2.0-dev. Though the build is out of date due to a problem I'm working on fixing elsewhere.

ah thanks! good to know

cevich · 2022-06-02T18:28:19Z

I don't understand why only some vars get the err_if_empty treatment.

Oh, that's simple to answer, it relates to #14021. We only need to check cases where a shell call may have a component exit non-zero and return an (otherwise) unexpected empty value. For example if when pipes are used (since pipefail is not set). Though to keep things simple, I tried to stick the check on anything based on a shell call.

I'd like to see the scope of this PR reduced to a reasonably small bare minimum

I'm not sure how to do both that, and fix #14021 😞

edsantiago · 2022-06-02T18:37:23Z

Ok then. Fingers crossed, then.

/lgtm

rhatdan · 2022-06-02T18:38:16Z

/approve

openshift-ci · 2022-06-02T18:38:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, n1hility, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cevich · 2022-06-03T14:09:29Z

Ok then. Fingers crossed, then.

Aye 😞 sometimes that's all there is to do. But a fix could be one revert away 😁 Thanks for reviewing, I do appreciate it.

cevich requested review from edsantiago and mheon May 31, 2022 15:56

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels May 31, 2022

Luap99 reviewed May 31, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

cevich force-pushed the makefile_empty branch 2 times, most recently from 4f8b00b to 593df42 Compare June 1, 2022 20:16

edsantiago reviewed Jun 1, 2022

View reviewed changes

contrib/cirrus/runner.sh Outdated Show resolved Hide resolved

cevich force-pushed the makefile_empty branch from 593df42 to 3fa09e9 Compare June 1, 2022 21:42

cevich marked this pull request as ready for review June 2, 2022 15:29

cevich changed the title ~~[WIP] Makefile: Handle unexpected empty var. values~~ Makefile: Handle unexpected empty var. values Jun 2, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2022

edsantiago reviewed Jun 2, 2022

View reviewed changes

n1hility approved these changes Jun 2, 2022

View reviewed changes

openshift-ci bot assigned edsantiago Jun 2, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2022

openshift-merge-robot merged commit 2777ecd into containers:main Jun 2, 2022

This was referenced Jun 2, 2022

Timeout in podman-save multi-image #14468

Closed

CI flake: secret file is not leaked into image #13417

Closed

cevich deleted the makefile_empty branch April 18, 2023 14:45

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 31, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile: Handle unexpected empty var. values #14435

Makefile: Handle unexpected empty var. values #14435

cevich commented May 31, 2022

cevich commented Jun 2, 2022

edsantiago left a comment

edsantiago Jun 2, 2022

edsantiago Jun 2, 2022

cevich Jun 2, 2022

edsantiago Jun 2, 2022

n1hility Jun 2, 2022

n1hility Jun 2, 2022

cevich Jun 2, 2022

n1hility Jun 2, 2022

cevich commented Jun 2, 2022

edsantiago commented Jun 2, 2022

rhatdan commented Jun 2, 2022

openshift-ci bot commented Jun 2, 2022

cevich commented Jun 3, 2022

		$(if $(GIT_COMMIT),-X $(LIBPOD)/define.gitCommit=$(GIT_COMMIT),) \
		$(if $(BUILD_INFO),-X $(LIBPOD)/define.buildInfo=$(BUILD_INFO),) \

		$(MAKE) podman-v$(call err_if_empty,RELEASE_NUMBER).msi
		podman-v%.msi: test/version/version podman-remote-windows podman-remote-windows-docs podman-winpath win-sshproxy

Makefile: Handle unexpected empty var. values #14435

Makefile: Handle unexpected empty var. values #14435

Conversation

cevich commented May 31, 2022

Does this PR introduce a user-facing change?

cevich commented Jun 2, 2022

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cevich commented Jun 2, 2022

edsantiago commented Jun 2, 2022

rhatdan commented Jun 2, 2022

openshift-ci bot commented Jun 2, 2022

cevich commented Jun 3, 2022