Add GitHub Actions build pipeline for driver packages - #7
Add GitHub Actions build pipeline for driver packages#7Manuel Richarz (mricharz) wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds ChangesNVIDIA Driver Build Automation
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Schedule,BuildWF: Daily Auto-Update Flow
participant Schedule as cron/dispatch
participant AutoUpdate as auto-update-matrix.yml
participant NVIDIA as nvidia.com
participant GH as GitHub/GHCR APIs
participant Repo as build-matrix.json / versions.json
participant BuildWF as build-nvidia.yml
Schedule->>AutoUpdate: trigger
AutoUpdate->>NVIDIA: scrape driver versions (prod/nf/beta/legacy)
NVIDIA-->>AutoUpdate: version strings
AutoUpdate->>GH: fetch Unraid kernel release tags
GH-->>AutoUpdate: X.Y.Z-Unraid tags (up to 3)
AutoUpdate->>GH: fetch toolkit/libnvidia-container tags
GH-->>AutoUpdate: runtime version tags
AutoUpdate->>GH: fetch gcc_* tag from GHCR
GH-->>AutoUpdate: gcc_tag
AutoUpdate->>Repo: jq mutate build-matrix.json + versions.json
AutoUpdate->>Repo: git commit + push (if changed)
AutoUpdate->>BuildWF: workflow_dispatch (if changed=true)
end
rect rgba(144, 238, 144, 0.5)
Note over BuildWF,Release: Matrix Build Flow
participant BuildWF as build-nvidia.yml
participant Prepare as prepare job
participant BuildJob as build job (matrix)
participant Docker as ghcr.io/ich777/unraid_kernel
participant Release as GitHub Release
BuildWF->>Prepare: start
Prepare->>Prepare: assemble matrix from build-matrix.json
Prepare-->>BuildJob: matrix + gcc_tag
BuildJob->>Release: check existing .txz asset
Release-->>BuildJob: found → skip=true
BuildJob->>Docker: run container: kernel src + nvidia_driver
Docker-->>BuildJob: *.txz + *.txz.md5
BuildJob->>BuildJob: verify md5 sidecars
BuildJob->>Release: ensure release exists + upload --clobber
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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.
🧹 Nitpick comments (3)
source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh (1)
79-81: Redundantsort -Vfor single-value beta.current field.The
get_beta()function appliessort -Vto.branches.beta.current, but this is a single version string (not an array likeproduction[]ornewfeature[]). The sort operation is harmless but unnecessary.This is consistent with how
update-check.shanddownload.shhandle beta - they also treat it as a single value. The redundant sort doesn't cause issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh` around lines 79 - 81, The get_beta() pipeline unnecessarily runs sort -V on a single-version value from jq '.branches.beta.current'; remove the redundant sort -V invocation in the command substitution inside function get_beta so it reads the single version string directly (keep the surrounding jq, awk and comm formatting intact), i.e., locate the get_beta() function and delete the "| sort -V" portion applied to the $(cat /tmp/nvidia_branches | jq -r '.branches.beta.current' ...) fragment to simplify the pipeline..github/workflows/build-nvidia.yml (2)
133-136: wget wrapper only strips first occurrence of --show-progress.The bash parameter expansion
${@/--show-progress/}only removes the first occurrence. If wget is called with--show-progressmultiple times (unlikely but possible), subsequent occurrences would remain and could cause issues in the container environment.Use global replacement pattern
- printf "#!/bin/bash\nexec ${real_wget} \"\${`@/--show-progress/`}\"" > /usr/local/bin/wget + printf "#!/bin/bash\nexec ${real_wget} \"\${`@//--show-progress/`}\"" > /usr/local/bin/wgetNote the double slash
//for global replacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-nvidia.yml around lines 133 - 136, The wget wrapper currently uses the bash parameter expansion `${`@/--show-progress/`}` which only removes the first `--show-progress`; update the wrapper to use the global replacement form `${`@//--show-progress/`}` so all occurrences are stripped before calling the real_wget (adjust the exec call that invokes real_wget to use `"${`@//--show-progress/`}"` and keep the existing chmod/placement logic).
138-139: Potential issue with dummy argument when building proprietary modules.When
MODULE_TYPEis notopensource,BUILD_ARGis empty, and line 138 passes"dummy"tocompile.shsource command. However, line 139 passes empty${BUILD_ARG}tonvidia_driver. This asymmetry may cause issues:
- The
sourcecommand on line 138 receives"dummy"as$2, but this only affects the initial sourcing context- The
nvidia_drivercall on line 139 receives an empty second argument when proprietaryLooking at the relevant snippet from
compile.sh:19-35, the function checksif [ "${2}" == "opensource" ]and falls through to proprietary logic otherwise, so an empty$2should work. However, the source line's dummy argument serves no purpose.Suggested simplification
- source source/compile.sh "${DRIVER_VERSION}" ${BUILD_ARG:-"dummy"} - nvidia_driver "${DRIVER_VERSION}" ${BUILD_ARG} + source source/compile.sh + nvidia_driver "${DRIVER_VERSION}" ${BUILD_ARG}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-nvidia.yml around lines 138 - 139, The source invocation passes a hardcoded "dummy" as the second arg while nvidia_driver uses ${BUILD_ARG}, causing inconsistency; remove the literal "dummy" and make the source call use the same expansion as nvidia_driver (e.g., source source/compile.sh "${DRIVER_VERSION}" "${BUILD_ARG:-}" or simply "${BUILD_ARG}") so both callers (the source of compile.sh and the nvidia_driver function) receive the same second parameter and the compile.sh check (if [ "${2}" == "opensource" ]) behaves consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/build-nvidia.yml:
- Around line 133-136: The wget wrapper currently uses the bash parameter
expansion `${`@/--show-progress/`}` which only removes the first
`--show-progress`; update the wrapper to use the global replacement form
`${`@//--show-progress/`}` so all occurrences are stripped before calling the
real_wget (adjust the exec call that invokes real_wget to use
`"${`@//--show-progress/`}"` and keep the existing chmod/placement logic).
- Around line 138-139: The source invocation passes a hardcoded "dummy" as the
second arg while nvidia_driver uses ${BUILD_ARG}, causing inconsistency; remove
the literal "dummy" and make the source call use the same expansion as
nvidia_driver (e.g., source source/compile.sh "${DRIVER_VERSION}"
"${BUILD_ARG:-}" or simply "${BUILD_ARG}") so both callers (the source of
compile.sh and the nvidia_driver function) receive the same second parameter and
the compile.sh check (if [ "${2}" == "opensource" ]) behaves consistently.
In `@source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh`:
- Around line 79-81: The get_beta() pipeline unnecessarily runs sort -V on a
single-version value from jq '.branches.beta.current'; remove the redundant sort
-V invocation in the command substitution inside function get_beta so it reads
the single version string directly (keep the surrounding jq, awk and comm
formatting intact), i.e., locate the get_beta() function and delete the "| sort
-V" portion applied to the $(cat /tmp/nvidia_branches | jq -r
'.branches.beta.current' ...) fragment to simplify the pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da1e7988-4ebf-44b1-8ddf-bf2b900d6022
📒 Files selected for processing (6)
.github/workflows/build-nvidia.ymlbuild-matrix.jsonsource/usr/local/emhttp/plugins/nvidia-driver/include/download.shsource/usr/local/emhttp/plugins/nvidia-driver/include/exec.shsource/usr/local/emhttp/plugins/nvidia-driver/include/update-check.shsource/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page
Manuel Richarz (mricharz)
changed the title
|
Sorry I've overlooked this PR, however SimonFair already made a PR with changes so that users only can select driver versions which match their GPU in the system.
Please open up a post in the support thread next time. The plugin will be changed in the coming months so that the OpenSource packages will be the default and only the legacy driver (version 580.x) will be the proprietary on.
Can you describe that a bit more in detail? Sure injection would be also be possible but if someone wants to do something malicious to your server and already has access to your server the plugin isn't the first attack vector I think.
How long does the build from the driver take on GitHub Actions?
There shouldn't be much API calls in the first place since the files are cached or do I get the new method wrong? Is that even necessary? One additional question about libnvidia-container: 1.19.0 is still in RC state and RC branches are not auto compiled so the RC version is not available yet for Unraid, I also don't want to ship RC components with this driver at least when it's avoidable. BTW A few minor changes will be made when 1.19.0 is released anyways, but that shouldn't mean anything for that PR. |
Actions took around 7 minuten for 12 artifacts
sure, injection can only happen if someone already has access to the UI. But security consists not only of one big wall. today we combine multiple vulneabilities to get a full security breach.
|
So you are building 12 drivers do I get that right?
I completely get your point but I don't think that this counts as a breach since someone already needs access to the Server, however this is always a big discussion anyways and I have nothing against the changes. Just saying.
Sorry, I haven't fully looked through the routine but seems okay to me.
Just click on Sorry that I've overlooked the issue and also this PR, but somehow GitHub doesn't send me notifications about new Issues anymore. |
cba5b1f to
63a120d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh (2)
16-24: Consider modernizing arithmetic and quoting variables.The cache logic correctly uses
FETCH_VERSIONS. For improved robustness and modern shell style:🔧 Proposed improvements
if [ -f /tmp/nvidia_driver ]; then FILETIME=$(stat /tmp/nvidia_driver -c %Y) - DIFF=$(expr $CURENTTIME - $FILETIME) - if [ $DIFF -gt $CHK_TIMEOUT ]; then + DIFF=$((CURENTTIME - FILETIME)) + if [ "$DIFF" -gt "$CHK_TIMEOUT" ]; then FETCH_VERSIONS fi else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh` around lines 16 - 24, The branch that checks /tmp/nvidia_driver should use modern POSIX-safe arithmetic and quote variables: replace the expr call with arithmetic expansion (e.g., DIFF=$(( CURENTTIME - FILETIME ))), ensure all variable expansions are quoted when used in tests and commands (e.g., "$FILETIME", "$DIFF", "$CHK_TIMEOUT"), and quote the filename "/tmp/nvidia_driver" in the -f test; keep the FETCH_VERSIONS calls unchanged. Also verify the correct variable name for CURENTTIME (fix typo to CURRENTTIME if applicable) before using it in the arithmetic.
8-15: Good extraction of version fetching logic; consider quoting$DRIVERS.The
FETCH_VERSIONS()function nicely centralizes the API fetch logic and fixes the variable scope issue. However,$DRIVERSshould be double-quoted in the heredoc to prevent word splitting issues with unusual version strings.🔧 Proposed fix
FETCH_VERSIONS() { DRIVERS="$(wget -qO- https://api.github.com/repos/unraid/unraid-nvidia-driver/releases/tags/${KERNEL_V} | jq -r '.assets[].name' | grep -E -v '\.md5$' | sort -V)" - echo -n "$(grep ${PACKAGE} <<< "$DRIVERS" | awk -F "-" '{print $2}' | sort -V | uniq)" > /tmp/nvidia_driver + echo -n "$(grep "${PACKAGE}" <<< "$DRIVERS" | awk -F "-" '{print $2}' | sort -V | uniq)" > /tmp/nvidia_driver echo -n "$(grep nvos <<< "$DRIVERS" | awk -F "-" '{print $2}' | sort -V | uniq)" > /tmp/nvos_driver🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh` around lines 8 - 15, In FETCH_VERSIONS ensure you use quoted variables to prevent word-splitting and globbing: change uses of grep ${PACKAGE} <<< "$DRIVERS" and grep nvos <<< "$DRIVERS" to use quoted/safer forms (e.g., grep -- "${PACKAGE}" <<< "$DRIVERS" and grep -- "nvos" <<< "$DRIVERS") and keep the heredoc input as "$DRIVERS"; also quote expansions like "$(awk ...)" outputs where appropriate so all uses of DRIVERS and PACKAGE are protected (refer to FETCH_VERSIONS, DRIVERS, and the grep/awk pipeline).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 166-177: The jq filter is using .branches inside select() while .
is the current .extra_build item, so capture the root object (e.g., using "as
$root") before iterating .extra_builds[] and then compare each item's
.driver_version against $root.branches.production.driver_version,
$root.branches.newfeature.driver_version, and $root.branches.beta.driver_version
(with // "" fallback) so the select actually filters out branch versions; keep
the subsequent .extra_builds = (.extra_builds | unique_by(.driver_version))
dedupe step as-is.
- Around line 32-44: The current fallback only runs when RAW_DATA is empty, so
parsing failures leave RAW_DATA set but PRB/NFB/BETA/LEGACY empty; change the
fallback logic to detect parse failure by checking the extracted variables (PRB,
NFB, BETA, LEGACY) and trigger the fallback fetch when those are empty (e.g., if
all or the primary expected variables like PRB are empty) in addition to
RAW_DATA being empty; implement the fallback fetch block to run when parse
results are missing, reassign RAW_DATA and re-extract PRB/NFB/BETA/LEGACY, and
keep the final guard that errors out only if RAW_DATA remains empty or required
version vars are still empty after the fallback.
---
Nitpick comments:
In `@source/usr/local/emhttp/plugins/nvidia-driver/include/exec.sh`:
- Around line 16-24: The branch that checks /tmp/nvidia_driver should use modern
POSIX-safe arithmetic and quote variables: replace the expr call with arithmetic
expansion (e.g., DIFF=$(( CURENTTIME - FILETIME ))), ensure all variable
expansions are quoted when used in tests and commands (e.g., "$FILETIME",
"$DIFF", "$CHK_TIMEOUT"), and quote the filename "/tmp/nvidia_driver" in the -f
test; keep the FETCH_VERSIONS calls unchanged. Also verify the correct variable
name for CURENTTIME (fix typo to CURRENTTIME if applicable) before using it in
the arithmetic.
- Around line 8-15: In FETCH_VERSIONS ensure you use quoted variables to prevent
word-splitting and globbing: change uses of grep ${PACKAGE} <<< "$DRIVERS" and
grep nvos <<< "$DRIVERS" to use quoted/safer forms (e.g., grep -- "${PACKAGE}"
<<< "$DRIVERS" and grep -- "nvos" <<< "$DRIVERS") and keep the heredoc input as
"$DRIVERS"; also quote expansions like "$(awk ...)" outputs where appropriate so
all uses of DRIVERS and PACKAGE are protected (refer to FETCH_VERSIONS, DRIVERS,
and the grep/awk pipeline).
🪄 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: 09e28314-245e-4e01-86a0-2622a4a8fe52
📒 Files selected for processing (7)
.github/workflows/auto-update-matrix.yml.github/workflows/build-nvidia.ymlbuild-matrix.jsonsource/usr/local/emhttp/plugins/nvidia-driver/include/download.shsource/usr/local/emhttp/plugins/nvidia-driver/include/exec.shsource/usr/local/emhttp/plugins/nvidia-driver/include/update-check.shsource/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page
✅ Files skipped from review due to trivial changes (2)
- build-matrix.json
- .github/workflows/build-nvidia.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- source/usr/local/emhttp/plugins/nvidia-driver/include/download.sh
- source/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page
|
The current version also auto-updates the build matrix with new driver versions, unraid kernel versions and gcc_tag versions and automatically builds them. And this is how the UI looks: |
|
Thanks for the PR Manuel Richarz (@mricharz) as said above:
Would it be possible to turn that around? I also made a comment on the forums here: I also mentioned there if a user has more than one card is there an index for users to see? BTW I also just noticed that the GPU UUID is not shown anymore, is that by design or can they somehow display the UUID's? |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
.github/workflows/auto-update-matrix.yml (1)
146-168: Copy the branchmodule_typesintoextra_builds.In
.github/workflows/build-nvidia.yml, Lines 14-44, extra builds fan out directly fromextra_builds[].module_types[]. Hardcoding both types here widens support for pinned old versions instead of preserving what the branch declared.♻️ Suggested fix
- .extra_builds += [{"driver_version": $cur_prb, "module_types": ["proprietary", "opensource"]}] + .extra_builds += [{ + "driver_version": $cur_prb, + "module_types": (.branches.production.module_types // ["proprietary", "opensource"]) + }] ... - .extra_builds += [{"driver_version": $cur_nfb, "module_types": ["proprietary", "opensource"]}] + .extra_builds += [{ + "driver_version": $cur_nfb, + "module_types": (.branches.newfeature.module_types // ["proprietary", "opensource"]) + }] ... - .extra_builds += [{"driver_version": $cur_beta, "module_types": ["proprietary", "opensource"]}] + .extra_builds += [{ + "driver_version": $cur_beta, + "module_types": (.branches.beta.module_types // ["proprietary", "opensource"]) + }]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/auto-update-matrix.yml around lines 146 - 168, The extra_builds insertion currently hardcodes module_types to ["proprietary","opensource"] when rotating branches (see the .extra_builds += [...] lines and branches.production.driver_version / branches.newfeature.driver_version / branches.beta.driver_version); change each insertion to copy the module_types from the branch being rotated (e.g. use the branch's .branches.production.module_types, .branches.newfeature.module_types and .branches.beta.module_types respectively) instead of the hardcoded array so the pinned extra_build preserves the original branch's module_types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 241-247: The workflow currently triggers the downstream "Build
NVIDIA Driver Packages" run with a hardcoded ref (--ref master); update the gh
workflow run invocation in the "Trigger build workflow" step to use the current
branch ref (${{ github.ref_name }}) instead of "master" so the downstream build
runs on the same branch where the commit was made (replace the --ref master
argument with --ref ${{ github.ref_name }} in the gh workflow run command).
In `@source/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page`:
- Around line 399-401: The three external anchor elements that open in a new tab
(the links with hrefs
"https://forums.unraid.net/topic/98978-plugin-nvidia-driver/",
"https://github.com/unraid/unraid-nvidia-driver", and
"https://www.nvidia.com/en-us/drivers/unix/" which currently use
target="_blank") should include rel="noopener noreferrer" to prevent
reverse-tabnabbing; update those <a> tags to add rel="noopener noreferrer"
alongside the existing target attribute.
- Around line 260-266: The echoed system-derived variables ($installed_v,
$module_type_display, $cuda_v, $kernel_v) are emitted raw into the HTML; update
each echo to output an HTML-escaped version (e.g., wrap values with
htmlspecialchars(..., ENT_QUOTES, 'UTF-8') or equivalent) so
command/file-derived data is safely encoded; ensure you change echoes in the
Driver Version, Kernel Module, CUDA Version (conditional), and Kernel lines to
use the escaping helper and keep the conditional logic for $cuda_v and $gpu_rows
unchanged.
- Around line 342-346: The code currently uses krsort($sorted) which sorts by
array keys and can misorder the pinned versions coming from $eachlines; replace
the key-based sort with a value-based, version-aware sort (e.g., use
usort($sorted, 'version_compare') and then reverse for descending order, or
rsort($sorted, SORT_NATURAL)) so the loop that calls
mk_option(trim($selected_v), $ver, 'v'.$ver, 'data-type="proprietary"') iterates
versions sorted correctly by their version strings rather than by array index.
- Around line 384-386: The label element uses for="updata_check_selected" but
the corresponding select lacks an id, breaking the label-control binding; update
the select element (the <select name="updata_check_selected">) to include
id="updata_check_selected" so the label correctly associates with the control
(no changes needed to mk_option usage).
- Around line 211-239: The filterVersions function currently always hides driver
options based on module_type, removing the prior user-overridable
"guidance-only" behavior; modify filterVersions to first check a user-controlled
override flag (preserve whatever UI element controls GPU-based filtering—e.g., a
checkbox or input that was used previously) and if that flag indicates filtering
is disabled, skip any hiding logic so drv_version shows all options; otherwise
proceed with the existing logic that reads module_type, computes allowed,
toggles option and optgroup display, and updates selectedIndex and
updateAutoUpdate(); also restore similar override handling where module-type
choice is conditionally removed (the code around lines 315-322) so the UI still
allows users to disable GPU-based filtering and pick any version.
- Around line 315-329: The page is allowing actions during conflict by emitting
a hidden module_type and leaving the update controls enabled; change the else
branch that runs when $is_conflict (and when $force_opensource/$force_legacy are
set) to not inject an implicit hidden `#module_type` value and to render the UI as
non-actionable — e.g., remove the hidden input creation and add a
disabled/hidden state for the Update & Download control or server-side gate the
submit action when $is_conflict is true; make these changes around the
conditional that uses $is_conflict, $force_legacy and $force_opensource and
target the "module_type" element and the Update & Download control so no
install/update can proceed while a conflict exists.
---
Nitpick comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 146-168: The extra_builds insertion currently hardcodes
module_types to ["proprietary","opensource"] when rotating branches (see the
.extra_builds += [...] lines and branches.production.driver_version /
branches.newfeature.driver_version / branches.beta.driver_version); change each
insertion to copy the module_types from the branch being rotated (e.g. use the
branch's .branches.production.module_types, .branches.newfeature.module_types
and .branches.beta.module_types respectively) instead of the hardcoded array so
the pinned extra_build preserves the original branch's module_types.
🪄 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: 48ec1f5a-db5a-44ae-929d-b3bb881db628
📒 Files selected for processing (3)
.github/workflows/auto-update-matrix.ymlbuild-matrix.jsonsource/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page
✅ Files skipped from review due to trivial changes (1)
- build-matrix.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/auto-update-matrix.yml (1)
12-15: Add workflow concurrency control to avoid push/dispatch races.Manual and scheduled runs can overlap and race on commit/push + downstream dispatch. A workflow-level
concurrencygroup would make this deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/auto-update-matrix.yml around lines 12 - 15, Add a workflow-level concurrency block to prevent overlapping runs: at the top-level of the YAML (above jobs) add a concurrency key with a stable group string like "auto-update-matrix-${{ github.ref }}" (or include github.workflow/github.event_name as needed) and set cancel-in-progress: true; this will ensure the detect-and-update job and any manual/scheduled runs do not race on push/dispatch.source/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page (1)
344-348: Consider escaping version strings in option labels for defense-in-depth.While the version values (
$latest_v,$latest_prb_v, etc.) come from controlled backend sources and contain version patterns like "535.104.05", applyinghtmlspecialchars()would provide defense-in-depth against any future changes to version string sources.🛡️ Optional defensive escaping
<optgroup label="Follow Branch (auto-update)" data-group="branch"> - <?php echo mk_option($selected_v, 'latest', 'Latest — v'.$latest_v); ?> - <?php if (!empty(trim($latest_prb_v))) echo mk_option($selected_v, 'latest_prb', 'Production — v'.$latest_prb_v); ?> - <?php if (!empty(trim($latest_nfb_v))) echo mk_option($selected_v, 'latest_nfb', 'New Feature — v'.$latest_nfb_v); ?> - <?php if (!empty(trim($latest_beta_v))) echo mk_option($selected_v, 'latest_beta', 'Beta — v'.$latest_beta_v); ?> - <?php if (!empty(trim($latest_nos_v))) echo mk_option($selected_v, 'latest_nos', 'Open Source — v'.$latest_nos_v); ?> + <?php echo mk_option($selected_v, 'latest', 'Latest — v'.htmlspecialchars($latest_v)); ?> + <?php if (!empty(trim($latest_prb_v))) echo mk_option($selected_v, 'latest_prb', 'Production — v'.htmlspecialchars($latest_prb_v)); ?> + <?php if (!empty(trim($latest_nfb_v))) echo mk_option($selected_v, 'latest_nfb', 'New Feature — v'.htmlspecialchars($latest_nfb_v)); ?> + <?php if (!empty(trim($latest_beta_v))) echo mk_option($selected_v, 'latest_beta', 'Beta — v'.htmlspecialchars($latest_beta_v)); ?> + <?php if (!empty(trim($latest_nos_v))) echo mk_option($selected_v, 'latest_nos', 'Open Source — v'.htmlspecialchars($latest_nos_v)); ?>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page` around lines 344 - 348, The option labels currently interpolate version variables ($latest_v, $latest_prb_v, $latest_nfb_v, $latest_beta_v, $latest_nos_v) directly into the mk_option calls; update those label strings to escape the version values using htmlspecialchars (e.g., htmlspecialchars($latest_v, ENT_QUOTES, 'UTF-8')) before concatenation so mk_option receives HTML-escaped labels for defense-in-depth while leaving the variable sources unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 147-166: The rotation logic for branches (variables
$cur_prb/$cur_nfb/$cur_beta and assignments like
.branches.production.driver_version) can add empty or "null" driver_version
entries into .extra_builds; update each rotation condition to also require the
old/current value is non-empty/non-"null" before pushing it: when checking (if
($new_prb != "") and ($cur_prb != $new_prb) ...) also ensure $cur_prb is not
empty or the string "null" (and do the same for $cur_nfb and $cur_beta) so you
only append valid previous driver_version values to .extra_builds and avoid
creating invalid entries.
- Around line 33-45: The workflow currently only fails when RAW_DATA is empty;
update the fallback parsing block to verify that required parsed variables (PRB,
NFB, BETA, and LEGACY) are present after fetching and parsing and fail fast if
any are empty: after the lines that set PRB, NFB, BETA, and LEGACY, add a guard
that echoes an error (e.g., "::error::Missing NVIDIA driver versions: ...") and
exits non-zero when any of PRB/NFB/BETA/LEGACY are empty so the job does not
continue with incomplete outputs.
---
Nitpick comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 12-15: Add a workflow-level concurrency block to prevent
overlapping runs: at the top-level of the YAML (above jobs) add a concurrency
key with a stable group string like "auto-update-matrix-${{ github.ref }}" (or
include github.workflow/github.event_name as needed) and set cancel-in-progress:
true; this will ensure the detect-and-update job and any manual/scheduled runs
do not race on push/dispatch.
In `@source/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page`:
- Around line 344-348: The option labels currently interpolate version variables
($latest_v, $latest_prb_v, $latest_nfb_v, $latest_beta_v, $latest_nos_v)
directly into the mk_option calls; update those label strings to escape the
version values using htmlspecialchars (e.g., htmlspecialchars($latest_v,
ENT_QUOTES, 'UTF-8')) before concatenation so mk_option receives HTML-escaped
labels for defense-in-depth while leaving the variable sources unchanged.
🪄 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: d82674e2-fbfb-422f-b14f-9afe88458511
📒 Files selected for processing (3)
.github/workflows/auto-update-matrix.yml.gitignoresource/usr/local/emhttp/plugins/nvidia-driver/nvidia-driver.page
✅ Files skipped from review due to trivial changes (1)
- .gitignore
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/auto-update-matrix.yml (1)
37-49:⚠️ Potential issue | 🟠 MajorStill succeeds on partial NVIDIA parse results.
The fallback and final guard still treat
BETAandLEGACYas optional. If NVIDIA changes only those sections, this job exits successfully and silently leaves the beta/legacy tracks stale.🛠️ Suggested fix
- if [ -z "${RAW_DATA}" ] || { [ -z "${PRB}" ] && [ -z "${NFB}" ] && [ -z "${BETA}" ]; }; then + if [ -z "${RAW_DATA}" ] || [ -z "${PRB}" ] || [ -z "${NFB}" ] || [ -z "${BETA}" ] || [ -z "${LEGACY}" ]; then RAW_DATA="$(wget -qO- https://forums.developer.nvidia.com/t/current-graphics-driver-releases/28500 | tidy -quiet -wrap 4096 2>/dev/null || true)" PRB="$(echo "${RAW_DATA}" | grep -i "^Current production branch" | grep -oE '\b[0-9]+\.[0-9]+(\.[0-9]+)?\b')" NFB="$(echo "${RAW_DATA}" | grep -i "^Current new feature branch" | grep -oE '\b[0-9]+\.[0-9]+(\.[0-9]+)?\b')" BETA="$(echo "${RAW_DATA}" | grep -i "^Current beta" | grep -oE '\b[0-9]+\.[0-9]+(\.[0-9]+)?\b')" LEGACY="$(echo "${RAW_DATA}" | grep -i -A1 '>Legacy releases' | tail -1 | grep -oE '\b[0-9]+\.[0-9]+(\.[0-9]+)?\b')" fi - if [ -z "${RAW_DATA}" ] || { [ -z "${PRB}" ] && [ -z "${NFB}" ]; }; then - echo "::error::Failed to fetch NVIDIA driver data from both sources" + if [ -z "${RAW_DATA}" ] || [ -z "${PRB}" ] || [ -z "${NFB}" ] || [ -z "${BETA}" ] || [ -z "${LEGACY}" ]; then + echo "::error::Missing NVIDIA driver versions: PRB=${PRB:-<empty>} NFB=${NFB:-<empty>} BETA=${BETA:-<empty>} LEGACY=${LEGACY:-<empty>}" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/auto-update-matrix.yml around lines 37 - 49, The fallback logic and final guard treat BETA and LEGACY as optional, allowing partial parses to pass; update the checks so that the wget/tidy fallback block and the final validation require PRB, NFB, BETA and LEGACY to be present (i.e., change occurrences of "{ [ -z \"${PRB}\" ] && [ -z \"${NFB}\" ]; }" and "{ [ -z \"${PRB}\" ] && [ -z \"${NFB}\" ]; }" to ensure all four variables PRB, NFB, BETA, and LEGACY are non-empty), and emit the error/exit if any of PRB, NFB, BETA or LEGACY are missing after both source attempts; keep using RAW_DATA for the fetch fallback as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 222-223: The current jq runtime tag check uses test("^[0-9]")
which only validates the first character; update the conditional tests for
$toolkit and $libnvidia (the expressions that set
.runtimes.containertoolkit.current and .runtimes.libnvidia.current) to use the
stricter regex test("^[0-9]+(\\.[0-9]+)+$") so only semantic versions like X.Y.Z
(or X.Y) pass and unstable tags (e.g. 1.19.0-rc1 or 1bad) are rejected.
- Around line 186-189: The compound command using "jq ... >
build-matrix.json.tmp && mv build-matrix.json.tmp build-matrix.json" masks jq
failures; change this to run the jq command and check its exit before moving the
tmp file so a jq error fails the step: run the jq redirection as a standalone
command (the "jq ... > build-matrix.json.tmp" invocation), ensure the shell
exits on non-zero (rely on set -e or explicitly test $? or use "|| exit 1"), and
only run "mv build-matrix.json.tmp build-matrix.json" if the jq command
succeeded, leaving the following "echo" and "cat build-matrix.json" unchanged.
---
Duplicate comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 37-49: The fallback logic and final guard treat BETA and LEGACY as
optional, allowing partial parses to pass; update the checks so that the
wget/tidy fallback block and the final validation require PRB, NFB, BETA and
LEGACY to be present (i.e., change occurrences of "{ [ -z \"${PRB}\" ] && [ -z
\"${NFB}\" ]; }" and "{ [ -z \"${PRB}\" ] && [ -z \"${NFB}\" ]; }" to ensure all
four variables PRB, NFB, BETA, and LEGACY are non-empty), and emit the
error/exit if any of PRB, NFB, BETA or LEGACY are missing after both source
attempts; keep using RAW_DATA for the fetch fallback as before.
🪄 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: de1295c9-bf7e-432e-9f05-8e0604f39ddd
📒 Files selected for processing (1)
.github/workflows/auto-update-matrix.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/auto-update-matrix.yml (1)
106-119: Minor robustness: GCC tag fallback could validate the retrieved value.If
build-matrix.jsonever lacks.gcc_tag,jq -rreturns the literal string"null", which would pass the!= ""check at line 143 and corrupt the field. This is unlikely given the current JSON structure, but for defense-in-depth:♻️ Proposed improvement
if [ -z "${GCC_TAG}" ]; then echo "::warning::Could not detect GCC tag, keeping current" - GCC_TAG=$(jq -r '.gcc_tag' build-matrix.json) + GCC_TAG=$(jq -r '.gcc_tag // ""' build-matrix.json) + if [ -z "${GCC_TAG}" ] || [ "${GCC_TAG}" = "null" ]; then + echo "::error::No valid gcc_tag found in build-matrix.json" + exit 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/auto-update-matrix.yml around lines 106 - 119, The current GCC tag detection writes whatever jq returns (including the literal "null") into GCC_TAG and exports it; update the logic around GCC_TAG so after attempting to detect it from the registry and before falling back to build-matrix.json you also validate that the value is non-empty and not the literal "null" (i.e., test for -z OR equal to "null"), and if invalid read fallback with jq -r '.gcc_tag' and validate that result as well; only write a valid tag to GITHUB_OUTPUT (and emit the warning/keep-current path if both are invalid) — check and update the shell variables GCC_TAG, the jq call that reads '.gcc_tag', and the final echo to "$GITHUB_OUTPUT" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 106-119: The current GCC tag detection writes whatever jq returns
(including the literal "null") into GCC_TAG and exports it; update the logic
around GCC_TAG so after attempting to detect it from the registry and before
falling back to build-matrix.json you also validate that the value is non-empty
and not the literal "null" (i.e., test for -z OR equal to "null"), and if
invalid read fallback with jq -r '.gcc_tag' and validate that result as well;
only write a valid tag to GITHUB_OUTPUT (and emit the warning/keep-current path
if both are invalid) — check and update the shell variables GCC_TAG, the jq call
that reads '.gcc_tag', and the final echo to "$GITHUB_OUTPUT" accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a46221b-010b-4746-b37c-ba6128cb05e9
📒 Files selected for processing (1)
.github/workflows/auto-update-matrix.yml
|
I have had a look at the changes. With the changes in #8 it disables drivers that are not compatible with the card installed. But this PR shows all and does not disabled incompatible ones. 
There is no indication the driver is not working correctly. 
The PR does not seem to get the supported drivers for the GPU. The values were from my orginal plugin before coping the updated files. For the version what does pin mean i.e. is will not update. |
|
good catch. let me check. |
|
Any update on this Manuel Richarz (@mricharz) |
|
Can this PR be split into two parts. One for the driver builds and a seperate PR for the frontend. |
|
Manuel Richarz (@mricharz) is there any update on this since I plan to deprecate my build pipeline for the Nvidia driver soon and I find it really impractical to have now two repositories from the Unraid Nvidia driver and not building it here in official repository. SimonFair what is the state switching the underlying distribution from Unraid? Any choices made yet? I assume depending on the choice what distribution you are using in the future the build pipeline needs to change anyways. |
61c194a to
12e8cec
Compare
|
sorry took me a while, had a lot going on. i split it up like SimonFair said. this PR is only the build pipeline now, took out all the frontend stuff. frontend comes as its own PR on top of #8 this time instead of replacing it, so the per gpu filtering and the uuid stay. you were right, my version dropped too much of #8. the pipeline builds to whatever repo it runs in (github.repository) and reads the kernel tags + runtime versions from the unraid repos, so in the official repo it just builds the packages there. around 7 min for 12 artifacts, should cover replacing the jenkins build. also the 1.19.0 rc thing isnt an issue anymore, 1.19.1 is out. fixed the review comments too (concurrency, gcc_tag check, fallback when a branch is missing). frontend PR follows, will link it here. |
Manuel Richarz (mricharz)
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/build-nvidia.yml (1)
1-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit least-privilege
permissions:block.The workflow has no
permissions:block, so it inherits the repo default token scope. Set a minimal top-level default and grantcontents: writeonly to thebuildjob (needed forgh release create/upload). Flagged by static analysis.🛡️ Suggested permissions scoping
on: workflow_dispatch: +permissions: + contents: read + jobs: prepare: runs-on: ubuntu-latest @@ build: needs: prepare runs-on: ubuntu-latest + permissions: + contents: write timeout-minutes: 120🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-nvidia.yml around lines 1 - 11, The workflow lacks an explicit permissions block, which means it uses the repository's default token scope. Add a top-level permissions block at the workflow level with minimal/default permissions (such as read-only or empty permissions), and then add a permissions block specifically to the build job that grants contents: write permission, since that job needs write access for gh release create and upload operations.Source: Linters/SAST tools
.github/workflows/auto-update-matrix.yml (1)
253-259: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid expanding
github.ref_namedirectly in the run script.
github.ref_nameis attacker-influenceable (branch/tag names), and expanding it inline allows shell injection. Pass it (andgithub.repository) viaenvand quote the references. Flagged as a code-injection error by static analysis.🛡️ Suggested hardening
- name: Trigger build workflow if: steps.commit.outputs.changed == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + TARGET_REF: ${{ github.ref_name }} run: | echo "Changes committed, triggering build workflow..." - gh workflow run "Build NVIDIA Driver Packages" --repo ${{ github.repository }} --ref ${{ github.ref_name }} + gh workflow run "Build NVIDIA Driver Packages" --repo "${REPO}" --ref "${TARGET_REF}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/auto-update-matrix.yml around lines 253 - 259, In the "Trigger build workflow" step, the github.repository and github.ref_name variables are being directly expanded in the run script, creating a shell injection vulnerability. Move both variables to the env section of the step (adding GH_REPO and GH_REF or similar), then update the gh workflow run command to reference these environment variables with proper quoting using the format "$VARIABLE_NAME" instead of directly expanding the github context variables inline.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 229-230: The regex pattern in the test conditions for both the
toolkit and libnvidia version matching contains four backslashes before the dot
character, which is over-escaped in the YAML literal block context. This causes
the pattern to fail matching valid version strings like 1.19.0. To fix this,
reduce the four backslashes to two backslashes in both regex patterns on lines
229-230, so the escaped period character is properly interpreted by jq and
correctly matches the decimal separator in version numbers.
---
Nitpick comments:
In @.github/workflows/auto-update-matrix.yml:
- Around line 253-259: In the "Trigger build workflow" step, the
github.repository and github.ref_name variables are being directly expanded in
the run script, creating a shell injection vulnerability. Move both variables to
the env section of the step (adding GH_REPO and GH_REF or similar), then update
the gh workflow run command to reference these environment variables with proper
quoting using the format "$VARIABLE_NAME" instead of directly expanding the
github context variables inline.
In @.github/workflows/build-nvidia.yml:
- Around line 1-11: The workflow lacks an explicit permissions block, which
means it uses the repository's default token scope. Add a top-level permissions
block at the workflow level with minimal/default permissions (such as read-only
or empty permissions), and then add a permissions block specifically to the
build job that grants contents: write permission, since that job needs write
access for gh release create and upload operations.
🪄 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: 6c2c69a9-7549-4739-ab84-05efce49192d
📒 Files selected for processing (4)
.github/workflows/auto-update-matrix.yml.github/workflows/build-nvidia.yml.gitignorebuild-matrix.json
✅ Files skipped from review due to trivial changes (1)
- .gitignore
|
small clarification on the jenkins point: the action just uploads the built .txz files as release assets to the repo it runs in, with the same tag (kernel version) and the same naming as now (nvidia-/nvos---1.txz). so the plugin downloads them exactly like before, it doesnt matter if jenkins or the action built them. its a drop in, no change to the plugin needed. only thing it still needs is the kernel archive from ich777/unraid_kernel to build against, like you mentioned. |
619830b to
0337563
Compare
Provides an in-repo alternative to an external Jenkins instance for building the NVIDIA driver packages. - build-nvidia.yml: matrix build from build-matrix.json, containerized builds, checksum verification, release asset upload, skips builds whose release asset already exists - auto-update-matrix.yml: scheduled detection of new driver, Unraid kernel, container-runtime and gcc versions; updates the matrix and triggers the build workflow. Concurrency-guarded, fails fast when the primary source yields no production/new-feature version, retries the forum source when any branch is missing, and validates the gcc_tag fallback - build-matrix.json: CI inputs (gcc_tag, kernel_versions, per-branch driver_version + module_types, extra_builds)
0337563 to
62afae7
Compare
Discovery work is ongoing, build pipe line may need to change or there may be pre-built options. Anything as part of the build that may be needs to be factored in for a new distro? |
|
re the distro change: the action doesnt really add any new distro coupling. the only distro-specific part is the actual compile + package step. it pulls the unraid kernel source/toolchain from ich777/unraid_kernel and builds the .txz the same way as now. the matrix, version detection and release upload around it are distro-agnostic. so if the base distro changes, that compile+package step needs the same adjustments jenkins would need anyway, the action just orchestrates it. happy to adapt that step once the direction is clear. |
3 participants
Split out from the previous combined PR as requested, this one is only the build pipeline now. Frontend changes come separately on top of #8.
build-nvidia.yml builds the matrix from build-matrix.json (containerized, checksums, uploads the release assets, skips ones that already exist). Releases go to github.repository so it builds wherever it runs.
auto-update-matrix.yml checks for new driver/kernel/runtime/gcc versions on a schedule, updates the matrix and starts the build.
It also removes update-versions.yml, since auto-update-matrix.yml already keeps versions.json up to date (plus the build matrix and triggering the build), so the two would otherwise both write versions.json on different schedules.
Kernel tags and runtime versions are pulled from the unraid repos, so in the official repo it builds the official packages directly. around 7 min for 12 artifacts.