Makefile: Improve containerized target #4928
Conversation
WalkthroughReplaced a multi-step containerized cross-build (build image, run container, copy artifacts, cleanup) with a single container run that mounts the repo and invokes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as Make (containerized)
participant CR as Container Runtime
participant DF as images/openshift-ci/Dockerfile
Note over Make: Old flow (removed)
Dev->>Make: make containerized
Make->>CR: build crc-build image
Make->>CR: run container (crc-cross) to make cross
Make->>CR: copy artifacts out
Make->>CR: remove container and image
par New flow (current)
Dev->>Make: make containerized
Make->>DF: read FROM -> image
Make->>CR: run --rm -v ${PWD}:/data${SELINUX_VOLUME_LABEL} image /bin/bash -c "cd /data && make cross"
CR-->>Make: cross build completes (artifacts in mounted dir)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
118-118: Optional: pull policy and caches for speed/repro.
- Consider adding
--pull=always(or--pull=newer) to match CI freshness.- Persist Go caches to speed subsequent runs.
- ${CONTAINER_RUNTIME} run --rm \ + ${CONTAINER_RUNTIME} run --rm --pull=always \ -v "$(CURDIR):/data${SELINUX_VOLUME_LABEL}" \ + -v "$(CURDIR)/.gocache:/gocache" -e GOMODCACHE=/gocache/mod -e GOCACHE=/gocache/build \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
| cross: $(BUILD_DIR)/macos-arm64/crc $(BUILD_DIR)/macos-amd64/crc $(BUILD_DIR)/linux-amd64/crc $(BUILD_DIR)/linux-arm64/crc $(BUILD_DIR)/windows-amd64/crc.exe | ||
|
|
||
| .PHONY: containerized ## Cross compile from container | ||
| containerized: image := $(shell grep -m 1 '^FROM' images/openshift-ci/Dockerfile | awk '{print $$2}') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make image derivation robust and overridable.
Current grep/awk breaks with FROM flags (e.g., --platform) or stage aliases (AS builder), and it can’t be overridden cleanly. Prefer a target-specific ?= (respects command-line overrides) and skip flags/alias.
-containerized: image := $(shell grep -m 1 '^FROM' images/openshift-ci/Dockerfile | awk '{print $$2}')
+containerized: image ?= $(shell awk 'BEGIN{IGNORECASE=1} $$1=="FROM"{for(i=2;i<=NF;i++){if($$i ~ /^--/) continue; print $$i; break}}' images/openshift-ci/Dockerfile | sed -E 's/[[:space:]]+[Aa][Ss][[:space:]].*$$//' | head -n1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| containerized: image := $(shell grep -m 1 '^FROM' images/openshift-ci/Dockerfile | awk '{print $$2}') | |
| containerized: image ?= $(shell awk 'BEGIN{IGNORECASE=1} $$1=="FROM"{for(i=2;i<=NF;i++){if($$i ~ /^--/) continue; print $$i; break}}' images/openshift-ci/Dockerfile | sed -E 's/[[:space:]]+[Aa][Ss][[:space:]].*$$//' | head -n1) |
🤖 Prompt for AI Agents
Makefile around line 116: the current image assignment should be made
target-overridable using a target-specific '?=' and the parsing must ignore FROM
flags (tokens starting with '--') and stage aliases like 'AS'. Replace the
single grep/awk one-liner with a '?=' assignment that extracts the first
non-flag, non-"AS" token from the Dockerfile FROM line (e.g., scan fields after
FROM and pick the first field that does not start with '--' and is not 'AS'), so
command-line overrides work and FROM lines containing flags or "AS <stage>" are
handled correctly.
Makefile
Outdated
| ${CONTAINER_RUNTIME} cp crc-cross:/opt/app-root/src/out ./ | ||
| ${CONTAINER_RUNTIME} rm crc-cross | ||
| ${CONTAINER_RUNTIME} rmi crc-build | ||
| ${CONTAINER_RUNTIME} run --rm -v ${PWD}:/data:Z ${image} /bin/bash -c "cd /data && make cross" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix SELinux label handling on macOS, preserve file ownership, and use $(MAKE).
- Use existing SELINUX_VOLUME_LABEL to avoid “:Z” on darwin.
- Map UID:GID so artifacts aren’t root-owned.
- Quote the path and prefer $(CURDIR).
- Use $(MAKE) (propagates flags/vars) and -w instead of cd.
- Fail fast if image couldn’t be derived.
- ${CONTAINER_RUNTIME} run --rm -v ${PWD}:/data:Z ${image} /bin/bash -c "cd /data && make cross"
+ @if [ -z "${image}" ]; then echo "containerized: failed to derive base image from images/openshift-ci/Dockerfile (override with 'make containerized image=<ref>')" >&2; exit 1; fi
+ ${CONTAINER_RUNTIME} run --rm \
+ -v "$(CURDIR):/data${SELINUX_VOLUME_LABEL}" \
+ -u "$$(id -u):$$(id -g)" \
+ -w /data \
+ ${image} /bin/bash -lc '$(MAKE) cross'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ${CONTAINER_RUNTIME} run --rm -v ${PWD}:/data:Z ${image} /bin/bash -c "cd /data && make cross" | |
| @if [ -z "${image}" ]; then echo "containerized: failed to derive base image from images/openshift-ci/Dockerfile (override with 'make containerized image=<ref>')" >&2; exit 1; fi | |
| ${CONTAINER_RUNTIME} run --rm \ | |
| -v "$(CURDIR):/data${SELINUX_VOLUME_LABEL}" \ | |
| -u "$$(id -u):$$(id -g)" \ | |
| -w /data \ | |
| ${image} /bin/bash -lc '$(MAKE) cross' |
🤖 Prompt for AI Agents
In Makefile around line 118, the docker run invocation needs improvements:
replace the hardcoded :Z with the existing SELINUX_VOLUME_LABEL variable to
avoid using “:Z” on darwin, mount the workspace with explicit UID:GID mapping
(e.g., $(shell id -u):$(shell id -g)) so artifacts aren’t root-owned, quote the
path and use $(CURDIR) instead of ${PWD}, call $(MAKE) with -w to propagate
flags/vars instead of cd /data && make cross, and add a guard to fail fast if
the image variable is empty or could not be derived. Ensure these changes
preserve SELinux handling, ownership, quoting, and proper make invocation.
Makefile
Outdated
| ${CONTAINER_RUNTIME} cp crc-cross:/opt/app-root/src/out ./ | ||
| ${CONTAINER_RUNTIME} rm crc-cross | ||
| ${CONTAINER_RUNTIME} rmi crc-build | ||
| ${CONTAINER_RUNTIME} run --rm -v ${PWD}:/data:Z ${image} /bin/bash -c "cd /data && make cross" |
There was a problem hiding this comment.
images/build/* should be removed then?
There was a problem hiding this comment.
@cfergeau I think that image is still used by QE, @lilyLuLiu or @albfan can confirm that.
As of now for this target we are building image first and then copy the binaries out from container to host. This PR uses same base image which openshift-ci using and mount the current working directory and execute cross builds so that building image is not required and also binary directly available to host.
praveenkumar
force-pushed
the
fix_container_run
branch
from
f66d1d1 to
8270ee9
Compare
|
@praveenkumar: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
As of now for this target we are building image first and then copy the binaries out from container to host. This PR uses same base image which openshift-ci using and mount the current working directory and execute cross builds so that building image is not required and also binary directly available to host.
Summary by CodeRabbit
No user-facing changes; application behavior remains the same.