fix: pip version constraint escaping in container build#41
Conversation
derekhiggins
requested review from
Elbehery,
VaishnaviHire,
cdoern,
leseb,
nathan-weinberg,
rhdedgar,
rhuss and
skamenan7
as code owners
WalkthroughWraps pip package specifiers containing comparison operators in single quotes when constructing install commands and updates several package specifiers in the Dockerfile to use quoted forms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Build as distribution/build.py
participant Dep as dependency generator
participant Shell as Shell
participant Pip as pip
Dev->>Build: run build script
Build->>Dep: fetch dependency tokens
Dep-->>Build: return tokens
Note over Build: dedupe & sort tokens
loop each token
alt token contains ">" or "<"
Build->>Build: wrap token in single quotes
else
Build->>Build: keep token as-is
end
end
Build->>Shell: emit pip install command (tokens quoted as needed)
Shell->>Pip: invoke pip install ...
Pip-->>Shell: install result
Shell-->>Build: exit status
Build-->>Dev: report outcome
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (1)
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: 0
🧹 Nitpick comments (1)
distribution/build.py (1)
83-85: Good fix for shell redirection; harden quoting + satisfy ruffQuoting specifiers prevents Bash from creating stray files. Use shlex.quote for correctness (handles embedded quotes) and wrap the comprehension to keep ruff-format happy.
Apply this diff within these lines:
- # Add quotes to packages with > or < to prevent bash redirection - packages = [f"'{package}'" if (">" in package or "<" in package) else package for package in packages] + # Add quotes to packages with > or < to prevent shell redirection + packages = [ + shlex.quote(p) if any(op in p for op in ("<", ">")) else p + for p in packages + ]Add the missing import near the other imports:
import shlex
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
distribution/Containerfile(3 hunks)distribution/build.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#33
File: distribution/Containerfile:17-21
Timestamp: 2025-09-15T14:25:54.837Z
Learning: In the opendatahub-io/llama-stack-distribution repository, the distribution/Containerfile is auto-generated by distribution/build.py based on configuration in build.yaml. When providers are added to build.yaml, the build script automatically regenerates the Containerfile with the required dependencies. Changes to the Containerfile should not be flagged as manual edits if they correspond to legitimate changes in the build configuration.
📚 Learning: 2025-09-15T14:25:54.837Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#33
File: distribution/Containerfile:17-21
Timestamp: 2025-09-15T14:25:54.837Z
Learning: In the opendatahub-io/llama-stack-distribution repository, the distribution/Containerfile is auto-generated by distribution/build.py based on configuration in build.yaml. When providers are added to build.yaml, the build script automatically regenerates the Containerfile with the required dependencies. Changes to the Containerfile should not be flagged as manual edits if they correspond to legitimate changes in the build configuration.
Applied to files:
distribution/Containerfile
🪛 GitHub Actions: Pre-commit
distribution/build.py
[error] 81-92: ruff-format formatting failed. 1 file reformatted (distribution/build.py); 1 file left unchanged.
⏰ 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). (1)
- GitHub Check: build-test-push (linux/amd64)
🔇 Additional comments (3)
distribution/Containerfile (1)
14-14: Quoting version constraints is correct and aligns with generator.The single quotes around packages containing >= prevent redirection and match the build.py behavior. Looks good.
If this file is regenerated as part of CI, ensure the committed output matches the generator by re-running the build script before merging.
Also applies to: 20-20, 29-29, 45-45
distribution/build.py (2)
83-85: Re-run pre-commit/format and commit the formatted filepre-commit was not available in the verification environment (pre-commit: command not found) and the attempted commit failed due to missing git identity; ruff reformatted distribution/build.py (1 file reformatted). Run locally and commit the changes:
- pre-commit run -a
- or: ruff format
- git add distribution/build.py
- git config user.email "you@example.com" && git config user.name "Your Name" (if not configured)
- git commit -m "Format distribution/build.py with ruff-format"
73-101: Don't sort option/value tokens — only sort package specifiersSorting flags and their values together can pair an option (e.g., --index-url) with the wrong URL; keep option/value pairs in original order and only sort/dedupe package specifiers.
File: distribution/build.py Lines: 73-101
- parts = line.replace("uv ", "RUN ", 1).split(" ", 3) + parts = line.replace("uv ", "RUN ", 1).split(" ", 3) if len(parts) >= 4: # We have packages to sort cmd_parts = parts[:3] # "RUN pip install" - packages = sorted( - set(parts[3].split()) - ) # Sort the package names and remove duplicates + raw_tokens = parts[3].split() + # Keep options (and their values) in-order; collect specifiers to sort/dedupe + option_value_flags = {"--index-url", "--extra-index-url"} + options = [] + specs = [] + i = 0 + while i < len(raw_tokens): + tok = raw_tokens[i] + if tok.startswith("--"): + if tok in option_value_flags and i + 1 < len(raw_tokens): + options.extend([tok, raw_tokens[i + 1]]) + i += 2 + continue + options.append(tok) + else: + specs.append(tok) + i += 1 + # Dedupe while preserving order for options; sort/dedupe specs for determinism + from collections import OrderedDict + options = list(OrderedDict.fromkeys(options)) + specs = sorted(set(specs)) + packages = options + specsrg search for 'RUN pip install' lines with both --index-url and --extra-index-url in distribution/Containerfile returned no matches; manual verification required.
derekhiggins
changed the title
derekhiggins
force-pushed
the
escape-versions
branch
from
4742f54 to
09002ef
Compare
- Add quotes around packages with > or < characters to prevent bash redirection - Prevents files like '=0.12.0' in final container - Affects packages: datasets>=4.0.0, mcp>=1.8.1, pymilvus>=2.4.10, torchao>=0.12.0 Signed-off-by: Derek Higgins <derekh@redhat.com>
derekhiggins
force-pushed
the
escape-versions
branch
from
09002ef to
22386fc
Compare
chore: bump wheel release to use 0.4.2
3 participants
--
Summary by CodeRabbit
Bug Fixes
Chores