RHAIENG-2846 Prefetch: COMPONENT_DIR from Makefile, local build default, Tekton discovery#3043
Conversation
…lt, Tekton discovery - GHA: Derive COMPONENT_DIR from Makefile; fail clearly if missing. - Makefile: Always pass LOCAL_BUILD=true for hermetic targets; require cachi2/output when prefetch-input exists; fix empty-variable warning. - prefetch-all.sh: Add find_tekton_yaml (ODH/RHDS by dockerfile param); remove --tekton-file and package-lock fallback; allow running without variant dir. - download-npm.sh: Exit 0 when Tekton file has no npm entries. - Regenerate artifacts.lock.yaml and pylock.cpu.toml; update README.
github-actions
bot
added
the
review-requested
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR refactors component directory derivation and prefetch logic to use Makefile-based extraction instead of simple path conversion. It introduces variant-aware npm discovery with a new helper function, updates lock file headers and artifact checksums, and revises the prefetch workflow documentation to reflect these changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 90-96: The $(error ...) guard (triggered via the conditional that
checks $(wildcard $(BUILD_DIR)prefetch-input) and cachi2/output) is being
evaluated at parse/expansion time (even during make -n) and must be deferred;
replace the top-level $(if ... $(error ...)) with a runtime check inside the
recipe (or skip it when MAKEFLAGS includes -n). Concretely, remove the $(error
...) usage from the global expansion and instead perform the prefetch guard in
shell code within the relevant target's recipe (use the same tests on
$(BUILD_DIR)prefetch-input and cachi2/output), or wrap the existing check with a
guard that detects dry-run via $(filter -n,$(MAKEFLAGS))/$(MAKEFLAGS) so the
error is suppressed during make -n; keep references to LOCAL_BUILD_ARG,
CACHI2_VOLUME and the same messages but ensure the check runs at execution time
not parse time.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.github/workflows/build-notebooks-TEMPLATE.yamlMakefilecodeserver/ubi9-python-3.12/prefetch-input/odh/artifacts.lock.yamlcodeserver/ubi9-python-3.12/uv.lock.d/pylock.cpu.tomlscripts/lockfile-generators/README.mdscripts/lockfile-generators/download-npm.shscripts/lockfile-generators/prefetch-all.sh
| # Make is only used for local and GHA builds (Konflux does not run make). | ||
| # Default to local build for hermetic-capable targets: always set LOCAL_BUILD=true | ||
| # when prefetch-input/ exists; mount cachi2/output only when it exists (after prefetch). | ||
| $(eval LOCAL_BUILD_ARG := $(if $(wildcard $(BUILD_DIR)prefetch-input),--build-arg LOCAL_BUILD=true,)) | ||
| $(eval CACHI2_VOLUME := $(if $(and $(wildcard cachi2/output),$(wildcard $(BUILD_DIR)prefetch-input)),--volume $(ROOT_DIR)cachi2/output:/cachi2/output:Z,)) | ||
| # Fail fast with a clear message if team runs make without prefetching first. | ||
| $(if $(wildcard $(BUILD_DIR)prefetch-input),$(if $(wildcard cachi2/output),,$(error Prefetch required for hermetic build. Run: scripts/lockfile-generators/prefetch-all.sh --component-dir $(patsubst %/,%,$(BUILD_DIR)) — see scripts/lockfile-generators/README.md)),) |
There was a problem hiding this comment.
$(error) triggers during make -n dry-run, breaking COMPONENT_DIR derivation.
The pipeline failures show this error occurring at line 96. The issue is that the GHA workflow (line 285) runs make -n "${{ inputs.target }}" to derive COMPONENT_DIR before running prefetch-all.sh. However, the $(error ...) on line 96 is evaluated during Makefile parsing/expansion—even during a dry-run—causing the workflow to fail before it can determine whether to run prefetch.
The $(if ...) with $(error ...) is evaluated when the recipe is expanded, which happens during make -n to show what commands would run.
Consider either:
- Move the guard to shell code within the recipe so it only runs during actual execution, not dry-run
- Skip the guard during dry-run by checking a variable or using a different pattern
🐛 Proposed fix: Move guard to shell code in the recipe
# Fail fast with a clear message if team runs make without prefetching first.
- $(if $(wildcard $(BUILD_DIR)prefetch-input),$(if $(wildcard cachi2/output),,$(error Prefetch required for hermetic build. Run: scripts/lockfile-generators/prefetch-all.sh --component-dir $(patsubst %/,%,$(BUILD_DIR)) — see scripts/lockfile-generators/README.md)),)
+ $(eval PREFETCH_GUARD := $(if $(wildcard $(BUILD_DIR)prefetch-input),$(if $(wildcard cachi2/output),,FAIL),))
$(info # Building $(IMAGE_NAME) using $(DOCKERFILE_NAME) with $(CONF_FILE) and $(BUILD_ARGS)...)
+ $(if $(PREFETCH_GUARD),`@echo` "Error: Prefetch required for hermetic build. Run: scripts/lockfile-generators/prefetch-all.sh --component-dir $(patsubst %/,%,$(BUILD_DIR)) — see scripts/lockfile-generators/README.md" >&2 && exit 1)
$(ROOT_DIR)/scripts/sandbox.py --dockerfile '$(2)' --platform '$(BUILD_ARCH)' -- \Alternatively, use a shell check inside the recipe:
- $(if $(wildcard $(BUILD_DIR)prefetch-input),$(if $(wildcard cachi2/output),,$(error Prefetch required for hermetic build. Run: scripts/lockfile-generators/prefetch-all.sh --component-dir $(patsubst %/,%,$(BUILD_DIR)) — see scripts/lockfile-generators/README.md)),)
$(info # Building $(IMAGE_NAME) using $(DOCKERFILE_NAME) with $(CONF_FILE) and $(BUILD_ARGS)...)
$(ROOT_DIR)/scripts/sandbox.py --dockerfile '$(2)' --platform '$(BUILD_ARCH)' -- \
- $(CONTAINER_ENGINE) build $(CONTAINER_BUILD_CACHE_ARGS) $(LOCAL_BUILD_ARG) $(CACHI2_VOLUME) --platform=$(BUILD_ARCH) --label release=$(RELEASE) --tag $(IMAGE_NAME) --file '$(2)' $(BUILD_ARGS) {}\;
+ sh -c 'if [ -d "$(BUILD_DIR)prefetch-input" ] && [ ! -d "cachi2/output" ]; then echo "Error: Prefetch required for hermetic build. Run: scripts/lockfile-generators/prefetch-all.sh --component-dir $(patsubst %/,%,$(BUILD_DIR)) — see scripts/lockfile-generators/README.md" >&2; exit 1; fi' && \
+ $(CONTAINER_ENGINE) build $(CONTAINER_BUILD_CACHE_ARGS) $(LOCAL_BUILD_ARG) $(CACHI2_VOLUME) --platform=$(BUILD_ARCH) --label release=$(RELEASE) --tag $(IMAGE_NAME) --file '$(2)' $(BUILD_ARGS) {}\;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 90 - 96, The $(error ...) guard (triggered via the
conditional that checks $(wildcard $(BUILD_DIR)prefetch-input) and
cachi2/output) is being evaluated at parse/expansion time (even during make -n)
and must be deferred; replace the top-level $(if ... $(error ...)) with a
runtime check inside the recipe (or skip it when MAKEFLAGS includes -n).
Concretely, remove the $(error ...) usage from the global expansion and instead
perform the prefetch guard in shell code within the relevant target's recipe
(use the same tests on $(BUILD_DIR)prefetch-input and cachi2/output), or wrap
the existing check with a guard that detects dry-run via $(filter
-n,$(MAKEFLAGS))/$(MAKEFLAGS) so the error is suppressed during make -n; keep
references to LOCAL_BUILD_ARG, CACHI2_VOLUME and the same messages but ensure
the check runs at execution time not parse time.
|
@ysok: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
1 participant
RHAIENG-2846 Post-hermetic cleanup
Summary by CodeRabbit