-
Notifications
You must be signed in to change notification settings - Fork 18
[WIP] LCORE-791: konflux hermetic build #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds hermetic dependency prefetching, optional source-image builds, and multi-arch targets to Tekton; introduces reproducible, hash-pinned Python dependency artifacts, Ruby/Bundler support in the Containerfile, UBI repo config, and a script to generate and wire hermetic requirements into pipelines. Changes
Sequence Diagram(s)mermaid Dev->>Git: push PR with prefetch-input & build-source-image Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rpms.in.yaml`:
- Around line 1-11: The packages list in rpms.in.yaml is missing rubygem-bundler
even though the Containerfile installs it and rpms.lock.yaml contains it; update
the packages array to include rubygem-bundler alongside rubygems (i.e., add the
symbol "rubygem-bundler" to the packages list) so the input manifest explicitly
declares the bundler RPM for hermetic builds and matches the lockfile and
Containerfile.
🧹 Nitpick comments (4)
pyproject.toml (1)
90-91: Align pip pin with the hermetic toolchain (or document the split)Dev deps pin pip==24.3.1 while the hermetic toolchain pins pip==25.3. Divergent resolver behavior can yield different lock outputs. Consider aligning these or adding a short note explaining the intentional split.
Containerfile (1)
13-13: Consider pinninguvto an exact version for hermetic builds.For a hermetic build aiming at reproducibility, using
uv>=0.7.20allows any newer version to be installed. Pin to an exact version (e.g.,uv==0.7.20) to ensure consistent builds.Suggested fix
-RUN pip3.12 install uv>=0.7.20 +RUN pip3.12 install uv==0.7.20requirements.overrides.txt (1)
1-11: Add trailing newline for POSIX compliance.The file is missing a trailing newline. Many tools expect text files to end with a newline character.
Suggested fix
faiss-cpu==1.12.0 +Makefile (1)
104-105: Add.PHONYdeclaration for consistency.Other targets in this Makefile use
.PHONYdeclarations. For consistency and to prevent issues if a file namedkonflux-requirementsis ever created, add the phony declaration.Suggested fix
+.PHONY: konflux-requirements konflux-requirements: ## generate hermetic requirements.*.txt file for konflux build ./scripts/konflux_requirements.sh
| packages: | ||
| [ | ||
| gcc, | ||
| cmake, | ||
| cargo, | ||
| rubygems, | ||
| python3.12, | ||
| python3.12-devel, | ||
| python3.12-pip, | ||
| git, | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the Containerfile and rpms.in.yaml files
fd -t f "Containerfile|Dockerfile" -x cat {}Repository: lightspeed-core/rag-content
Length of output: 4262
🏁 Script executed:
# Search for rubygem-bundler and bundler references
rg -i "rubygem-bundler|bundler" -B 2 -A 2Repository: lightspeed-core/rag-content
Length of output: 1442
🏁 Script executed:
# Check the rpms.in.yaml file to see its full content
cat rpms.in.yamlRepository: lightspeed-core/rag-content
Length of output: 269
Add rubygem-bundler to the packages list.
The Containerfile installs both rubygems and rubygem-bundler (line 9), and rubygem-bundler is already listed in rpms.lock.yaml for both architectures. For consistency and clarity in hermetic builds, rubygem-bundler should be explicitly included in the input packages.
🤖 Prompt for AI Agents
In `@rpms.in.yaml` around lines 1 - 11, The packages list in rpms.in.yaml is
missing rubygem-bundler even though the Containerfile installs it and
rpms.lock.yaml contains it; update the packages array to include rubygem-bundler
alongside rubygems (i.e., add the symbol "rubygem-bundler" to the packages list)
so the input manifest explicitly declares the bundler RPM for hermetic builds
and matches the lockfile and Containerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Containerfile`:
- Line 13: The RUN pip install line uses an unquoted package spec containing ">"
so the shell treats ">" as redirection; update the Dockerfile RUN command that
currently says RUN pip3.12 install uv>=0.7.20 to quote or escape the version
spec (e.g., use single or double quotes around uv>=0.7.20 or escape the >) so
the version constraint is preserved and no file is created by shell redirection.
In `@requirements-build.txt`:
- Around line 65-70: The requirements file contains conflicting pins for
hatchling (hatchling==1.26.3 and hatchling==1.28.0); resolve the duplicate
requirement by choosing a single hatchling version and updating the pins
accordingly (either bump dependencies that require 1.26.3 or downgrade those
requiring 1.28.0) so only one hatchling spec remains (remove or replace the
other occurrence of hatchling==1.26.3 or hatchling==1.28.0); ensure any direct
consumers (e.g., hatch-fancy-pypi-readme, openai, banks, bracex, cibuildwheel,
filelock, fsspec) are compatible with the chosen version before committing.
🧹 Nitpick comments (1)
Makefile (1)
104-105: Markkonflux-requirementsas phony to avoid file collisions.If a file named
konflux-requirementsappears, this target won’t run. Consider adding a.PHONYentry.♻️ Proposed change
+.PHONY: konflux-requirements konflux-requirements: ## generate hermetic requirements.*.txt file for konflux build ./scripts/konflux_requirements.sh
|
|
||
| # Install uv package manager | ||
| RUN pip3.12 install uv==0.7.20 | ||
| RUN pip3.12 install uv>=0.7.20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote the version spec to avoid shell redirection.
Line 13 will redirect output because > is parsed by the shell. This drops the version constraint and creates a stray file.
🐛 Proposed fix
-RUN pip3.12 install uv>=0.7.20
+RUN pip3.12 install "uv>=0.7.20"📝 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.
| RUN pip3.12 install uv>=0.7.20 | |
| RUN pip3.12 install "uv>=0.7.20" |
🤖 Prompt for AI Agents
In `@Containerfile` at line 13, The RUN pip install line uses an unquoted package
spec containing ">" so the shell treats ">" as redirection; update the
Dockerfile RUN command that currently says RUN pip3.12 install uv>=0.7.20 to
quote or escape the version spec (e.g., use single or double quotes around
uv>=0.7.20 or escape the >) so the version constraint is preserved and no file
is created by shell redirection.
| hatchling==1.26.3 | ||
| # via | ||
| # hatch-fancy-pypi-readme | ||
| # openai | ||
| hatchling==1.28.0 | ||
| # via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual content of requirements-build.txt around lines 65-70
sed -n '60,75p' requirements-build.txtRepository: lightspeed-core/rag-content
Length of output: 355
🏁 Script executed:
# Then run the duplicate detection script mentioned in the review
python - <<'PY'
import re
from collections import Counter
pkgs = []
with open("requirements-build.txt", "r", encoding="utf-8") as f:
for line in f:
line = line.strip()
if not line or line.startswith("#"):
continue
if " @ " in line:
name = line.split(" @ ", 1)[0]
else:
name = re.split(r"[<=>]", line, 1)[0]
pkgs.append(name.lower())
dups = [p for p, c in Counter(pkgs).items() if c > 1]
print("Duplicates found:", ", ".join(dups) if dups else "none")
print("\nFull package counts:")
for pkg, count in sorted(Counter(pkgs).items()):
if count > 1:
print(f" {pkg}: {count} occurrences")
PYRepository: lightspeed-core/rag-content
Length of output: 147
🏁 Script executed:
# Also check if there are any other versions of hatchling in the file
grep -n "hatchling" requirements-build.txtRepository: lightspeed-core/rag-content
Length of output: 204
Resolve duplicate hatchling pins to prevent pip failure.
hatchling is pinned to two different versions: 1.26.3 at line 65 (required by hatch-fancy-pypi-readme, openai) and 1.28.0 at line 69 (required by banks, bracex, cibuildwheel, filelock, fsspec). Pip will reject this with a "double requirement given" error. Consolidate to a single version by either upgrading/downgrading the conflicting dependencies or manually resolving the pins to a common version.
🤖 Prompt for AI Agents
In `@requirements-build.txt` around lines 65 - 70, The requirements file contains
conflicting pins for hatchling (hatchling==1.26.3 and hatchling==1.28.0);
resolve the duplicate requirement by choosing a single hatchling version and
updating the pins accordingly (either bump dependencies that require 1.26.3 or
downgrade those requiring 1.28.0) so only one hatchling spec remains (remove or
replace the other occurrence of hatchling==1.26.3 or hatchling==1.28.0); ensure
any direct consumers (e.g., hatch-fancy-pypi-readme, openai, banks, bracex,
cibuildwheel, filelock, fsspec) are compatible with the chosen version before
committing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/konflux_requirements.sh`:
- Around line 89-90: The sed substitution that pins maturin only matches
digits/dots and will break for suffixes like .post1 or rc; update the regex in
the sed command that touches BUILD_FILE (the line using sed and the token
"maturin==") to match the entire version token (e.g., use a non-whitespace token
matcher such as [^[:space:]]* or \S* instead of [0-9.]*) so the replacement
always replaces the full version token with maturin==1.10.2.
- Around line 41-86: The script runs `uv pip compile` on $WHEEL_FILE_PYPI even
when it contains only comments, causing failure; add a guard before the `uv pip
compile "$WHEEL_FILE_PYPI" ... > "$WHEEL_HASH_FILE_PYPI"` call that checks for
any non-comment/non-empty package lines (e.g. grep -v '^[[:space:]]*#'
"$WHEEL_FILE_PYPI" | grep -q '\S') and if none are found, skip the compile and
create/write an empty or placeholder $WHEEL_HASH_FILE_PYPI (or touch it) so
downstream steps don’t fail; update the block around the WHEEL_FILE_PYPI compile
invocation to use this conditional and keep the other compile commands
unchanged.
♻️ Duplicate comments (3)
rpms.in.yaml (1)
1-11: Addrubygem-bundlerto the packages list.Bundler is installed in the Containerfile; include the RPM here so the hermetic input manifest matches the build dependencies.
🐛 Proposed fix
rubygems, + rubygem-bundler, python3.12,Containerfile (1)
12-13: Quote the version spec to avoid shell redirection.Unquoted
>is parsed by the shell, so the constraint is dropped and a file is created.🐛 Proposed fix
-RUN pip3.12 install uv>=0.7.20 +RUN pip3.12 install "uv>=0.7.20"requirements-build.txt (1)
65-76: Resolve duplicatehatchlingpins to avoid pip conflicts.Two
hatchlingversions are pinned; pip will reject this. Consolidate to a single compatible version and adjust dependents as needed.🐛 Example consolidation (remove the older pin)
-hatchling==1.26.3 - # via - # hatch-fancy-pypi-readme - # openai
🧹 Nitpick comments (2)
Makefile (1)
104-105: Declarekonflux-requirementsas phony.Avoids conflicts if a file named
konflux-requirementsappears and keeps parity with other utility targets.♻️ Proposed change
+.PHONY: konflux-requirements konflux-requirements: ## generate hermetic requirements.*.txt file for konflux build ./scripts/konflux_requirements.sh.tekton/rag-tool-push.yaml (1)
27-59: Align prefetch binary architectures with build platforms.
build-platformsis x86_64 only, but prefetchbinary.archincludes aarch64. If multi-arch isn’t intended here, this fetches extra artifacts; if it is, consider adding aarch64 tobuild-platforms.♻️ One possible alignment (x86_64-only)
- "arch": "x86_64,aarch64", + "arch": "x86_64",
| echo "# Packages from pypi.org to be fetched as wheels" > "$WHEEL_FILE_PYPI" | ||
| echo "# This file was autogenerated by konflux_requirements.sh" >> "$WHEEL_FILE_PYPI" | ||
|
|
||
| current_package="" | ||
|
|
||
| while IFS= read -r line || [[ -n "$line" ]]; do | ||
| # Check if this is a package line (starts with a letter/digit, not whitespace or #) | ||
| if [[ "$line" =~ ^[a-zA-Z0-9] ]]; then | ||
| current_package="$line" | ||
| # Check if this is a "# from" annotation line | ||
| elif [[ "$line" =~ ^[[:space:]]*#[[:space:]]*from[[:space:]]+(.*) ]]; then | ||
| index_url="${BASH_REMATCH[1]}" | ||
|
|
||
| if [[ -n "$current_package" ]]; then | ||
| if [[ "$index_url" == "https://pypi.org/simple/" ]]; then | ||
| # Extract package name (without version) for comparison | ||
| package_name=$(echo "$current_package" | sed 's/[=<>!].*//') | ||
| if echo ",${PYPI_WHEELS}," | grep -qF ",${package_name},"; then | ||
| echo "$current_package" >> "$WHEEL_FILE_PYPI" | ||
| else | ||
| echo "$current_package" >> "$SOURCE_FILE" | ||
| fi | ||
| elif [[ "$index_url" == "https://console.redhat.com/api/pypi/public-rhai/rhoai/3.2/cpu-ubi9/simple/" ]]; then | ||
| echo "$current_package" >> "$WHEEL_FILE" | ||
| fi | ||
| current_package="" | ||
| fi | ||
| fi | ||
| done < "$RAW_REQ_FILE" | ||
|
|
||
| # replace the list of binary packages in konflux pipeline configuration | ||
| # only the package names, not the versions, delimited by commas | ||
| wheel_packages=$(grep -v "^[#-]" "$WHEEL_FILE" | sed 's/==.*//' | tr '\n' ',' | sed 's/,$//') | ||
| # append extra wheels to the list | ||
| wheel_packages="$wheel_packages,$EXTRA_WHEELS,$PYPI_WHEELS" | ||
| sed -i 's/"packages": "[^"]*"/"packages": "'"$wheel_packages"'"/' .tekton/rag-tool-pull-request.yaml | ||
| sed -i 's/"packages": "[^"]*"/"packages": "'"$wheel_packages"'"/' .tekton/rag-tool-push.yaml | ||
|
|
||
| echo "Packages from pypi.org written to: $SOURCE_FILE ($(wc -l < "$SOURCE_FILE") packages)" | ||
| echo "Packages from console.redhat.com written to: $WHEEL_FILE ($(wc -l < "$WHEEL_FILE") packages)" | ||
|
|
||
|
|
||
| # Use stdout redirection instead of -o flag to work around uv bug where -o reuses stale hashes from existing output file | ||
| uv pip compile "$WHEEL_FILE" --refresh --generate-hashes --index-url https://console.redhat.com/api/pypi/public-rhai/rhoai/3.2/cpu-ubi9/simple/ --python-version 3.12 --emit-index-url --no-deps --no-annotate --universal > "$WHEEL_HASH_FILE" | ||
| uv pip compile "$WHEEL_FILE_PYPI" --refresh --generate-hashes --python-version 3.12 --emit-index-url --no-deps --no-annotate > "$WHEEL_HASH_FILE_PYPI" | ||
| uv pip compile "$SOURCE_FILE" --refresh --generate-hashes --python-version 3.12 --emit-index-url --no-deps --no-annotate > "$SOURCE_HASH_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against empty PyPI wheel lists before compiling hashes.
If none of the PYPI_WHEELS entries are present in the compiled requirements, $WHEEL_FILE_PYPI will contain only comments. uv pip compile typically exits with “No requirements specified,” which breaks the script under set -e even though an empty wheel list is valid. Consider skipping compilation (or writing an empty hash file) when no package lines exist.
🔧 Proposed fix
-uv pip compile "$WHEEL_FILE_PYPI" --refresh --generate-hashes --python-version 3.12 --emit-index-url --no-deps --no-annotate > "$WHEEL_HASH_FILE_PYPI"
+if grep -qEv '^[[:space:]]*($|#)' "$WHEEL_FILE_PYPI"; then
+ uv pip compile "$WHEEL_FILE_PYPI" --refresh --generate-hashes --python-version 3.12 --emit-index-url --no-deps --no-annotate > "$WHEEL_HASH_FILE_PYPI"
+else
+ echo "# No PyPI wheel requirements" > "$WHEEL_HASH_FILE_PYPI"
+fi🤖 Prompt for AI Agents
In `@scripts/konflux_requirements.sh` around lines 41 - 86, The script runs `uv
pip compile` on $WHEEL_FILE_PYPI even when it contains only comments, causing
failure; add a guard before the `uv pip compile "$WHEEL_FILE_PYPI" ... >
"$WHEEL_HASH_FILE_PYPI"` call that checks for any non-comment/non-empty package
lines (e.g. grep -v '^[[:space:]]*#' "$WHEEL_FILE_PYPI" | grep -q '\S') and if
none are found, skip the compile and create/write an empty or placeholder
$WHEEL_HASH_FILE_PYPI (or touch it) so downstream steps don’t fail; update the
block around the WHEEL_FILE_PYPI compile invocation to use this conditional and
keep the other compile commands unchanged.
| # pin maturin to the version available in the Red Hat registry | ||
| sed -i 's/maturin==[0-9.]*/maturin==1.10.2/' "$BUILD_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the maturin pin resilient to non-numeric version suffixes.
The current regex only matches digits and dots; if the resolved version includes a suffix (e.g., .post1, rc), the substitution can produce an invalid version string. Match the full token instead.
🔧 Proposed fix
-sed -i 's/maturin==[0-9.]*/maturin==1.10.2/' "$BUILD_FILE"
+sed -i 's/maturin==[^[:space:]]*/maturin==1.10.2/' "$BUILD_FILE"📝 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.
| # pin maturin to the version available in the Red Hat registry | |
| sed -i 's/maturin==[0-9.]*/maturin==1.10.2/' "$BUILD_FILE" | |
| sed -i 's/maturin==[^[:space:]]*/maturin==1.10.2/' "$BUILD_FILE" |
🤖 Prompt for AI Agents
In `@scripts/konflux_requirements.sh` around lines 89 - 90, The sed substitution
that pins maturin only matches digits/dots and will break for suffixes like
.post1 or rc; update the regex in the sed command that touches BUILD_FILE (the
line using sed and the token "maturin==") to match the entire version token
(e.g., use a non-whitespace token matcher such as [^[:space:]]* or \S* instead
of [0-9.]*) so the replacement always replaces the full version token with
maturin==1.10.2.
Description
Enable hermetic build.
Tools to regenerate requirement files.
Ruby gems are installed using bundler, for Konflux only support bundler to install Ruby gems in hermetic builds.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.