Feat/add deb for rust - #16
Conversation
WalkthroughAdds CI workflows for multi-arch builds and releases, Debian packaging and service/unit files, packaging helper scripts (local and Docker), Dockerfile and .dockerignore, build script updates, docs for Debian install, and VCS ignore updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (push/tag)
participant GH as GitHub
participant Runner as Actions Runner
participant DotNet as .NET build
participant Rust as Rust build (amd64/arm64)
participant Artifact as Artifacts Storage
participant Packager as Packaging job (tar + deb)
participant Release as GitHub Release API
Dev->>GH: push to branch / create tag
GH->>Runner: trigger on-push / on-tag workflow
Runner->>DotNet: Build Blazor (LibMan, dotnet publish)
DotNet-->>Artifact: upload webdeploy artifact
Runner->>Rust: Build Rust binary (matrix targets)
Rust-->>Artifact: upload per-arch binary artifacts
Runner->>Packager: download artifacts, validate, create tar + .deb
Packager-->>Artifact: upload tarballs & debs
GH->>Runner: on-tag workflow uses latest main_rs run id
Runner->>Artifact: download filepi artifacts, extract & rename for tag
Runner->>Release: create Release and attach prepared assets
Release-->>Dev: Release published with assets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.sh (1)
172-191: Debian build prereq check is inconsistent with build-deb.sh expectationsHere you allow either
./filepiortarget/release/filepi:if [ ! -f "filepi" ] && [ ! -f "target/release/filepi" ]; thenbut
build-deb.shonly looks forfilepiin the current directory and errors out otherwise. That can produce a confusing failure path if onlytarget/release/filepiexists.To keep behavior consistent and user-friendly, auto-copy from
target/releasewhen needed and enforce a single canonical location:- # Check prerequisites - if [ ! -f "filepi" ] && [ ! -f "target/release/filepi" ]; then - print_error "filepi binary not found. Run with --type rust first." - return 1 - fi + # Ensure filepi binary exists in project root (what build-deb.sh expects) + if [ ! -f "filepi" ]; then + if [ -f "target/release/filepi" ]; then + cp "target/release/filepi" ./filepi + else + print_error "filepi binary not found. Run ./build.sh --type rust --mode release first." + return 1 + fi + fi
🧹 Nitpick comments (9)
Dockerfile (1)
14-14: Consider making the UID configurable.The hardcoded UID 1000 may not match the host user on all systems, potentially causing permission issues with mounted volumes.
Consider using a build argument to allow flexibility:
+ARG USER_UID=1000 + # Create a non-root user matching the host UID (1000) -RUN useradd -m -u 1000 builder +RUN useradd -m -u ${USER_UID} builderThen build with:
docker build --build-arg USER_UID=$(id -u) -t filepi-builder .build.sh (2)
149-168: Align cp source with actual binary name and remove redundant cpThe release/debug branches now copy
target/*/filepiinto./filepi, but each uses a duplicatedcp ... || cp ... || truechain with the same source/target and silently ignores failures. If the cargo binary name is stillfilepi-rust, this will “succeed” while leaving no./filepi. Suggest simplifying and surfacing problems:- # Copy binary to root for easier access - cp target/release/filepi ./filepi || cp target/release/filepi ./filepi 2>/dev/null || true + # Copy binary to root for easier access + cp "target/release/filepi" ./filepi 2>/dev/null \ + || print_warning "Could not copy target/release/filepi to ./filepi (check binary name/target)." ... - # Copy binary to root for easier access - cp target/debug/filepi ./filepi || cp target/debug/filepi ./filepi 2>/dev/null || true + # Copy binary to root for easier access + cp "target/debug/filepi" ./filepi 2>/dev/null \ + || print_warning "Could not copy target/debug/filepi to ./filepi (check binary name/target)."Also please double‑check that cargo actually produces
filepi(notfilepi-rust) so the copy works as expected.
29-73: Help text says default build mode is release, but script sets debug
BUILD_MODEis initialized to"debug"while the help text advertises--mode [debug|release] (default: release). Either change the default to release or adjust the help text so users aren’t surprised by slower debug builds.-BUILD_MODE="debug" +BUILD_MODE="release"(or update the help string instead, if debug is intentional.)
debian/postinst (1)
4-32: Guard systemd calls in postinst for non-systemd environmentsUnder
set -e,systemctl daemon-reloadandsystemctl enable filepi.servicewill fail installations on systems without systemd (or in chroots/containers) even though the package contents are otherwise fine. Consider wrapping these in checks:if command -v systemctl >/dev/null 2>&1 && [ -d /run/systemd/system ]; then systemctl daemon-reload || true systemctl enable filepi.service || true systemctl start filepi.service || true fiThis keeps behavior correct on systemd hosts without breaking installs in lighter environments.
build-in-docker.sh (1)
4-9: Remove unused YELLOW color constant (lint cleanup)Shellcheck flags
YELLOWas unused here. Either remove it or start using it for a warning helper to keep the script clean and quiet under lint:-YELLOW='\033[1;33m'(or add a
print_warningthat uses it, similar to build.sh)..github/workflows/on-push-build-all.yml (1)
40-67: Tighten quoting when writing to$GITHUB_OUTPUT(Shellcheck SC2086/SC2129)Shellcheck (via actionlint) points out unquoted
$GITHUB_OUTPUTand repeated redirects. Safer, lint-clean pattern:- echo "build-name=$VERSION" >> $GITHUB_OUTPUT - echo "version=$VERSION" >> $GITHUB_OUTPUT - echo "deb-version=$DEB_VERSION" >> $GITHUB_OUTPUT + { + echo "build-name=$VERSION" + echo "version=$VERSION" + echo "deb-version=$DEB_VERSION" + } >> "$GITHUB_OUTPUT"Prevents issues if the path ever contains spaces and resolves SC2086/SC2129.
build-deb.sh (1)
61-102: Limit whichdebian/*files are copied into DEBIAN for a cleaner package layout
cp -r debian/* "${STAGING_DIR}/DEBIAN/"pulls in everything underdebian/(rules, changelog, compat, etc.) into the control area, then you delete onlyfilepi.service. While dpkg-deb will mostly ignore the extra files, it’s unconventional and makes the package noisier than necessary.Consider copying only the control-related files:
-if [ -d "debian" ]; then - cp -r debian/* "${STAGING_DIR}/DEBIAN/" +if [ -d "debian" ]; then + for f in control postinst postrm preinst prerm; do + [ -f "debian/$f" ] && cp "debian/$f" "${STAGING_DIR}/DEBIAN/" + doneYou can then drop the
rm -f "${STAGING_DIR}/DEBIAN/filepi.service"line. This keeps the generated .deb closer to Debian norms while preserving your existing control-file processing..github/workflows/on-tag-push-create-release.yml (2)
129-177: Simplify filename substitution using bash parameter expansion.Lines 158 and 169 use
sedfor filename substitution, which is flagged by shellcheck as an opportunity to use bash built-in parameter expansion for better portability and performance. Additionally, the script lacks error handling if no.tar.gzor.debfiles are found in the artifacts directory.Apply this diff to use bash parameter expansion instead of
sed:# Rename artifacts to include tag version instead of build number echo "Preparing release assets..." # Process binary tarballs for tarball in ${{ env.ARTIFACTS_DIR }}/*.tar.gz; do if [ -f "$tarball" ]; then filename=$(basename "$tarball") - # Replace the build name with tag name in filename - new_filename=$(echo "$filename" | sed "s/main_rs_[0-9]\+/${TAG_NAME}/g") + # Replace the build name with tag name in filename + new_filename="${filename//main_rs_[0-9]*/}" + new_filename="${new_filename}${TAG_NAME}${filename##*main_rs_[0-9]*}"Alternative (simpler):
- new_filename=$(echo "$filename" | sed "s/main_rs_[0-9]\+/${TAG_NAME}/g") + new_filename="${filename//main_rs_+([0-9])/${TAG_NAME}}"Note: The second approach assumes
shopt -s extglobis enabled or usesedif portability to shells without extglob is required.
295-322: Refactor release summary script to fix quoting and redirect patterns.Shellcheck flags multiple issues in the Release Summary step:
- SC2012: Using
lsin a context better served byfind- SC2086: Unquoted variables that could cause word splitting
- SC2129: Multiple
>>redirects that could be combinedApply this diff to improve robustness and follow shell best practices:
- name: Release Summary run: | - echo "## 🎉 FilePi Server Release Created Successfully!" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "**🏷️ Tag:** ${{ needs.common-vars.outputs.tag-name }}" >> $GITHUB_STEP_SUMMARY - echo "**📦 Release:** ${{ needs.common-vars.outputs.release-name }}" >> $GITHUB_STEP_SUMMARY - echo "**🔖 Type:** ${{ needs.common-vars.outputs.is-prerelease == 'true' && 'Pre-release' || 'Stable Release' }}" >> $GITHUB_STEP_SUMMARY - echo "**🔨 Source Build:** Run #${{ needs.common-vars.outputs.latest-run-id }} from main_rs" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - echo "### 📦 Release Assets:" >> $GITHUB_STEP_SUMMARY - for file in ${{ env.RELEASE_DIR }}/*; do + { + echo "## 🎉 FilePi Server Release Created Successfully!" + echo "" + echo "**🏷️ Tag:** ${{ needs.common-vars.outputs.tag-name }}" + echo "**📦 Release:** ${{ needs.common-vars.outputs.release-name }}" + echo "**🔖 Type:** ${{ needs.common-vars.outputs.is-prerelease == 'true' && 'Pre-release' || 'Stable Release' }}" + echo "**🔨 Source Build:** Run #${{ needs.common-vars.outputs.latest-run-id }} from main_rs" + echo "" + echo "### 📦 Release Assets:" + find "${{ env.RELEASE_DIR }}" -maxdepth 1 -type f ! -name "RELEASE_NOTES.md" | while read -r file; do - if [ -f "$file" ] && [[ "$file" != *"RELEASE_NOTES.md" ]]; then - filename=$(basename "$file") - size=$(ls -lh "$file" | awk '{print $5}') - if [[ "$filename" == *.tar.gz ]]; then - echo "- 🗃️ $filename ($size) - Binary with Web UI" >> $GITHUB_STEP_SUMMARY - elif [[ "$filename" == *.deb ]]; then - echo "- 📦 $filename ($size) - Debian Package" >> $GITHUB_STEP_SUMMARY + filename=$(basename "$file") + size=$(stat -f%z "$file" 2>/dev/null || stat -c%s "$file" 2>/dev/null) # Works on macOS and Linux + size=$(numfmt --to=iec-i --suffix=B "$size" 2>/dev/null || echo "$size B") # Format bytes + if [[ "$filename" == *.tar.gz ]]; then + echo "- 🗃️ $filename ($size) - Binary with Web UI" + elif [[ "$filename" == *.deb ]]; then + echo "- 📦 $filename ($size) - Debian Package" fi - fi + done - done - echo "" >> $GITHUB_STEP_SUMMARY - echo "### 🌟 Key Features:" >> $GITHUB_STEP_SUMMARY - echo "- 🌐 Modern Blazor WebAssembly frontend" >> $GITHUB_STEP_SUMMARY - echo "- 📱 Responsive design for all devices" >> $GITHUB_STEP_SUMMARY - echo "- 🎬 Video streaming with thumbnails" >> $GITHUB_STEP_SUMMARY - echo "- 📁 Complete file management" >> $GITHUB_STEP_SUMMARY - echo "- ⚡ Optimized for Raspberry Pi" >> $GITHUB_STEP_SUMMARY + echo "" + echo "### 🌟 Key Features:" + echo "- 🌐 Modern Blazor WebAssembly frontend" + echo "- 📱 Responsive design for all devices" + echo "- 🎬 Video streaming with thumbnails" + echo "- 📁 Complete file management" + echo "- ⚡ Optimized for Raspberry Pi" + } >> "$GITHUB_STEP_SUMMARY"Note: The script continues to run on
ubuntu-latest, sostatis available. However, the refactored version is slightly more complex; you may prefer to keep the simpler original if the quoting warnings don't impact functionality.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.dockerignore(1 hunks).github/workflows/on-push-build-all.yml(1 hunks).github/workflows/on-tag-push-create-release.yml(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)build-deb.sh(1 hunks)build-in-docker.sh(1 hunks)build.sh(4 hunks)debian/changelog(1 hunks)debian/compat(1 hunks)debian/control(1 hunks)debian/filepi.service(1 hunks)debian/postinst(1 hunks)debian/postrm(1 hunks)debian/rules(1 hunks)docs/DEBIAN-INSTALL.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
build.sh (1)
build-in-docker.sh (2)
print_success(15-17)print_step(11-13)
build-in-docker.sh (1)
build.sh (3)
print_error(24-26)print_step(12-14)print_success(16-18)
🪛 actionlint (1.7.9)
.github/workflows/on-push-build-all.yml
42-42: shellcheck reported issue in this script: SC2086:info:18:31: Double quote to prevent globbing and word splitting
(shellcheck)
42-42: shellcheck reported issue in this script: SC2086:info:19:28: Double quote to prevent globbing and word splitting
(shellcheck)
42-42: shellcheck reported issue in this script: SC2086:info:20:36: Double quote to prevent globbing and word splitting
(shellcheck)
42-42: shellcheck reported issue in this script: SC2129:style:18:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
158-158: shellcheck reported issue in this script: SC2012:info:18:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
158-158: shellcheck reported issue in this script: SC2012:info:9:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:13:12: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:14:34: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:19:35: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:3:54: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:4:12: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:5:61: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
.github/workflows/on-tag-push-create-release.yml
130-130: shellcheck reported issue in this script: SC2001:style:28:20: See if you can use ${variable//search/replace} instead
(shellcheck)
130-130: shellcheck reported issue in this script: SC2001:style:39:20: See if you can use ${variable//search/replace} instead
(shellcheck)
296-296: shellcheck reported issue in this script: SC2012:info:12:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:14:61: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:1:60: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:20:12: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:21:31: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:22:50: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:23:49: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:24:47: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:25:40: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:26:42: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:3:65: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:4:72: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:5:117: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:6:96: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:7:12: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2086:info:8:33: Double quote to prevent globbing and word splitting
(shellcheck)
296-296: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
296-296: shellcheck reported issue in this script: SC2129:style:20:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🪛 Shellcheck (0.11.0)
build-in-docker.sh
[warning] 7-7: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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: build (arm64)
- GitHub Check: build (amd64)
🔇 Additional comments (8)
debian/changelog (1)
5-5: Verify the changelog date.The timestamp shows a future date (March 9, 2025). Debian changelog entries typically use the actual date of the release or package creation.
Please confirm whether this is:
- A placeholder that should be updated to the current date
- An intentional future date for testing purposes
.gitignore (1)
27-31: LGTM!The new ignore patterns appropriately exclude build artifacts, packaging outputs, and executable binaries generated by the build and packaging workflows.
README.md (1)
12-40: LGTM!The prerequisite and build documentation is clear and accurate. The platform-specific FFmpeg installation commands are correct for Debian/Ubuntu and macOS systems.
.dockerignore (1)
1-26: LGTM!The ignore patterns comprehensively cover build artifacts, logs, version control, and development environment files, ensuring clean Docker image builds.
Dockerfile (1)
20-20: Build script verification confirms no issues.The
build-deb.shscript exists at the repository root with proper executable permissions (rwxr-xr-x). The Dockerfile'sCMD ["./build-deb.sh"]instruction is correctly configured and will execute as expected.build.sh (1)
216-227: Generated-files summary omits error handling but looks consistent with new pathsThe final summary now checks for
target/release/filepiandtarget/debug/filepi, matching the earlier copy commands. Once thebuild_rustcp logic is corrected, this section is fine and will accurately reflect built binaries..github/workflows/on-push-build-all.yml (2)
157-179: Summary step is fine; ls-based size lookup is acceptable hereThe artifact listing uses
ls -lh "$file" | awk '{print $5}'and simplefor file in all-artifacts/*.tar.gzloops. Shellcheck prefersfind/stat, but for CI summary output this is adequate and easy to read. No changes needed unless you want stricter lint cleanliness.
35-43: The .NET 10.0.x version is valid and currently available..NET 10.0.0 (LTS) was released on November 11, 2025, with SDK 10.0.100 available. The version string
'10.0.x'is a supported syntax foractions/setup-dotnet@v5(documented asA.B.xformat to resolve to the latest patch on that channel). The workflow will not fail due to this version specification.
| @@ -0,0 +1 @@ | |||
| Build-Depends: debhelper-compat (>= 10) No newline at end of file | |||
There was a problem hiding this comment.
Incorrect file format - this will break package builds.
The debian/compat file should contain only a single integer representing the debhelper compatibility level (e.g., 10 or 12), not a Build-Depends declaration. The Build-Depends: debhelper-compat (= 12) declaration belongs in debian/control (which already has it according to the PR changes).
Modern Debian packaging typically uses debhelper-compat in the Build-Depends field of debian/control and doesn't require a separate debian/compat file. You can either:
Option 1 (Recommended): Remove this file entirely since debian/control already declares debhelper-compat (= 12).
Option 2: Replace the content with a single integer:
-Build-Depends: debhelper-compat (>= 10)
+12📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Build-Depends: debhelper-compat (>= 10) | |
| 12 |
🤖 Prompt for AI Agents
In debian/compat around lines 1-1, the file incorrectly contains a Build-Depends
declaration instead of a single debhelper compatibility integer; remove this
file entirely (preferred) since debian/control already declares debhelper-compat
(= 12), or replace its contents with the single integer "12" (no other text) if
you must keep the file.
| [Service] | ||
| Type=simple | ||
| User=root | ||
| WorkingDirectory=/opt/filepi | ||
| ExecStart=/opt/filepi/filepi | ||
| Restart=always | ||
| RestartSec=10 | ||
| Environment=FILE_PI_PORT=8080 | ||
| Environment=FILE_PI_ROOT_DIR=/var/lib/filepi/media |
There was a problem hiding this comment.
Service runs as root; consider dedicated system user for security
User=root means the HTTP server runs with full privileges. Safer pattern is to create a dedicated filepi system user (in postinst), chown /opt/filepi and /var/lib/filepi, and run the unit as that user. You may also want Restart=on-failure instead of always so manual stops don’t get auto‑restarted.
🤖 Prompt for AI Agents
In debian/filepi.service around lines 5 to 13 the unit runs the service as root
and always restarts; create and use a dedicated system user (e.g. filepi)
instead of root: update packaging/postinst to create a system user/group (system
account, no login), chown /opt/filepi and /var/lib/filepi to that user and
ensure WorkingDirectory and ExecStart paths are readable/executable by it, then
change User=root to User=filepi in the unit file; also change Restart=always to
Restart=on-failure so manual stops are respected (or document why always is
desired) and ensure the postinst enables/reloads the systemd unit after
installation.
191e1a2 to
1428897
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/on-push-build-all.yml(1 hunks).github/workflows/on-tag-push-create-release.yml(1 hunks)
⏰ 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: Build Rust (arm64)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (8)
.github/workflows/on-push-build-all.yml (5)
14-50: Version and artifact naming logic is sound.The setup job correctly derives build-name, version, and deb-version from branch/run context. Branch name extraction handles both PR and push events, and the Debian version format complies with conventions.
51-77: Blazor build and artifact upload is standard.The job correctly sets up .NET, installs dependencies, builds the frontend via build.sh, and uploads the webdeploy directory with appropriate retention.
79-127: Rust build with cross-compilation matrix is correct.The job properly sets up per-architecture toolchains, conditionally installs ARM linker dependencies, and builds the binary with the correct name. The past critical issue referencing a non-existent binary name has been fixed.
208-240: Build summary job is well-structured.The job correctly downloads all per-arch artifacts and generates a formatted summary with file sizes and types. Defensive file existence checks prevent errors if artifacts are missing.
184-194: Verify dpkg-deb availability or add explicit error checking to build-deb.sh.The
build-deb.shscript is present and correctly outputs tooutputs/filepi_*.deb. However, line 149-150 ofbuild-deb.shsilently skips package creation ifdpkg-debis unavailable, only printing a warning. This causes line 191'smvcommand to fail (not silently, but loudly) when no file matches the glob. Either ensure the build environment hasdpkg-debavailable, or add explicit error handling inbuild-deb.shto exit with a failure if the.debfile is not created..github/workflows/on-tag-push-create-release.yml (3)
14-73: Tag parsing and latest build discovery is sound.The job correctly extracts tag information, determines prerelease status based on semantic versioning conventions, and queries for the latest successful build on main_rs with appropriate error handling.
129-176: Asset preparation with tag-based renaming is correct.The sed pattern correctly matches and replaces the build number with the tag name in both tarball and Debian package filenames, ensuring released artifacts use the tag version rather than build numbers.
289-321: Cleanup and release summary steps are well-structured.The job appropriately removes temporary artifacts and generates a comprehensive summary with release metadata and asset details.
1428897 to
5e76d37
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
build-deb.sh (2)
84-98: Control file sed operations could be combined for efficiency (optional refactor).The current approach runs four separate sed operations on the control file, each writing to a temp file and moving it back. While this works correctly and is readable, it could be optimized into a single sed pass or combined regex for better efficiency:
# Current: 4 passes sed 's/${shlibs:Depends}, //g' "$CONTROL_FILE" > "$CONTROL_TMP" && mv "$CONTROL_TMP" "$CONTROL_FILE" sed 's/${misc:Depends}, //g' "$CONTROL_FILE" > "$CONTROL_TMP" && mv "$CONTROL_TMP" "$CONTROL_FILE" # ... etc # Optional: Combined single pass (using ; or -e) sed -e 's/${shlibs:Depends}, //g' -e 's/${misc:Depends}, //g' -e 's/Depends: , /Depends: /g' "$CONTROL_FILE" > "$CONTROL_TMP" && mv "$CONTROL_TMP" "$CONTROL_FILE"This is a nice-to-have optimization; the current implementation is functionally correct.
76-79: Sed -i portability handled reasonably but could be clearer.The fallback pattern
sed -i.bak "..." || sed -i "" "..."works across Linux and macOS but relies on the first command failing. A more explicit approach (checkingunameor usinggsedon macOS if available) would reduce ambiguity. However, this is acceptable for a build script that runs in CI where the environment is controlled..github/workflows/on-push-build-all.yml (1)
25-49: Shell script quoting improvements for robustness.Lines 25–49 (and similar patterns in lines 220+) have several unquoted variable expansions flagged by actionlint (SC2086). While these work in CI, quoting
$GITHUB_OUTPUTand similar variables protects against word-splitting and globbing edge cases. Additionally, consider grouping multiple>>redirects into a single block (SC2129).Example improvement:
- echo "build-name=$VERSION" >> $GITHUB_OUTPUT - echo "version=$VERSION" >> $GITHUB_OUTPUT - echo "deb-version=$DEB_VERSION" >> $GITHUB_OUTPUT + { + echo "build-name=$VERSION" + echo "version=$VERSION" + echo "deb-version=$DEB_VERSION" + } >> "$GITHUB_OUTPUT"This is a style refinement; functionality is not impacted.
.github/workflows/on-tag-push-create-release.yml (1)
327-344: Shell script quoting improvements for robustness.Lines 327–344 (Release Summary loop) contain unquoted variable expansions flagged by actionlint (SC2086). While these work in CI, quoting
"$file"and"$filename"protects against edge cases:- for file in ${{ env.RELEASE_DIR }}/*; do - if [ -f "$file" ] && [[ "$file" != *"RELEASE_NOTES.md" ]]; then + for file in "${{ env.RELEASE_DIR }}"/*; do + if [ -f "$file" ] && [[ "$file" != *"RELEASE_NOTES.md" ]]; thenThis is a style refinement; functionality is not impacted.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/on-push-build-all.yml(1 hunks).github/workflows/on-tag-push-create-release.yml(1 hunks)build-deb.sh(1 hunks)build.ps1(1 hunks)build.sh(6 hunks)debian/control(1 hunks)debian/postinst(1 hunks)debian/postrm(1 hunks)debian/rules(1 hunks)docs/DEBIAN-INSTALL.md(1 hunks)frontend/FilePiWeb/Layout/EmptyLayout.razor(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- build.ps1
- frontend/FilePiWeb/Layout/EmptyLayout.razor
🚧 Files skipped from review as they are similar to previous changes (4)
- debian/postrm
- debian/rules
- debian/control
- debian/postinst
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/on-push-build-all.yml
25-25: shellcheck reported issue in this script: SC2086:info:18:31: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:19:28: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:20:36: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2129:style:18:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
220-220: shellcheck reported issue in this script: SC2012:info:18:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
220-220: shellcheck reported issue in this script: SC2012:info:9:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:13:12: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:14:34: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:19:35: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:3:54: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:4:12: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:5:61: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
.github/workflows/on-tag-push-create-release.yml
130-130: shellcheck reported issue in this script: SC2001:style:28:20: See if you can use ${variable//search/replace} instead
(shellcheck)
130-130: shellcheck reported issue in this script: SC2001:style:39:20: See if you can use ${variable//search/replace} instead
(shellcheck)
277-277: shellcheck reported issue in this script: SC2012:info:12:13: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
277-277: shellcheck reported issue in this script: SC2012:info:4:17: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
318-318: shellcheck reported issue in this script: SC2012:info:12:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:14:61: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:1:60: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:20:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:21:31: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:22:50: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:23:49: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:24:47: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:25:40: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:26:42: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:3:65: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:4:72: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:5:117: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:6:96: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:7:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:8:33: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
318-318: shellcheck reported issue in this script: SC2129:style:20:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (9)
build.sh (2)
155-155: Redundant binary copy commands—clarify intent or simplify.Lines 155 and 164 both repeat the same
cpcommand twice (with identical source and destination). This appears to be a copy-paste error or unclear fallback logic. Either clarify the intent or simplify to a single command:- cp target/release/filepi ./filepi || cp target/release/filepi ./filepi 2>/dev/null || true + cp target/release/filepi ./filepi || trueApply the same fix to line 164 for debug mode.
Also applies to: 164-164
176-177: Binary naming and build workflow updates look correct.The prerequisites check now correctly references
filepi(not the oldfilepi-rust), the deb case is properly enabled in the build type switch, and the final output listing appropriately shows all possible binary locations. Build orchestration aligns with the packaging scripts.Also applies to: 201-203, 222-227
docs/DEBIAN-INSTALL.md (1)
146-160: Uninstall paths are now clear and mutually consistent.The distinction between non-purge (preserve data) and purge (remove all) is now explicit, with each path clearly labeled and the destructive behavior documented. The previous review concern about conflicting guidance has been addressed.
build-deb.sh (1)
29-40: Prerequisite checks, file copying, and dpkg-deb handling are solid.Binary and webdeploy validation work correctly; permissions are set appropriately; installed-size calculation is accurate; and the fallback for missing dpkg-deb gracefully handles limited environments while still leaving useful artifacts.
Also applies to: 115-139, 143-151
.github/workflows/on-push-build-all.yml (2)
116-116: Binary copy now correctly referencesfilepi(not the oldfilepi-rust).The path correctly points to the actual Rust binary produced by cargo, ensuring the packaging step will find the binary. This fixes the prior critical issue flagged in the previous review.
128-206: Multi-arch CI/CD orchestration is well-structured.Setup, Blazor build, multi-arch Rust builds, and packaging jobs are logically organized with appropriate dependencies. Artifact naming, uploading, and verification steps are clear and maintainable. The tarball and Debian package creation workflows separate cleanly without duplication.
.github/workflows/on-tag-push-create-release.yml (3)
276-309: Asset verification and fail-fast behavior now properly in place.The explicit verification step (lines 276–296) counts expected tarballs and Debian packages, failing clearly if either count is zero. Combined with
fail_on_unmatched_files: true(line 309), the release action now fails fast rather than silently creating an incomplete release. This directly addresses the previous review concern about missing safeguards.
50-74: Latest build lookup handles edge cases well.The GitHub API query to fetch the latest successful
main_rsbuild includes proper error handling (lines 65–68) when no successful builds exist. Output of run ID and commit SHA is useful for traceability.
154-173: Asset renaming and release notes generation are clear and appropriate.The sed-based tag replacement (lines 158, 169) is readable and handles the version substitution correctly. Release notes (lines 200–274) are well-organized with clear sections, installation steps, feature highlights, and links. Format and content are user-friendly.
Also applies to: 200-274
5e76d37 to
b38c617
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/on-tag-push-create-release.yml (1)
158-158: Consider using bash parameter expansion instead ofsedfor variable substitution.Lines 158 and 169 use
sedto replace the build number placeholder. For improved readability and to align with shell best practices, you could use bash parameter expansion instead:# Replace the build name with tag name in filename - new_filename=$(echo "$filename" | sed "s/main_rs_[0-9]\+/${TAG_NAME}/g") + new_filename="${filename//main_rs_[0-9]*/}" # or similar pattern + new_filename="${TAG_NAME}${new_filename##*main_rs_[0-9]*}"Alternatively, if the pattern match is non-trivial for parameter expansion,
sedis acceptable. This is purely optional.Also applies to: 169-169
.github/workflows/on-push-build-all.yml (2)
25-49: Optional: Address shellcheck style suggestions in the setup script.The static analysis tool flagged several minor shell script style issues:
- Variables in
echostatements should be quoted- Multiple redirects to
$GITHUB_OUTPUTcould be combined using a brace groupApply this diff to improve shell script style:
- echo "build-name=$VERSION" >> $GITHUB_OUTPUT - echo "version=$VERSION" >> $GITHUB_OUTPUT - echo "deb-version=$DEB_VERSION" >> $GITHUB_OUTPUT + { + echo "build-name=${VERSION}" + echo "version=${VERSION}" + echo "deb-version=${DEB_VERSION}" + } >> "$GITHUB_OUTPUT"
219-241: Optional: Address shellcheck warnings in the summary script.The static analysis tool flagged several shell script style issues in the artifact listing logic:
- Using
ls -lhpiped toawkinstead of more portable commands- Unquoted variables in several places
Consider applying these improvements:
echo "### 📦 Rust Binaries (with Blazor WebAssembly UI):" >> $GITHUB_STEP_SUMMARY for file in all-artifacts/*.tar.gz; do if [ -f "$file" ]; then filename=$(basename "$file") - size=$(ls -lh "$file" | awk '{print $5}') - echo "- $filename ($size)" >> $GITHUB_STEP_SUMMARY + size=$(du -h "$file" | cut -f1) + echo "- ${filename} (${size})" >> "$GITHUB_STEP_SUMMARY" fi done echo "" >> $GITHUB_STEP_SUMMARY echo "### 📦 Debian Packages:" >> $GITHUB_STEP_SUMMARY for file in all-artifacts/*.deb; do if [ -f "$file" ]; then filename=$(basename "$file") - size=$(ls -lh "$file" | awk '{print $5}') - echo "- $filename ($size)" >> $GITHUB_STEP_SUMMARY + size=$(du -h "$file" | cut -f1) + echo "- ${filename} (${size})" >> "$GITHUB_STEP_SUMMARY" fi donedebian/control (1)
5-5: Update debhelper-compat to version 13 and Standards-Version to 4.7.0.The current debhelper-compat (= 12) is from Debian 10, which has limited support. Version 13 (available since Debian 11) is the current standard for modern Debian packaging and better aligns with the package's
unstabledistribution target. Updating Standards-Version from 4.5.0 to 4.7.0 would also ensure compliance with current Debian policy.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/on-push-build-all.yml(1 hunks).github/workflows/on-tag-push-create-release.yml(1 hunks)build-deb.sh(1 hunks)build.ps1(1 hunks)build.sh(6 hunks)debian/control(1 hunks)debian/postinst(1 hunks)debian/postrm(1 hunks)debian/rules(1 hunks)docs/DEBIAN-INSTALL.md(1 hunks)frontend/FilePiWeb/Layout/EmptyLayout.razor(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- build.ps1
🚧 Files skipped from review as they are similar to previous changes (6)
- debian/postrm
- docs/DEBIAN-INSTALL.md
- debian/rules
- build.sh
- build-deb.sh
- frontend/FilePiWeb/Layout/EmptyLayout.razor
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/on-push-build-all.yml
25-25: shellcheck reported issue in this script: SC2086:info:18:31: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:19:28: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:20:36: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2129:style:18:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
220-220: shellcheck reported issue in this script: SC2012:info:18:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
220-220: shellcheck reported issue in this script: SC2012:info:9:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:13:12: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:14:34: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:19:35: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:3:54: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:4:12: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2086:info:5:61: Double quote to prevent globbing and word splitting
(shellcheck)
220-220: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
.github/workflows/on-tag-push-create-release.yml
130-130: shellcheck reported issue in this script: SC2001:style:28:20: See if you can use ${variable//search/replace} instead
(shellcheck)
130-130: shellcheck reported issue in this script: SC2001:style:39:20: See if you can use ${variable//search/replace} instead
(shellcheck)
277-277: shellcheck reported issue in this script: SC2012:info:12:13: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
277-277: shellcheck reported issue in this script: SC2012:info:4:17: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
318-318: shellcheck reported issue in this script: SC2012:info:12:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:14:61: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:1:60: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:20:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:21:31: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:22:50: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:23:49: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:24:47: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:25:40: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:26:42: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:3:65: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:4:72: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:5:117: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:6:96: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:7:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:8:33: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
318-318: shellcheck reported issue in this script: SC2129:style:20:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (3)
.github/workflows/on-tag-push-create-release.yml (1)
276-309: ✅ Previous critical issue fully resolved.The verification step (lines 276–296) now explicitly checks for both tarball and Debian package patterns and fails immediately if either is missing, before attempting release creation. Combined with
fail_on_unmatched_files: trueon line 309, this ensures the release cannot be created with missing assets. The fail-fast approach is solid..github/workflows/on-push-build-all.yml (2)
79-126: Build Rust job looks good!The cross-compilation setup for amd64 and arm64 is correctly configured with appropriate toolchain targets and linkers. The binary name reference has been fixed from the previous review.
59-62: No issue with .NET version specification..NET 10 was released on November 11, 2025, and is available as an LTS release. The workflow's specification of
dotnet-version: '10.0.x'is correct and will not cause build failures.
| - name: Build Debian package | ||
| run: | | ||
| echo "Building Debian package..." | ||
| chmod +x build-deb.sh | ||
| ./build-deb.sh "${{ needs.setup.outputs.deb-version }}" "${{ matrix.arch }}" | ||
|
|
||
| # Rename the deb file to include architecture | ||
| mv outputs/filepi_*.deb filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb | ||
|
|
||
| echo "Debian package created:" | ||
| ls -la filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen the Debian package rename operation.
The mv command on line 191 uses a glob pattern that could match multiple files or fail if the expected file doesn't exist. This could cause silent failures or unexpected behavior.
Apply this diff to make the rename more robust:
echo "Building Debian package..."
chmod +x build-deb.sh
./build-deb.sh "${{ needs.setup.outputs.deb-version }}" "${{ matrix.arch }}"
- # Rename the deb file to include architecture
- mv outputs/filepi_*.deb filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb
+ # Rename the deb file to include build name
+ DEB_FILE=$(find outputs -name "filepi_*.deb" -type f)
+ if [ -z "$DEB_FILE" ]; then
+ echo "ERROR: No .deb file found in outputs/"
+ exit 1
+ fi
+ mv "$DEB_FILE" filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb
echo "Debian package created:"
ls -la filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Build Debian package | |
| run: | | |
| echo "Building Debian package..." | |
| chmod +x build-deb.sh | |
| ./build-deb.sh "${{ needs.setup.outputs.deb-version }}" "${{ matrix.arch }}" | |
| # Rename the deb file to include architecture | |
| mv outputs/filepi_*.deb filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb | |
| echo "Debian package created:" | |
| ls -la filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb | |
| - name: Build Debian package | |
| run: | | |
| echo "Building Debian package..." | |
| chmod +x build-deb.sh | |
| ./build-deb.sh "${{ needs.setup.outputs.deb-version }}" "${{ matrix.arch }}" | |
| # Rename the deb file to include build name | |
| DEB_FILE=$(find outputs -name "filepi_*.deb" -type f) | |
| if [ -z "$DEB_FILE" ]; then | |
| echo "ERROR: No .deb file found in outputs/" | |
| exit 1 | |
| fi | |
| mv "$DEB_FILE" filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb | |
| echo "Debian package created:" | |
| ls -la filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb |
🤖 Prompt for AI Agents
In .github/workflows/on-push-build-all.yml around lines 184–194, the mv that
renames the generated .deb uses a glob that can match zero or multiple files;
replace it with a robust check: expand the glob into an array, validate there is
exactly one match, print a clear error and exit non‑zero if none or multiple
matches, then perform the mv using that single filename to the target filepi_${{
needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb; keep the working
directory explicit and fail the step on error.
| find /opt/filepi/webdeploy -type d -exec chmod 755 {} \; | ||
|
|
||
| # Create the cache directory | ||
| mkdir -p /var/lib/filepi/media/.cache |
There was a problem hiding this comment.
Set ownership and permissions for the cache directory.
The cache directory /var/lib/filepi/media/.cache is created but no ownership or permissions are explicitly set. If the service runs as a non-root user, it may not have write access to this directory.
Apply this diff to set appropriate ownership and permissions:
# Create the cache directory
mkdir -p /var/lib/filepi/media/.cache
+chown root:root /var/lib/filepi/media/.cache
+chmod 755 /var/lib/filepi/media/.cacheNote: Adjust the ownership if the service runs as a different user.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p /var/lib/filepi/media/.cache | |
| mkdir -p /var/lib/filepi/media/.cache | |
| chown root:root /var/lib/filepi/media/.cache | |
| chmod 755 /var/lib/filepi/media/.cache |
🤖 Prompt for AI Agents
In debian/postinst around line 15, the script creates
/var/lib/filepi/media/.cache but does not set ownership or permissions; update
the postinst to create the directory and then set its owner to the service user
(e.g., chown filepi:filepi /var/lib/filepi/media/.cache) and set safe writable
permissions for the service (e.g., chmod 0755 or 0775 if a group needs write
access); adjust the user/group and mode if your service runs as a different user
or requires group write access.
| # Reload systemd to recognize the new service | ||
| systemctl daemon-reload | ||
|
|
||
| # Enable and start the service | ||
| systemctl enable filepi.service | ||
| systemctl start filepi.service || true |
There was a problem hiding this comment.
Add systemd availability check before calling systemctl.
The script calls systemctl without checking if systemd is available, which will fail in chroot environments during package builds or in containers.
Apply this diff to add a systemd check:
- # Reload systemd to recognize the new service
- systemctl daemon-reload
-
- # Enable and start the service
- systemctl enable filepi.service
- systemctl start filepi.service || true
+ # Reload systemd to recognize the new service
+ if [ -d /run/systemd/system ]; then
+ systemctl daemon-reload
+ systemctl enable filepi.service
+ systemctl start filepi.service || true
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Reload systemd to recognize the new service | |
| systemctl daemon-reload | |
| # Enable and start the service | |
| systemctl enable filepi.service | |
| systemctl start filepi.service || true | |
| # Reload systemd to recognize the new service | |
| if [ -d /run/systemd/system ]; then | |
| systemctl daemon-reload | |
| systemctl enable filepi.service | |
| systemctl start filepi.service || true | |
| fi |
🤖 Prompt for AI Agents
In debian/postinst around lines 17 to 22, the script calls systemctl
unconditionally which fails in chroot/container builds; wrap the daemon-reload,
enable and start calls in a guard that checks for systemd availability (e.g.,
verify systemctl exists and that /run/systemd/system or similar runtime
indicator is present) and only run the systemctl commands inside that
conditional, preserving the existing fallback behavior for start (|| true) if
desired.
b38c617 to
a41c7f7
Compare
a41c7f7 to
20843d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/on-push-build-all.yml (1)
186-196: Glob pattern for deb rename could fail silently or match multiple files.Line 193 uses
mv outputs/filepi_*.deb filepi_...which could:
- Fail silently if no .deb is found
- Match multiple files unpredictably
Apply this diff to add explicit validation:
# Rename the deb file to include architecture - mv outputs/filepi_*.deb filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb + DEB_FILE=$(find outputs -name "filepi_*.deb" -type f) + if [ -z "$DEB_FILE" ]; then + echo "ERROR: No .deb file found in outputs/" + exit 1 + fi + mv "$DEB_FILE" filepi_${{ needs.setup.outputs.build-name }}_${{ matrix.arch }}.deb
🧹 Nitpick comments (4)
build.sh (1)
155-155: Simplify redundant binary copy with fallback.Lines 155 and 164 contain duplicate
cpcommands with error suppression that doesn't add value. If the binary is expected in one location, copying with|| truecould mask real failures silently.Consider simplifying to a single, strict copy that fails loudly on error:
- # Copy binary to root for easier access - cp target/release/filepi ./filepi || cp target/release/filepi ./filepi 2>/dev/null || true + # Copy binary to root for easier access + cp target/release/filepi ./filepiThis ensures errors are caught early and the script output is clearer. If both locations need to be checked, use an explicit conditional instead.
Also applies to: 164-164
build-in-docker.sh (1)
5-9: Remove unused YELLOW variable.Line 7 defines
YELLOWbut it's never used in the script output. Remove it to avoid confusion and pass shell linting.RED='\033[0;31m' GREEN='\033[0;32m' -YELLOW='\033[1;33m' BLUE='\033[0;34m' NC='\033[0m' # No Colorbuild-deb.sh (1)
84-98: Multiple sed operations could be consolidated for clarity.Lines 84–98 perform several sequential sed operations to clean up debhelper placeholders. This could be consolidated into a single pass for better performance and readability:
- # Remove debhelper variable placeholders - sed 's/${shlibs:Depends}, //g' "$CONTROL_FILE" > "$CONTROL_TMP" - mv "$CONTROL_TMP" "$CONTROL_FILE" - - sed 's/${misc:Depends}, //g' "$CONTROL_FILE" > "$CONTROL_TMP" - mv "$CONTROL_TMP" "$CONTROL_FILE" - - sed 's/${shlibs:Depends}//g' "$CONTROL_FILE" > "$CONTROL_TMP" - mv "$CONTROL_TMP" "$CONTROL_FILE" - - sed 's/${misc:Depends}//g' "$CONTROL_FILE" > "$CONTROL_TMP" - mv "$CONTROL_TMP" "$CONTROL_FILE" - - # Clean up any trailing commas or spaces in Depends - sed 's/Depends: , /Depends: /g' "$CONTROL_FILE" > "$CONTROL_TMP" - mv "$CONTROL_TMP" "$CONTROL_FILE" + # Remove debhelper variable placeholders and clean up + sed -E \ + -e 's/\$\{shlibs:Depends\}, //g' \ + -e 's/\$\{misc:Depends\}, //g' \ + -e 's/\$\{shlibs:Depends\}//g' \ + -e 's/\$\{misc:Depends\}//g' \ + -e 's/Depends: , /Depends: /g' \ + "$CONTROL_FILE" > "$CONTROL_TMP" + mv "$CONTROL_TMP" "$CONTROL_FILE".github/workflows/on-tag-push-create-release.yml (1)
154-162: Consider using bash parameter expansion instead of sed for filename transformation.Lines 158 and 169 use
sedto replace build numbers with tag names in filenames. This can be simplified using bash parameter expansion for better readability and performance:# Process binary tarballs for tarball in ${{ env.ARTIFACTS_DIR }}/*.tar.gz; do if [ -f "$tarball" ]; then filename=$(basename "$tarball") - # Replace the build name with tag name in filename - new_filename=$(echo "$filename" | sed "s/main_rs_[0-9]\+/${TAG_NAME}/g") + # Replace the build name with tag name in filename + new_filename="${filename//main_rs_[0-9]*/${TAG_NAME}}" cp "$tarball" "${{ env.RELEASE_DIR }}/${new_filename}" echo "Prepared: ${new_filename}" fi done # Process Debian packages for deb in ${{ env.ARTIFACTS_DIR }}/*.deb; do if [ -f "$deb" ]; then filename=$(basename "$deb") - # Replace the build name with tag name in filename - new_filename=$(echo "$filename" | sed "s/main_rs_[0-9]\+/${TAG_NAME}/g") + # Replace the build name with tag name in filename + new_filename="${filename//main_rs_[0-9]*/${TAG_NAME}}" cp "$deb" "${{ env.RELEASE_DIR }}/${new_filename}" echo "Prepared: ${new_filename}" fi doneAlso applies to: 164-173
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.dockerignore(1 hunks).github/workflows/on-push-build-all.yml(1 hunks).github/workflows/on-tag-push-create-release.yml(1 hunks).gitignore(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)build-deb.sh(1 hunks)build-in-docker.sh(1 hunks)build.ps1(1 hunks)build.sh(6 hunks)debian/changelog(1 hunks)debian/control(1 hunks)debian/filepi.service(1 hunks)debian/postinst(1 hunks)debian/postrm(1 hunks)debian/rules(1 hunks)docs/DEBIAN-INSTALL.md(1 hunks)frontend/FilePiWeb/Layout/EmptyLayout.razor(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- build.ps1
- debian/postrm
- frontend/FilePiWeb/Layout/EmptyLayout.razor
🚧 Files skipped from review as they are similar to previous changes (9)
- Dockerfile
- debian/control
- debian/changelog
- .gitignore
- debian/rules
- .dockerignore
- debian/filepi.service
- debian/postinst
- docs/DEBIAN-INSTALL.md
🧰 Additional context used
🧬 Code graph analysis (1)
build.sh (1)
build-in-docker.sh (3)
print_error(19-21)print_step(11-13)print_success(15-17)
🪛 actionlint (1.7.9)
.github/workflows/on-push-build-all.yml
25-25: shellcheck reported issue in this script: SC2086:info:18:31: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:19:28: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:20:36: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2129:style:18:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
222-222: shellcheck reported issue in this script: SC2012:info:18:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
222-222: shellcheck reported issue in this script: SC2012:info:9:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:13:12: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:14:34: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:19:35: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:3:54: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:4:12: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2086:info:5:61: Double quote to prevent globbing and word splitting
(shellcheck)
222-222: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
.github/workflows/on-tag-push-create-release.yml
130-130: shellcheck reported issue in this script: SC2001:style:28:20: See if you can use ${variable//search/replace} instead
(shellcheck)
130-130: shellcheck reported issue in this script: SC2001:style:39:20: See if you can use ${variable//search/replace} instead
(shellcheck)
277-277: shellcheck reported issue in this script: SC2012:info:12:13: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
277-277: shellcheck reported issue in this script: SC2012:info:4:17: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
318-318: shellcheck reported issue in this script: SC2012:info:12:12: Use find instead of ls to better handle non-alphanumeric filenames
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:14:61: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:16:56: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:1:60: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:20:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:21:31: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:22:50: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:23:49: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:24:47: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:25:40: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:26:42: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:3:65: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:4:72: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:5:117: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:6:96: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:7:12: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2086:info:8:33: Double quote to prevent globbing and word splitting
(shellcheck)
318-318: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
318-318: shellcheck reported issue in this script: SC2129:style:20:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🪛 markdownlint-cli2 (0.18.1)
README.md
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
21-21: Headings must start at the beginning of the line
(MD023, heading-start-left)
27-27: Headings must start at the beginning of the line
(MD023, heading-start-left)
🪛 Shellcheck (0.11.0)
build-in-docker.sh
[warning] 7-7: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Blazor Frontend
🔇 Additional comments (5)
build-in-docker.sh (1)
58-87: Comprehensive prerequisites and helpful output.The Docker packaging script does a good job validating that both the
filepibinary andwebdeploydirectory exist before attempting the build, with clear error messages guiding the user to the correct build commands. The user ID preservation (-uflag) and final verification command example are solid touches for a containerized workflow..github/workflows/on-push-build-all.yml (1)
110-128: Binary name and artifact handling look correct.Line 118 correctly references the
filepibinary (notfilepi-rust), matching the Cargo.toml binary name. The cross-compilation setup and target-specific linking environment variable are properly configured for arm64 builds.build-deb.sh (1)
28-40: Solid prerequisite validation and version normalization.The script properly validates that both the
filepibinary andwebdeploydirectory exist before proceeding, with clear error guidance. Version normalization (underscore-to-hyphen conversion at line 18) ensures Debian package naming compliance. The package structure creation and control file processing handle standard Debian metadata correctly..github/workflows/on-tag-push-create-release.yml (2)
276-309: Excellent defensive design with explicit verification and fail-fast behavior.The workflow includes comprehensive checks:
- Explicit verification step (lines 276–296) counts tarballs and .deb files, failing if counts are zero
fail_on_unmatched_files: trueat line 309 ensures the release action fails if asset patterns don't match- This dual-layer validation prevents silent failures and ensures release integrity
200-274: Well-structured release notes with comprehensive user guidance.The generated release notes include feature highlights, quick-start instructions for both Debian packages and binary installations, documentation links, and architecture information. The formatting is clear and user-friendly, with appropriate emoji for visual scanning.
| * **.Net Libman** | ||
|
|
||
| * Install .Net Libman | ||
| ```bash | ||
| dotnet tool install --global dotnet-libman | ||
| ``` | ||
|
|
||
| * **FFmpeg** | ||
|
|
||
| #### On Raspberry Pi / Debian / Ubuntu: | ||
| ```bash | ||
| sudo apt update | ||
| sudo apt install ffmpeg | ||
| ``` | ||
|
|
||
| #### On macOS: | ||
| ```bash | ||
| brew install ffmpeg | ||
| ``` |
There was a problem hiding this comment.
Fix Markdown formatting issues to pass linting.
Several formatting violations are present:
- Line 14: List item indentation should be 2 spaces, not 4 (MD007).
- Lines 21 and 27: Headings should increment by one level (should be h3
###not h4####under h2 section) (MD001). - Lines 21 and 27: Headings must start at column 0, not indented with spaces (MD023).
Apply this diff to fix:
-* **.Net Libman**
+* **.NET LibMan**
- * Install .Net Libman
+ * Install .NET LibMan
```bash
dotnet tool install --global dotnet-libman
```
-* **FFmpeg**
+* **FFmpeg**
- #### On Raspberry Pi / Debian / Ubuntu:
+ ### On Raspberry Pi / Debian / Ubuntu:
```bash
sudo apt update
sudo apt install ffmpeg
```
- #### On macOS:
+ ### On macOS:
```bash
brew install ffmpeg
```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
21-21: Headings must start at the beginning of the line
(MD023, heading-start-left)
27-27: Headings must start at the beginning of the line
(MD023, heading-start-left)
🤖 Prompt for AI Agents
In README.md around lines 12 to 30, fix Markdown lint issues by changing the
".Net Libman" sub-list item indentation from 4 spaces to 2 spaces, and change
the two indented level-4 headings ("#### On Raspberry Pi / Debian / Ubuntu:" and
"#### On macOS:") to level-3 headings ("### ...") and remove their leading
spaces so each heading starts at column 0; ensure code fences remain intact
after these edits.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.