-
-
Notifications
You must be signed in to change notification settings - Fork 17
Rework FFmpeg installation in unit test workflow #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds a local GitHub Action to install FFmpeg/FFprobe cross‑platform, updates the unit-test workflow to use it and bump FFmpeg to 8.0.1, updates the Dockerfile to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as GitHub Runner
participant Action as setup-ffmpeg Action
participant DockerHub as Docker Hub (mwader/static-ffmpeg)
participant HTTP as External hosts (osxexperts / gyan.dev)
participant FS as Runner filesystem / PATH
Runner->>Action: invoke with ffmpeg-version
Action->>Runner: detect OS
alt Linux
Action->>DockerHub: pull mwader/static-ffmpeg:8.0.1
DockerHub-->>Action: container with /ffmpeg & /ffprobe
Action->>FS: copy binaries to $HOME/ffmpeg, set exec
else macOS
Action->>HTTP: download macOS build zips (trimmed version)
HTTP-->>Action: deliver zips
Action->>FS: unzip, move to $HOME/ffmpeg, set exec
else Windows
Action->>HTTP: download Windows zip (gyan.dev)
HTTP-->>Action: deliver zip
Action->>FS: unzip, move executables to $HOME/ffmpeg
end
Action->>Runner: add $HOME/ffmpeg to PATH
Action->>FS: run `ffmpeg -version` and `ffprobe -version` for verification
FS-->>Action: output
Action-->>Runner: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/actions/setup-ffmpeg/action.yml (4)
19-25: Consider cleaning up Docker image after extraction.The Docker image pulled on line 20 remains on the runner after the container is removed. Whilst GitHub-hosted runners are ephemeral, cleaning up the image could reduce disk usage during the workflow run.
🔎 Optional cleanup suggestion
docker cp "$CID:/ffmpeg" "$INSTALL_DIR/ffmpeg" docker cp "$CID:/ffprobe" "$INSTALL_DIR/ffprobe" docker rm "$CID" + docker rmi "mwader/static-ffmpeg:$FF_VERSION" chmod +x "$INSTALL_DIR/"*
28-33: Clean up temporary zip files.The downloaded zip files (ffmpeg.zip, ffprobe.zip) are not removed after extraction, leaving unnecessary artefacts in the workspace.
🔎 Add cleanup
curl -L "https://evermeet.cx/ffmpeg/ffmpeg-$FF_VERSION.zip" -o ffmpeg.zip unzip -q ffmpeg.zip + rm ffmpeg.zip mv ffmpeg "$INSTALL_DIR/ffmpeg" curl -L "https://evermeet.cx/ffmpeg/ffprobe-$FF_VERSION.zip" -o ffprobe.zip unzip -q ffprobe.zip + rm ffprobe.zip mv ffprobe "$INSTALL_DIR/ffprobe"
37-40: Clean up temporary files after extraction.The downloaded zip file and extracted directory structure are not removed after moving the binaries.
🔎 Add cleanup
curl -L "https://www.gyan.dev/ffmpeg/builds/packages/ffmpeg-$FF_VERSION-essentials_build.zip" -o ffmpeg.zip unzip -q ffmpeg.zip find . -type f \( -name ffmpeg.exe -o -name ffprobe.exe \) -exec mv {} "$INSTALL_DIR/" \; + rm -rf ffmpeg.zip ffmpeg-*-essentials_build/
44-48: Consider verifying the installed version matches the requested version.The current verification confirms the binaries are executable but doesn't validate that the installed version matches
$FF_VERSION. This could help catch version mismatch issues.🔎 Enhanced verification
- name: Verify FFmpeg and FFprobe installations shell: bash run: | - ffmpeg -version - ffprobe -version + echo "Verifying FFmpeg version ${{ inputs.ffmpeg-version }}..." + ffmpeg -version | head -1 + ffprobe -version | head -1 + + # Optional: strict version check + INSTALLED_VERSION=$(ffmpeg -version | head -1 | grep -oP 'version \K[0-9.]+') + if [[ "$INSTALLED_VERSION" != "${{ inputs.ffmpeg-version }}" ]]; then + echo "Warning: Expected version ${{ inputs.ffmpeg-version }}, got $INSTALLED_VERSION" + fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/actions/setup-ffmpeg/action.yml.github/workflows/unit-test.ymlDockerfileLICENSE
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-22T20:09:04.299Z
Learnt from: rob93c
Repo: Stickerifier/Stickerify PR: 318
File: .github/workflows/unit-test.yml:27-29
Timestamp: 2024-12-22T20:09:04.299Z
Learning: The user tested AnimMouse’s action and deemed it reliable for their environment, so there’s no need to revert to FedericoCarboni/setup-ffmpeg.
Applied to files:
.github/workflows/unit-test.yml
⏰ 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). (4)
- GitHub Check: Execute unit tests (macos-15-intel)
- GitHub Check: Execute unit tests (windows-latest)
- GitHub Check: Build and upload Docker image
- GitHub Check: Execute unit tests (ubuntu-latest)
🔇 Additional comments (4)
LICENSE (1)
3-3: Copyright year extended to 2026.The copyright year range now includes 2026. Whilst slightly forward-looking, this is acceptable given the current date (December 2025).
Dockerfile (1)
5-6: FFmpeg version updated consistently.The Docker image version aligns with the workflow changes. The bump comment ensures automated version tracking.
.github/actions/setup-ffmpeg/action.yml (1)
42-42: PATH update implemented correctly.The installation directory is properly added to the GitHub Actions PATH using the standard approach.
.github/workflows/unit-test.yml (1)
26-30: Confirmation of cross-platform compatibility already completed.The custom local action has been tested and works reliably across your environment, as noted in your previous verification. FFmpeg 8.0.1 binaries are confirmed available from all three platform sources: the mwader/static-ffmpeg Docker image for Linux, evermeet.cx for macOS, and gyan.dev for Windows. No further verification script is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/actions/setup-ffmpeg/action.yml (2)
37-41: Thefindcommand may behave unexpectedly with nested directories.Line 40 searches the entire current directory tree for
ffmpeg.exeandffprobe.exe. If the gyan.dev archive structure changes or contains multiple matches, this could move unintended files or fail silently.🔎 More robust approach
Use a more predictable path based on gyan.dev's archive structure:
elif [[ "${{ runner.os }}" == "Windows" ]]; then curl -L "https://www.gyan.dev/ffmpeg/builds/packages/ffmpeg-$FF_VERSION-essentials_build.zip" -o ffmpeg.zip unzip -q ffmpeg.zip - find . -type f \( -name ffmpeg.exe -o -name ffprobe.exe \) -exec mv {} "$INSTALL_DIR/" \; + EXTRACT_DIR=$(find . -maxdepth 1 -name "ffmpeg-*" -type d | head -n1) + if [[ -z "$EXTRACT_DIR" ]]; then + echo "Error: Could not locate extracted FFmpeg directory" + exit 1 + fi + mv "$EXTRACT_DIR/bin/ffmpeg.exe" "$INSTALL_DIR/" + mv "$EXTRACT_DIR/bin/ffprobe.exe" "$INSTALL_DIR/"
45-49: Good verification step, but consider cleanup.The verification step correctly confirms installation success. However, temporary files (
ffmpeg.zip, extracted directories) are not cleaned up, which leaves artifacts in the runner workspace.🔎 Add cleanup step
run: | ffmpeg -version ffprobe -version + + - name: Clean up temporary files + shell: bash + run: | + rm -f ffmpeg.zip ffprobe.zip + find . -maxdepth 1 -name "ffmpeg-*" -type d -exec rm -rf {} +
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/setup-ffmpeg/action.yml.github/workflows/unit-test.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit-test.yml
⏰ 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). (2)
- GitHub Check: Execute unit tests (windows-latest)
- GitHub Check: Execute unit tests (ubuntu-latest)
🔇 Additional comments (6)
.github/actions/setup-ffmpeg/action.yml (6)
1-6: LGTM!The action metadata and input definition are clear and appropriate.
7-9: LGTM!Composite action type is appropriate for this multi-step installation workflow.
10-18: LGTM!Appropriate use of strict error handling and clear variable definitions.
21-27: LGTM!The Docker-based approach for Linux provides consistent, statically-linked binaries and includes proper container cleanup.
43-43: LGTM!Correctly adds the installation directory to the GitHub Actions PATH.
30-30: Correct the version transformation endpoint reference.The review comment references evermeet.cx for verification, but the code uses osxexperts.net for macOS ARM builds. Additionally, the fragility concern about the awk command is overstated—the transformation handles the version format used in the workflow (8.0.1 → 80) without issues, and the code runs successfully in CI on macOS-latest. The awk command will not fail silently; it will simply process whatever fields are available.
MartelliEnrico
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
413f9b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/actions/setup-ffmpeg/action.yml (2)
29-38: Security concern: GPG signature verification remains unimplemented.The binaries downloaded from osxexperts.net (formerly evermeet.cx) are not being verified with GPG signatures. This security concern was previously raised and remains unaddressed.
40-45: Security concern: Checksum verification remains unimplemented.The Windows installation downloads binaries from gyan.dev without verifying checksums. This security concern was previously raised and remains unaddressed.
🧹 Nitpick comments (1)
.github/actions/setup-ffmpeg/action.yml (1)
21-27: Consider cleaning up the Docker image after use.The Linux installation works correctly, but the pulled Docker image remains in the system after the container is removed. In CI environments, this typically doesn't cause issues, but for long-running or frequently used runners, it could accumulate images.
🔎 Optional improvement: Add image cleanup
docker cp "$CID:/ffmpeg" "$INSTALL_DIR/ffmpeg" docker cp "$CID:/ffprobe" "$INSTALL_DIR/ffprobe" docker rm "$CID" + docker rmi "mwader/static-ffmpeg:$FF_VERSION" chmod +x "$INSTALL_DIR/"*
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/setup-ffmpeg/action.yml
⏰ 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). (3)
- GitHub Check: Execute unit tests (windows-latest)
- GitHub Check: Execute unit tests (ubuntu-latest)
- GitHub Check: Execute unit tests (macos-latest)
🔇 Additional comments (5)
.github/actions/setup-ffmpeg/action.yml (5)
1-6: LGTM!The action metadata and input definition are clear and appropriately structured. The required
ffmpeg-versioninput is properly declared.
7-9: LGTM!The composite action configuration is correctly structured.
10-20: LGTM!Excellent setup with strict error handling (
set -euo pipefail) and proper variable initialization. The installation directory is created safely with-p.
47-47: LGTM!The installation directory is correctly added to the GitHub Actions PATH with proper variable quoting.
49-53: LGTM!Excellent practice to verify both installations by running version checks. This ensures the binaries are correctly installed and accessible.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.