Revert "Sorting out yet another AppImage breakage."#1950
Conversation
WalkthroughThe changes update the Linux build workflow and Dockerfile by removing FUSE-related dependencies and container options. The Dockerfile now installs Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Docker as Docker Build
participant Pip as pip3
participant Patch as Patch File
CI->>Docker: Start build (no FUSE/SYS_ADMIN/AppArmor options)
Docker->>Pip: Install appimage-builder, pydpkg via pip3
Docker->>Patch: Apply appimage-dpkg.patch
Patch->>Docker: Patch version comparison logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/build/Dockerfile (1)
47-47: Consider the implications of --break-system-packages flag.The
--break-system-packagesflag allows pip to install packages that may conflict with system packages. While this resolves the immediate build issue, it could potentially lead to dependency conflicts.Consider using a virtual environment or documenting why this flag is necessary:
-RUN pip3 install --break-system-packages appimage-builder pydpkg +RUN python3 -m venv /opt/appimage-env && \ + /opt/appimage-env/bin/pip install appimage-builder pydpkg && \ + ln -s /opt/appimage-env/bin/appimage-builder /usr/bin/appimage-builder
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/linux-build.yml(0 hunks)tools/build/Dockerfile(1 hunks)tools/build/appimage-dpkg.patch(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/linux-build.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nicolasnoble
PR: grumpycoders/pcsx-redux#1885
File: src/support/djbhash.h:35-39
Timestamp: 2025-03-11T00:58:51.671Z
Learning: Tail recursion is preferred by nicolasnoble in the codebase, as it doesn't affect recursion depth when properly optimized by the compiler.
⏰ 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). (13)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: build-openbios
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: cross-arm64
- GitHub Check: asan
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: aur-build
🔇 Additional comments (4)
tools/build/Dockerfile (2)
48-49: LGTM: Patch application approach is sound.The patch file copying and application looks correct. This approach allows for targeted fixes to the appimage-builder package without modifying the upstream package directly.
40-40: AppImageKit URL change verifiedA
HEADrequest to the new URL returns a 302 redirect to a valid AppImage asset, confirming the repository path and binary name are correct.Next steps to fully validate the build changes:
- Review the
pip3 install --break-system-packagesusage—ensure it won’t conflict with system packages or consider installing in an isolated environment.- Verify that the patch file (
tools/build/appimage-builder.patch) is present, correctly referenced, and applies cleanly.- Confirm that dropping FUSE packages (e.g.,
libfuse2,fuse) won’t break any downstream AppImage mount operations.tools/build/appimage-dpkg.patch (2)
9-10: Appropriate change for Debian package version handling.Switching from
packaging.versiontopydpkg.Dpkgis the correct approach for handling Debian package versions, which have different semantics than Python package versions (PEP 440).
18-19: Confirm completeness of version comparisonsOur inspection of the patch shows only
__gt__being updated to useDpkg.compare_versions(), and no other dunder comparison methods are present in this diff. However, since this patch is applied to the externalappimagebuilderpackage, please manually review the fullPackageclass inappimagebuilder/modules/deploy/apt/package.pyto ensure that any other comparison methods (__lt__,__le__,__ge__,__eq__,__ne__) are also migrated frompackaging.version.parse()toDpkg.compare_versions().
1 participant
Reverts #1947
This actually segfaults on startup.