Run Nuitka standalone builds across CI platforms#61
Open
Ahmed-Hindy wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends the Nuitka standalone CI from Windows-only to a matrix build (Windows, Ubuntu, macOS). Adds cross-platform FFmpeg staging, refactors build packaging and path handling, verifies bundled FFmpeg and encoders in CI, and uploads per-platform artifacts. ChangesCross-OS Nuitka Build Pipeline
Sequence DiagramsequenceDiagram
participant GitHubActions
participant Runner
participant StageScript
participant BuildScript
participant ArtifactUploader
GitHubActions->>Runner: start matrix job (windows/linux/macos)
Runner->>StageScript: run scripts/stage_portable_ffmpeg.py (non-Windows)
StageScript-->>Runner: staged ffmpeg in vendor/ffmpeg/<platform>
Runner->>BuildScript: run scripts/build_nuitka.ps1 (bundle FFmpeg, produce zip)
BuildScript-->>Runner: produce dist-nuitka/*-standalone.zip
Runner->>Runner: smoke-test zip (extract, run ffmpeg -version, check encoders)
Runner->>ArtifactUploader: upload artifact named by matrix.platform
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/nuitka-standalone-zip.yml (1)
66-66: 💤 Low valueConsider pinning actions to commit SHAs for supply-chain security.
The workflow uses version tags (
@v4) for actions. While acceptable for official GitHub actions, pinning to commit SHAs prevents potential supply-chain attacks if tags are moved maliciously.Also applies to: 106-106, 122-122, 168-168
🤖 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/nuitka-standalone-zip.yml at line 66, Replace floating version tags for GitHub Actions with pinned commit SHAs to mitigate supply-chain risks: locate each "uses: actions/cache@v4" and the other "uses:" entries (e.g., the actions referenced at lines noted in the review) and update them to the corresponding full commit SHA for that action's repo (fetch the canonical commit from the action's GitHub releases/tags) so the workflow references a specific immutable commit; apply this change to all occurrences mentioned in the review (the actions at lines 66, 106, 122, 168) and verify the SHAs are correct and tested in CI.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/nuitka-standalone-zip.yml:
- Line 69: The cache key uses an invalid runner context property
runner.image_version which will be empty; update the cache key strings that
reference runner.image_version (e.g., the key values containing
ffmpeg-nuitka-${{ matrix.os }}-${{ runner.image_version }}-${{
steps.ffmpeg-version-linux.outputs.version }}-bundle and the similar occurrence
later) to remove the runner.image_version segment so they become
ffmpeg-nuitka-${{ matrix.os }}-${{ steps.ffmpeg-version-linux.outputs.version
}}-bundle; keep matrix.os and steps.ffmpeg-version-linux.outputs.version for
sufficient granularity.
---
Nitpick comments:
In @.github/workflows/nuitka-standalone-zip.yml:
- Line 66: Replace floating version tags for GitHub Actions with pinned commit
SHAs to mitigate supply-chain risks: locate each "uses: actions/cache@v4" and
the other "uses:" entries (e.g., the actions referenced at lines noted in the
review) and update them to the corresponding full commit SHA for that action's
repo (fetch the canonical commit from the action's GitHub releases/tags) so the
workflow references a specific immutable commit; apply this change to all
occurrences mentioned in the review (the actions at lines 66, 106, 122, 168) and
verify the SHAs are correct and tested in CI.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 827f18f4-48a5-477f-b902-d7df83c16a2d
📒 Files selected for processing (2)
.github/workflows/nuitka-standalone-zip.ymlscripts/build_nuitka.ps1
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
62-72:⚠️ Potential issue | 🟡 MinorCI dependency manifest/cache sync for
zip
.github/workflows/build.ymlinstallszipon Ubuntu, but the repo doesn’t contain a trackedapt-packages.txt; meanwhile.github/workflows/ci.ymlbuilds apt cache keys fromhashFiles('**/apt-packages.txt'), so the cache key won’t change whenzipis added.- Add/update the in-repo
apt-packages.txtto includezip(matching the apt install list), or adjust the cache-key input to the actual manifest file(s) used.🤖 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.yml around lines 62 - 72, The CI installs zip in .github/workflows/build.yml but the cache key in .github/workflows/ci.yml is built from hashFiles('**/apt-packages.txt'), so add "zip" to the repository's apt-packages.txt (or update the cache-key input in ci.yml to point to the actual manifest file you modified) to keep the apt cache key in sync; update the apt-packages.txt entry to match the package name used in the build step and commit both changes so cache invalidation works as expected.Source: Learnings
🧹 Nitpick comments (2)
.github/workflows/build.yml (1)
130-134: ⚡ Quick winHarden script input handling in the smoke-test step.
Injecting
${{ steps.package.outputs.package_path }}directly into executable PowerShell text is avoidable risk; pass it viaenvand read$env:PACKAGE_PATHinstead.Proposed patch
- name: Smoke test bundled FFmpeg + env: + PACKAGE_PATH: ${{ steps.package.outputs.package_path }} shell: pwsh run: | - $zipPath = "${{ steps.package.outputs.package_path }}" + $zipPath = $env:PACKAGE_PATH $extractRoot = Join-Path $env:RUNNER_TEMP "renderkit-pyinstaller-smoke"🤖 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.yml around lines 130 - 134, The smoke-test step named "Smoke test bundled FFmpeg" currently injects ${{ steps.package.outputs.package_path }} directly into the PowerShell run block; instead, add an env entry (e.g., PACKAGE_PATH: ${{ steps.package.outputs.package_path }}) on that job/step and update the run script to read the value from $env:PACKAGE_PATH (avoid string interpolation of GitHub expressions inside the script). Keep shell: pwsh and ensure any other references in the run block (like $zipPath) are assigned from $env:PACKAGE_PATH to harden input handling.Source: Linters/SAST tools
scripts/stage_portable_ffmpeg.py (1)
35-51: ⚡ Quick winAdd explicit timeouts to FFmpeg probe subprocesses.
Both probes can block indefinitely and stall CI jobs if the binary hangs.
Proposed patch
REQUIRED_ENCODERS = ("libx264", "libx265", "libaom-av1") +FFMPEG_PROBE_TIMEOUT_SECONDS = 30 @@ version_result = subprocess.run( [str(path), "-version"], check=True, capture_output=True, text=True, + timeout=FFMPEG_PROBE_TIMEOUT_SECONDS, ) @@ encoders_result = subprocess.run( [str(path), "-hide_banner", "-encoders"], check=True, capture_output=True, text=True, + timeout=FFMPEG_PROBE_TIMEOUT_SECONDS, )🤖 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 `@scripts/stage_portable_ffmpeg.py` around lines 35 - 51, The two subprocess.run calls that populate version_result and encoders_result can hang indefinitely; add a reasonable timeout argument (e.g., timeout=10) to both subprocess.run invocations and handle subprocess.TimeoutExpired around the calls (catching the exception, logging an informative message and failing gracefully or returning a fallback like "ffmpeg version unknown" for version_result and an empty encoder list for encoders_result). Update the calls that reference version_result and encoders_result so they behave correctly when a timeout occurs and ensure subprocess.run still uses check=True, capture_output=True, and text=True.
🤖 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.
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 62-72: The CI installs zip in .github/workflows/build.yml but the
cache key in .github/workflows/ci.yml is built from
hashFiles('**/apt-packages.txt'), so add "zip" to the repository's
apt-packages.txt (or update the cache-key input in ci.yml to point to the actual
manifest file you modified) to keep the apt cache key in sync; update the
apt-packages.txt entry to match the package name used in the build step and
commit both changes so cache invalidation works as expected.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 130-134: The smoke-test step named "Smoke test bundled FFmpeg"
currently injects ${{ steps.package.outputs.package_path }} directly into the
PowerShell run block; instead, add an env entry (e.g., PACKAGE_PATH: ${{
steps.package.outputs.package_path }}) on that job/step and update the run
script to read the value from $env:PACKAGE_PATH (avoid string interpolation of
GitHub expressions inside the script). Keep shell: pwsh and ensure any other
references in the run block (like $zipPath) are assigned from $env:PACKAGE_PATH
to harden input handling.
In `@scripts/stage_portable_ffmpeg.py`:
- Around line 35-51: The two subprocess.run calls that populate version_result
and encoders_result can hang indefinitely; add a reasonable timeout argument
(e.g., timeout=10) to both subprocess.run invocations and handle
subprocess.TimeoutExpired around the calls (catching the exception, logging an
informative message and failing gracefully or returning a fallback like "ffmpeg
version unknown" for version_result and an empty encoder list for
encoders_result). Update the calls that reference version_result and
encoders_result so they behave correctly when a timeout occurs and ensure
subprocess.run still uses check=True, capture_output=True, and text=True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d880603f-c700-43f4-829f-4655486b8dfc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/build.yml.github/workflows/nuitka-standalone-zip.ymldocs/development.mdpyproject.tomlscripts/stage_portable_ffmpeg.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nuitka-standalone-zip.yml
|
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
mainanddevelop.Validation
git diff --checkpassed.Summary by CodeRabbit
New Features
Chores
Documentation