feat: add LLVM setup script for modular installation#116
Merged
cmagina merged 14 commits intoredhat-et:mainfrom Apr 8, 2026
Merged
feat: add LLVM setup script for modular installation#116cmagina merged 14 commits intoredhat-et:mainfrom
cmagina merged 14 commits intoredhat-et:mainfrom
Conversation
This was referenced Feb 7, 2026
cmagina
force-pushed
the
pr-02-llvm-setup-script
branch
from
6d03c97 to
c911c9d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new devinstall script and integrates it into Makefile, Dockerfiles, and setup scripts; updates several scripts to use ~/.bashrc.d snippets; updates container runtime env/mounts; and expands GitHub Actions workflow path filters to trigger on changes to the new script. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant CB as Container Build
participant Script as devinstall_llvm.sh
participant Git as LLVM Git Server
Dev->>GH: push or PR touching `scripts/devinstall_llvm.sh`
GH->>CB: trigger image build job
CB->>CB: COPY `devinstall_llvm.sh` into image
CB->>Script: RUN `devinstall_llvm source` (CUSTOM_LLVM path)
Script->>Git: git clone / fetch `llvm-project`
Script->>CB: install Python deps and configure build path (writes ~/.bashrc.d snippet)
CB-->>GH: image build completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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 |
cmagina
force-pushed
the
pr-02-llvm-setup-script
branch
4 times, most recently
from
485d1cf to
5dd5ceb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dockerfiles/Dockerfile.triton (1)
52-65:⚠️ Potential issue | 🟠 MajorSplit the
ENVthat derivesPYTHONPATH.Line 60 references
PYTHON_VERSION, but Docker ENV expansion uses the value of variables at the start of the instruction. Variables defined earlier in the sameENVblock are not available for substitution, soPYTHONPATHwill expand incorrectly. Split into separateENVinstructions:ENV PYTHON_VERSION=3.12 ENV PYTHONPATH=/workspace/lib/python$PYTHON_VERSION/site-packages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfiles/Dockerfile.triton` around lines 52 - 65, The ENV block sets PYTHON_VERSION and then defines PYTHONPATH using that variable, but Docker expands ENV variables only from prior ENV instructions, so PYTHONPATH will not get the intended value; split the combined ENV into at least two instructions so PYTHON_VERSION is set first and then PYTHONPATH is set using it (e.g., separate ENV PYTHON_VERSION=3.12 and ENV PYTHONPATH=/workspace/lib/python$PYTHON_VERSION/site-packages), keeping other vars either in earlier or subsequent ENV lines as needed and preserving PATH, TRITON_* and other variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockerfiles/Dockerfile.llvm`:
- Around line 20-21: The Dockerfile currently hard-codes the LLVM checkout path
to "${HOME}/llvm-project" which bypasses workspace semantics; change the
commands that source devinstall_llvm and cd into the checkout to use the
repository/workspace directory variable (e.g., replace "${HOME}/llvm-project"
with "${WORKSPACE}/llvm-project" or an existing workspace env like
"${GITHUB_WORKSPACE}"/llvm-project) and ensure the devinstall_llvm script itself
does not assume $HOME for its checkout; update any calls to devinstall_llvm and
the cd that reference "${HOME}/llvm-project" to use the workspace variable
instead.
- Around line 18-19: The Dockerfile runs "python3 -m pip install --upgrade uv"
then invokes "uv pip install --upgrade cmake ninja sccache pybind11" without a
virtualenv; change the second invocation to pass the --system flag so it becomes
"uv pip install --system --upgrade ..." (i.e., add --system to the "uv pip
install" command) to ensure installation works outside a virtual environment.
In `@Makefile`:
- Around line 52-54: The MAX_JOBS assignment in the Makefile currently uses
`$(nproc --all)`, which mistakenly treats the command as a Make variable; change
the assignment for MAX_JOBS to use the Make shell function so the command is
executed (i.e., replace the current expansion with `$(shell nproc --all)`) so
MAX_JOBS correctly captures the CPU count; update the line that defines MAX_JOBS
in the Makefile (symbol: MAX_JOBS) accordingly.
In `@scripts/devcreate_user.sh`:
- Around line 110-117: The fix_permissions() function is chowning the current
root HOME and using the wrong UID/GID variables; change it to look up the new
user's home and IDs and use those consistently: inside fix_permissions() obtain
the new user's home with something like getent passwd "$USERNAME" to set
HOME_DIR (use the passwd home instead of $HOME), and use USER_ID and GROUP_ID
(not USER_UID/USER_GID) when calling chown and creating /run/user (e.g., mkdir
-p "/run/user/$USER_ID" and chown "$USER_ID:$GROUP_ID" "/run/user/$USER_ID");
also keep the WORKSPACE existence test quoted and conditional ( [[ -n
"$WORKSPACE" && -d "$WORKSPACE" ]] ) to avoid set -u failures.
In `@scripts/devinstall_llvm.sh`:
- Around line 66-71: The helper install_build_deps() currently only installs
Python deps and must also provision LLVM's system build prerequisites when
INSTALL_LLVM=source is used; update install_build_deps() (and reference
INSTALL_LLVM or LLVM_DIR) to detect the host package manager (apt/yum/pacman),
require/root-check, and install a curated list of system packages needed to
build LLVM (or read them from a new file such as mlir/build-deps.system or
scripts/llvm-system-deps) before attempting Python installs; ensure the new
logic is idempotent and logs failures clearly so the source build flow no longer
relies on upstream Dockerfiles.
In `@scripts/devinstall_triton.sh`:
- Around line 205-214: The source case currently runs setup_src,
install_build_deps, and install_deps but never builds or installs Triton, so add
the source-install step: after install_deps in the source) branch call the
function that builds/installs Triton from source (e.g., build_triton,
install_triton_from_source, or install_triton if that helper exists) so the
container ends up with Triton installed (mirror how release) and ensure the
function is invoked in the source) block.
In `@scripts/devsetup.sh`:
- Around line 22-29: The script is incorrectly preserving the original HOME
across runuser via the SAVE_VARS whitelist, causing subsequent installs to
append into root's home; remove "HOME" from the SAVE_VARS array and any other
places that copy or export HOME (the array SAVE_VARS and the code that
exports/propagates those variables), and instead ensure when switching users
with runuser you either unset HOME from preserved vars or explicitly set HOME to
the target user's home (e.g., obtain the passwd entry for USERNAME and export
that HOME) so post-switch install steps operate in the correct user home.
- Around line 66-71: The devsetup ordering causes devinstall_llvm to clone the
wrong LLVM ref because LLVM_GITREF is only written by
scripts/devinstall_triton.sh after Triton is cloned; change the setup so
LLVM_GITREF is determined before calling devinstall_llvm — either call
run_as_user devinstall_triton (or a new lightweight helper that reads Triton's
LLVM hash) prior to run_as_user devinstall_llvm, or extract LLVM_GITREF from the
Triton repo/config and export it early; update scripts/devsetup.sh to ensure
LLVM_GITREF is exported/set before invoking the devinstall_llvm function.
- Around line 45-52: The run_as_user helper currently always invokes runuser
when USERNAME is set, which breaks the "already-unprivileged" container path;
change run_as_user to first detect if the script is already running as a
non-root UID (use id -u and id -g) and, if so, execute commands directly
(without runuser) and skip any actions that attempt to mutate /etc; in practice
update the run_as_user function to: check if id -u != 0 then run "$@" directly,
otherwise keep the existing SAVED_VARS -> ENV_LIST logic and call runuser -u
"$USERNAME" -- "$@"; also ensure other setup blocks that mutate /etc are gated
behind a root check so they are no-ops when the container is started with host
UID/GID.
In `@scripts/ldpretend.sh`:
- Line 32: The test [ -d "${PYTHONPATH}/nvidia" ] can fail under set -u because
PYTHONPATH may be unset; update the conditional around the
"${PYTHONPATH}/nvidia" check (the if that currently tests that path) to first
ensure PYTHONPATH is defined/non-empty (e.g. check -n "${PYTHONPATH:-}" or use
parameter expansion with a safe default) before testing for the directory, so
the script doesn’t abort under set -u.
---
Outside diff comments:
In `@dockerfiles/Dockerfile.triton`:
- Around line 52-65: The ENV block sets PYTHON_VERSION and then defines
PYTHONPATH using that variable, but Docker expands ENV variables only from prior
ENV instructions, so PYTHONPATH will not get the intended value; split the
combined ENV into at least two instructions so PYTHON_VERSION is set first and
then PYTHONPATH is set using it (e.g., separate ENV PYTHON_VERSION=3.12 and ENV
PYTHONPATH=/workspace/lib/python$PYTHON_VERSION/site-packages), keeping other
vars either in earlier or subsequent ENV lines as needed and preserving PATH,
TRITON_* and other variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d90aee17-52b6-47e5-a461-828eaf542c64
📒 Files selected for processing (18)
.github/workflows/amd-image.yml.github/workflows/cpu-image.yml.github/workflows/llvm-image.yml.github/workflows/nvidia-image.ymlMakefiledockerfiles/Dockerfile.llvmdockerfiles/Dockerfile.tritondockerfiles/Dockerfile.triton-amddockerfiles/Dockerfile.triton-cpudockerfiles/entrypoint.shdockerfiles/user.shscripts/devcreate_user.shscripts/devinstall_llvm.shscripts/devinstall_software.shscripts/devinstall_triton.shscripts/devsetup.shscripts/entrypoint.shscripts/ldpretend.sh
💤 Files with no reviewable changes (2)
- dockerfiles/user.sh
- dockerfiles/entrypoint.sh
Add scripts/devinstall_llvm.sh to support LLVM installation and configuration within containers. This script: - Downloads LLVM source from GitHub when not mounted as a volume - Installs build dependencies for LLVM compilation - Provides a clean interface for LLVM integration The script supports the INSTALL_LLVM environment variable with 'source' or 'skip' options, enabling flexible LLVM setup for different container configurations. This is part of the modular script architecture introduced in PR redhat-et#115. Signed-off-by: Craig Magina <cmagina@redhat.com>
cmagina
force-pushed
the
pr-02-llvm-setup-script
branch
from
cd3ff34 to
ad371a0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
159-161: Initializeinstall_llvmbefore the conditional to avoid environment bleed-through.At lines 159-161,
install_llvmis only set whenCUSTOM_LLVM=false. If the caller environment already hasinstall_llvmexported, it will persist whenCUSTOM_LLVM=true, causing stale values to be passed to the container at lines 164 and 167. Initializeinstall_llvm=""before the conditional to match the defensive pattern already used forport_arg(lines 155-158).Proposed fix
+ install_llvm=""; \ if [ "$(CUSTOM_LLVM)" = "false" ]; then \ install_llvm="-e INSTALL_LLVM=$(INSTALL_LLVM)"; \ fi; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 159 - 161, The variable install_llvm is only set inside the CUSTOM_LLVM conditional, which lets an exported install_llvm from the caller leak through when CUSTOM_LLVM=true; to fix, initialize install_llvm="" before the conditional that checks CUSTOM_LLVM (same defensive pattern used for port_arg) so install_llvm is always defined and won't pass stale values into the container commands that reference install_llvm.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 159-161: The variable install_llvm is only set inside the
CUSTOM_LLVM conditional, which lets an exported install_llvm from the caller
leak through when CUSTOM_LLVM=true; to fix, initialize install_llvm="" before
the conditional that checks CUSTOM_LLVM (same defensive pattern used for
port_arg) so install_llvm is always defined and won't pass stale values into the
container commands that reference install_llvm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db9a8342-7340-41d6-93a4-366a93cab6b3
📒 Files selected for processing (11)
.github/workflows/amd-image.yml.github/workflows/cpu-image.yml.github/workflows/llvm-image.yml.github/workflows/nvidia-image.ymlMakefiledockerfiles/Dockerfile.llvmdockerfiles/Dockerfile.tritondockerfiles/Dockerfile.triton-amddockerfiles/Dockerfile.triton-cpuscripts/devinstall_llvm.shscripts/devsetup.sh
✅ Files skipped from review due to trivial changes (5)
- .github/workflows/nvidia-image.yml
- .github/workflows/cpu-image.yml
- .github/workflows/amd-image.yml
- .github/workflows/llvm-image.yml
- scripts/devinstall_llvm.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- dockerfiles/Dockerfile.triton
- dockerfiles/Dockerfile.triton-cpu
- dockerfiles/Dockerfile.triton-amd
- scripts/devsetup.sh
Also added a empty install_llvm var in the Makefile as suggested by coderabbit Signed-off-by: Craig Magina <cmagina@redhat.com>
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 `@scripts/devinstall_triton.sh`:
- Around line 108-118: The script currently only sets env vars when CUSTOM_LLVM
is true and /llvm-install exists, silently doing nothing if the directory is
missing; update the logic in scripts/devinstall_triton.sh so that when
${CUSTOM_LLVM:-false} is "true" and /llvm-install is NOT present you print a
clear error (mentioning /llvm-install and CUSTOM_LLVM) and exit non‑zero (e.g.,
exit 1), otherwise proceed to append the environment exports as before; modify
the existing if that references CUSTOM_LLVM and [ -d "/llvm-install" ] to add
the explicit failure branch to fail fast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: befb505d-971c-489d-9058-0eead7fc68ba
📒 Files selected for processing (2)
Makefilescripts/devinstall_triton.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
Signed-off-by: Craig Magina <cmagina@redhat.com>
hinriksnaer
reviewed
Apr 6, 2026
Collaborator
hinriksnaer
left a comment
hinriksnaer
left a comment
There was a problem hiding this comment.
Review of PR #116: feat: add LLVM setup script for modular installation. See inline comments for specific issues found.
hinriksnaer
reviewed
Apr 6, 2026
hinriksnaer
reviewed
Apr 6, 2026
hinriksnaer
reviewed
Apr 6, 2026
hinriksnaer
requested changes
Apr 6, 2026
Collaborator
hinriksnaer
left a comment
hinriksnaer
left a comment
There was a problem hiding this comment.
Left a couple of comments
Moving devinstall_triton to before devinstall_llvm will set the LLVM_GITREF variable, which devinstall_llvm will consume and ensure the matching version is checked out Signed-off-by: Craig Magina <cmagina@redhat.com>
Removes the requirement for --system as the devinstall_llvm script will just use pip since uv is not installed Signed-off-by: Craig Magina <cmagina@redhat.com>
Signed-off-by: Craig Magina <cmagina@redhat.com>
- Moved all adds to .bashrc to separate files under .bashrc.d - Calling devcreate_user all of the time as it checks for USERNAME and !root before creating a user - Added bashrc copy from /etc/skel as it includes .bashrc.d hooks Signed-off-by: Craig Magina <cmagina@redhat.com>
There should be no real clash having both as CUSTOM_LLVM is installed to /llvm-install and INSTALL_LLVM exists under /workspace/llvm-project. The only thing that the user would need to be aware of are the LLVM environment variables triton looks for when building. Signed-off-by: Craig Magina <cmagina@redhat.com>
Instead of appending to .bashrc and having to check if something already exists, just use a .bashrc.d file that can just be overwritten. - Also fixed a few small issues around create_user and HOME Signed-off-by: Craig Magina <cmagina@redhat.com>
Signed-off-by: Craig Magina <cmagina@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/devcreate_user.sh (1)
113-113:⚠️ Potential issue | 🟠 MajorQuote
WORKSPACEin the directory test to fix SC2086 and path-splitting risk.Line 113 currently uses unquoted
$WORKSPACE, which is also failing pre-commit shellcheck.Proposed fix
- [[ -n "${WORKSPACE:-}" && -d $WORKSPACE ]] && chown "$USERNAME:$GROUP_ID" -R "$WORKSPACE" + [[ -n "${WORKSPACE:-}" && -d "$WORKSPACE" ]] && chown "$USERNAME:$GROUP_ID" -R "$WORKSPACE"As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/devcreate_user.sh` at line 113, The directory test uses an unquoted WORKSPACE variable which can cause word splitting/SC2086; update the conditional and chown invocation to quote "$WORKSPACE" everywhere (the expression [[ -n "${WORKSPACE:-}" && -d "$WORKSPACE" ]] && chown "$USERNAME:$GROUP_ID" -R "$WORKSPACE") so the directory existence check and recursive chown use the quoted WORKSPACE variable to avoid path-splitting issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/devcreate_user.sh`:
- Around line 119-121: get_user_home() currently uses grep "$USERNAME" which can
match partial usernames and return the wrong home; change it to perform an exact
passwd lookup (e.g. use getent passwd "$USERNAME" or match the username field
exactly with awk/grep anchored to "^$USERNAME:") in the get_user_home function,
return the sixth field (home) only when a match exists, and ensure the returned
value is validated before it's exported/used later for permission fixes
(references: get_user_home function and the subsequent export/permission-fix
usage).
- Around line 123-133: The create_bashrc function only creates ${HOME}/.bashrc.d
when ${HOME}/.bashrc is missing; move or add an unconditional mkdir -p
${HOME}/.bashrc.d so .bashrc.d always exists even if .bashrc already exists,
keeping the existing install -m ... block for copying skel files and preserving
the use of ${HOME} and $USERNAME for logging.
---
Duplicate comments:
In `@scripts/devcreate_user.sh`:
- Line 113: The directory test uses an unquoted WORKSPACE variable which can
cause word splitting/SC2086; update the conditional and chown invocation to
quote "$WORKSPACE" everywhere (the expression [[ -n "${WORKSPACE:-}" && -d
"$WORKSPACE" ]] && chown "$USERNAME:$GROUP_ID" -R "$WORKSPACE") so the directory
existence check and recursive chown use the quoted WORKSPACE variable to avoid
path-splitting issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc6811ae-eff4-4635-ac6a-bb3820ae11eb
📒 Files selected for processing (7)
Makefiledockerfiles/Dockerfile.llvmscripts/devcreate_user.shscripts/devinstall_llvm.shscripts/devinstall_software.shscripts/devinstall_triton.shscripts/devsetup.sh
✅ Files skipped from review due to trivial changes (1)
- scripts/devinstall_llvm.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- scripts/devsetup.sh
Signed-off-by: Craig Magina <cmagina@redhat.com>
- Added guards to failure to get the users home directory - Always create the .bashrc.d directory when calling setup_bashrc Signed-off-by: Craig Magina <cmagina@redhat.com>
Signed-off-by: Craig Magina <cmagina@redhat.com>
Signed-off-by: Craig Magina <cmagina@redhat.com>
hinriksnaer
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is part 2 of 11 in the rework modernization effort. It adds the LLVM setup script as part of the modular framework installation architecture.
Changes:
scripts/setup_llvm.shfor LLVM installation and configurationFeatures:
INSTALL_LLVMenvironment variable withsourceorskipoptionsWhy this change?
LLVM is a foundational component for Triton and other frameworks. This dedicated setup script:
Testing:
Dependencies:
Related PRs:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Refactor