feat: add Fedora RPM packaging support - #101
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughLinux packaging now includes RPM. The build workflow installs RPM tooling and uploads generated RPM files with the other Linux artifacts. ChangesLinux RPM packaging and distribution
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🧹 Nitpick comments (1)
flatpak/com.etiennelescot.openscreen.yml (1)
38-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize the Linux build command for Flatpak.
Running
npm run build:linuxwill invokeelectron-builderto package the AppImage, deb, pacman, and rpm formats. Since the Flatpak build only needs the unpacked binary directory (linux-unpacked), generating all these formats wastes significant build time and disk space.Consider running the Vite build step explicitly and instructing
electron-builderto only produce the unpacked directory.♻️ Proposed refactor for faster builds
# Install dependencies and build packaged assets - npm install - - npm run build:linux + - npm run build-vite + - npx electron-builder --linux dir --config.npmRebuild=false🤖 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 `@flatpak/com.etiennelescot.openscreen.yml` around lines 38 - 39, Replace the flatpak build step that invokes npm run build:linux with an explicit Vite build followed by electron-builder configured to generate only the linux-unpacked directory, preserving the existing npm install step.
🤖 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 `@flatpak/com.etiennelescot.openscreen.yml`:
- Around line 47-50: Update the git source URL in the Flatpak manifest’s sources
entry to the active continuation fork instead of the archived
siddharthvaddem/openscreen.git repository, while preserving the main branch
configuration.
- Around line 33-36: Update the module’s build-options in the Flatpak manifest
to explicitly enable network access during build-commands, while preserving the
existing PATH environment configuration. Add the standard network-sharing option
at the build-options level so npm install can resolve and download dependencies.
- Around line 44-46: Update the Flatpak installation steps near the icon
installs to create an openscreen.desktop entry containing the application name,
executable, icon, and required desktop-entry metadata, then install it into
/app/share/applications/openscreen.desktop. Ensure this matches the manifest’s
rename-desktop-file value and enables launcher integration.
In `@README.md`:
- Around line 124-126: The release output paths are hardcoded to version 1.6.0.
Update README.md lines 124-126 to use a wildcard release directory in the RPM
installation command, and update flatpak/com.etiennelescot.openscreen.yml lines
40-41 to use a wildcard release directory in the linux-unpacked copy command.
---
Nitpick comments:
In `@flatpak/com.etiennelescot.openscreen.yml`:
- Around line 38-39: Replace the flatpak build step that invokes npm run
build:linux with an explicit Vite build followed by electron-builder configured
to generate only the linux-unpacked directory, preserving the existing npm
install step.
🪄 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: cbef1f34-c471-4f0a-a52d-a7fd6048a265
📥 Commits
Reviewing files that changed from the base of the PR and between d5966ed and f9b37599079d24ca9e63e1a7ae9b4b38267e196b.
📒 Files selected for processing (5)
.gitignoreREADME.mdelectron-builder.json5flatpak/com.etiennelescot.openscreen.ymlpackage.json
| build-options: | ||
| env: | ||
| - PATH=/usr/lib/sdk/node22/bin:/usr/bin:/bin | ||
| build-commands: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Allow network access during the build phase.
npm install requires network access to resolve and download dependencies. By default, flatpak-builder isolates the network namespace during the build-commands phase, which will cause the build to fail immediately. You must explicitly share the network in the module's build-options.
🐛 Proposed fix to enable network access
build-options:
+ build-args:
+ - --share=network
env:
- PATH=/usr/lib/sdk/node22/bin:/usr/bin:/bin
build-commands:📝 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-options: | |
| env: | |
| - PATH=/usr/lib/sdk/node22/bin:/usr/bin:/bin | |
| build-commands: | |
| build-options: | |
| build-args: | |
| - --share=network | |
| env: | |
| - PATH=/usr/lib/sdk/node22/bin:/usr/bin:/bin | |
| build-commands: |
🤖 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 `@flatpak/com.etiennelescot.openscreen.yml` around lines 33 - 36, Update the
module’s build-options in the Flatpak manifest to explicitly enable network
access during build-commands, while preserving the existing PATH environment
configuration. Add the standard network-sharing option at the build-options
level so npm install can resolve and download dependencies.
| # Install desktop entry and icons | ||
| - install -Dm644 icons/icons/png/512x512.png /app/share/icons/hicolor/512x512/apps/openscreen.png | ||
| - install -Dm644 public/openscreen.png /app/share/pixmaps/openscreen.png |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Provide a .desktop file to integrate with the application launcher.
The manifest specifies rename-desktop-file: openscreen.desktop (Line 8), but electron-builder's unpacked output does not include a .desktop file. Without explicitly creating and installing one into /app/share/applications/, the application will not appear in the user's desktop environment or application menu.
🐛 Proposed fix to create and install the desktop entry
# Install desktop entry and icons
+ - mkdir -p /app/share/applications
+ - |
+ cat <<EOF > /app/share/applications/openscreen.desktop
+ [Desktop Entry]
+ Name=Openscreen
+ Exec=openscreen %U
+ Terminal=false
+ Type=Application
+ Icon=openscreen
+ StartupWMClass=openscreen
+ Categories=AudioVideo;
+ EOF
- install -Dm644 icons/icons/png/512x512.png /app/share/icons/hicolor/512x512/apps/openscreen.png
- install -Dm644 public/openscreen.png /app/share/pixmaps/openscreen.png📝 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.
| # Install desktop entry and icons | |
| - install -Dm644 icons/icons/png/512x512.png /app/share/icons/hicolor/512x512/apps/openscreen.png | |
| - install -Dm644 public/openscreen.png /app/share/pixmaps/openscreen.png | |
| # Install desktop entry and icons | |
| - mkdir -p /app/share/applications | |
| - | | |
| cat <<EOF > /app/share/applications/openscreen.desktop | |
| [Desktop Entry] | |
| Name=Openscreen | |
| Exec=openscreen %U | |
| Terminal=false | |
| Type=Application | |
| Icon=openscreen | |
| StartupWMClass=openscreen | |
| Categories=AudioVideo; | |
| EOF | |
| - install -Dm644 icons/icons/png/512x512.png /app/share/icons/hicolor/512x512/apps/openscreen.png | |
| - install -Dm644 public/openscreen.png /app/share/pixmaps/openscreen.png |
🤖 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 `@flatpak/com.etiennelescot.openscreen.yml` around lines 44 - 46, Update the
Flatpak installation steps near the icon installs to create an
openscreen.desktop entry containing the application name, executable, icon, and
required desktop-entry metadata, then install it into
/app/share/applications/openscreen.desktop. Ensure this matches the manifest’s
rename-desktop-file value and enables launcher integration.
| sources: | ||
| - type: git | ||
| url: https://github.com/siddharthvaddem/openscreen.git | ||
| branch: main |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Point source URL to the active repository.
The manifest is currently pointing to the archived original repository (siddharthvaddem/openscreen.git). This will cause flatpak-builder to fetch the old codebase, completely missing your new packaging features and any ongoing community fixes.
🐛 Proposed fix to use the continuation fork
sources:
- type: git
- url: https://github.com/siddharthvaddem/openscreen.git
+ url: https://github.com/EtienneLescot/openscreen.git
branch: main📝 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.
| sources: | |
| - type: git | |
| url: https://github.com/siddharthvaddem/openscreen.git | |
| branch: main | |
| sources: | |
| - type: git | |
| url: https://github.com/EtienneLescot/openscreen.git | |
| branch: main |
🤖 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 `@flatpak/com.etiennelescot.openscreen.yml` around lines 47 - 50, Update the
git source URL in the Flatpak manifest’s sources entry to the active
continuation fork instead of the archived siddharthvaddem/openscreen.git
repository, while preserving the main branch configuration.
| # Install the generated RPM locally | ||
| sudo dnf install ./release/1.6.0/Openscreen-Linux-*.rpm | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded release version in build output paths.
Both the documentation and the Flatpak manifest hardcode the 1.6.0 version in paths referencing the release/ directory. When the application version is updated, the Flatpak build will fail and the documented installation commands will become stale. Ensure you use a wildcard directory match.
README.md#L124-L126: replacesudo dnf install ./release/1.6.0/Openscreen-Linux-*.rpmwithsudo dnf install ./release/*/Openscreen-Linux-*.rpm.flatpak/com.etiennelescot.openscreen.yml#L40-L41: replacecp -r release/1.6.0/linux-unpacked/* /app/withcp -r release/*/linux-unpacked/* /app/.
📍 Affects 2 files
README.md#L124-L126(this comment)flatpak/com.etiennelescot.openscreen.yml#L40-L41
🤖 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 `@README.md` around lines 124 - 126, The release output paths are hardcoded to
version 1.6.0. Update README.md lines 124-126 to use a wildcard release
directory in the RPM installation command, and update
flatpak/com.etiennelescot.openscreen.yml lines 40-41 to use a wildcard release
directory in the linux-unpacked copy command.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Review — REQUEST CHANGES
Welcome direction: adding RPM and Flatpak is a reasonable complement to the existing homebrew / winget / nix / aur Tier-3 list. But the Flatpak manifest as written will fail to build, and a few smaller issues need attention before this can land. Suggested path: scope this PR to RPM-only (clean ~10-line change), and iterate the Flatpak manifest against the issues below as a separate PR.
Flatpak manifest (flatpak/com.etiennelescot.openscreen.yml)
- Dead-on-arrival build commands.
npm run build:linuxrunselectron-builder --linux AppImage deb pacman rpm— those are packaging targets. electron-builder only emitslinux-unpacked/when--diris passed, sorelease/1.6.0/linux-unpacked/doesn't exist and thecpat line 41 fails with "No such file or directory". Either split intobuild:flatpak(electron-builder --linux dir --config.npmRebuild=false) or add--dirto the existing script. - Hardcoded version
1.6.0at line 41. Will break on every release bump (the promote workflow rewritespackage.json). Derive frompackage.jsonat build time, e.g.release/$(node -p "require('./package.json').version")/linux-unpacked/. - Desktop file is never installed.
rename-desktop-file: openscreen.desktoptells flatpak-builder to look foropenscreen.desktopin the final tree and rename it. Nothing in the build commands copies a.desktopfile into/app/— electron-builder generates one inside the package artifacts but not in the unpacked dir. The build will fail with "desktop file not found". - Missing
--talk-name=org.freedesktop.portal.Desktop. Without it, screen capture through the XDG Desktop Portal will silently fail on GNOME/KDE Wayland — the recorder's primary use case. The current--talk-name=org.freedesktop.Notificationsis unrelated. --device=allis over-broad. A recorder only needs the camera (--device=video); microphone/speakers go through PulseAudio (already granted via--socket=pulseaudio).- Source URL points at the archived upstream. Line 49 references
https://github.com/siddharthvaddem/openscreen.git— that's the original author's archived repo. This repo is the community continuation (perAGENTS.mdandpackage.json#maintainers). A flatpak built from this URL would ship the archived code, not what users get fromdeb/rpm/pacman. Should behttps://github.com/EtienneLescot/openscreen.git(or this repo). branch: mainis not Flathub-acceptable. Abranch: mainsource makes builds non-reproducible and won't pass Flathub review. Pin to a tag (e.g.tag: v1.6.0) or document that Flathub submission is out of scope and this is a personal "build locally" manifest.
Tier-3 doc / CI gaps (out of immediate PR but should be tracked)
.github/workflows/build.ymldoes not upload the new.rpmartifact — thepath:glob at line 264-269 listsAppImage / zsync / deb / pacmanbut notrpm. Without this, CI silently drops the RPM and the README's promise is misleading.- Tier-3 references in
.harness/docs/git-workflow.md(lines 62, 84) anddocs/secrets.md(line 140) still sayhomebrew/winget/nix/aur. Either add Flatpak or explicitly call it build-locally-only. - App ID
com.etiennelescot.openscreenembeds the contributor's GitHub handle. Once a Flatpak is installed, the ID is effectively immutable (D-Bus names,~/.var/app/paths, future Flathub submissions). Consider a project-owned ID (io.github.openscreen.Openscreen) — at minimum, get maintainer input before this lands.
Other
.gitignoreaddsgraphify-out/(line 70-71) — unrelated to this PR. Looks like a stray. Flatpak build artifacts that should be gitignored are missing:build-dir/,.flatpak-builder/,*.flatpak,repos/.- The Flatpak manifest adds 8 inline comments. Per
AGENTS.md, comments should be added only when asked. They're decorative; fold into a top-of-file docstring or drop.
| # Network access for downloading Whisper/ONNX translation model | ||
| - --share=network | ||
| # Desktop notifications | ||
| - --talk-name=org.freedesktop.Notifications |
There was a problem hiding this comment.
Missing the screen-cast portal permission. This grants --talk-name=org.freedesktop.Notifications but not the XDG Desktop Portal bus name. On Wayland (the modern Linux desktop), screen capture goes through org.freedesktop.portal.Desktop (ScreenCast / RemoteDesktop / access dialogs). Without it, recording silently fails on GNOME/KDE Wayland for flatpak users — defeats the app's primary purpose.
Add:
- --talk-name=org.freedesktop.portal.Desktop
Also worth considering --socket=pcsc and --socket=cups if/when annotation or printing flows need them.
| # Audio access (Microphone and system loopback) | ||
| - --socket=pulseaudio | ||
| # Webcam and peripheral devices access | ||
| - --device=all |
There was a problem hiding this comment.
--device=all is over-broad. A screen recorder only needs the camera. Microphone/speakers already go through PulseAudio (granted via --socket=pulseaudio at line 16). Narrow to:
- --device=video
Grants every device node otherwise — a privacy regression.
| build-commands: | ||
| # Install dependencies and build packaged assets | ||
| - npm install | ||
| - npm run build:linux |
There was a problem hiding this comment.
npm run build:linux produces packaging artifacts, not linux-unpacked/. That script runs electron-builder --linux AppImage deb pacman rpm (per package.json:33). electron-builder only emits the unpacked tree when --dir is passed. The cp on line 41 will fail with "No such file or directory".
Either split into a build:flatpak script:
"build:flatpak": "tsc && vite build && electron-builder --linux dir --config.npmRebuild=false"
or add --dir to the existing script (and live with a slower packaging flow for everyone).
| - npm install | ||
| - npm run build:linux | ||
| # Copy packaged Linux application assets to Flatpak destination | ||
| - cp -r release/1.6.0/linux-unpacked/* /app/ |
There was a problem hiding this comment.
Hardcoded version 1.6.0. The promote workflow (promote.yml) strips -rc.N from package.json on every release. This literal will break on the next bump (1.6.1, 1.7.0, …).
Derive from package.json at build time:
- cp -r release/$(node -p "require('./package.json').version")/linux-unpacked/* /app/
| - npm install | ||
| - npm run build:linux | ||
| # Copy packaged Linux application assets to Flatpak destination | ||
| - cp -r release/1.6.0/linux-unpacked/* /app/ |
There was a problem hiding this comment.
Desktop file is never installed. rename-desktop-file: openscreen.desktop (line 8) tells flatpak-builder to look for openscreen.desktop in the final tree and rename it to com.etiennelescot.openscreen.desktop. Nothing in the build commands copies a .desktop file into /app/ — electron-builder generates one inside the package artifacts, not in the unpacked tree. Build will fail with "desktop file not found".
Either copy from electron-builder's output (e.g. cp release/<ver>/linux-unpacked/resources/openscreen.desktop /app/) or generate a minimal one inline.
| - install -Dm644 public/openscreen.png /app/share/pixmaps/openscreen.png | ||
| sources: | ||
| - type: git | ||
| url: https://github.com/siddharthvaddem/openscreen.git |
There was a problem hiding this comment.
Source URL points at the archived upstream. siddharthvaddem/openscreen.git is the original author's archived repo (per package.json#maintainers and AGENTS.md). This repo is the community continuation. A flatpak built from this URL would ship the archived code, not what users get from deb/rpm/pacman.
Change to:
url: https://github.com/EtienneLescot/openscreen.git
(or whatever canonical URL the maintainers prefer).
| sources: | ||
| - type: git | ||
| url: https://github.com/siddharthvaddem/openscreen.git | ||
| branch: main |
There was a problem hiding this comment.
branch: main is not Flathub-acceptable. Tagless / branch sources are non-reproducible and Flathub requires a pinned tag. Either pin to a tag (tag: v1.6.0) and update on each release, or document explicitly in this manifest that Flathub submission is out of scope and this is a "build locally from a checked-out branch" manifest.
| @@ -78,7 +78,8 @@ | |||
| "target": [ | |||
| "AppImage", | |||
| "deb", | |||
| "pacman" | |||
| "pacman", | |||
| "rpm" | |||
There was a problem hiding this comment.
Build will produce .rpm, but CI doesn't ship it. The target list now includes rpm at line 82, but .github/workflows/build.yml line 264-269 only uploads AppImage / zsync / deb / pacman. The new RPM will be silently dropped. Add release/**/*.rpm to the upload glob (and the publish-release job will auto-include it via find artifacts … | sort).
Or, if you want to keep this PR focused on Flatpak-only, drop the rpm entry from target and file a follow-up.
| @@ -30,7 +30,8 @@ | |||
| "build:mac": "npm run build:native:mac && tsc && vite build && electron-builder --mac", | |||
| "build:native:win": "node scripts/build-windows-wgc-helper.mjs", | |||
| "build:win": "npm run build:native:win && tsc && vite build && electron-builder --win --config.npmRebuild=false", | |||
| "build:linux": "tsc && vite build && electron-builder --linux AppImage deb pacman --config.npmRebuild=false", | |||
| "build:linux": "tsc && vite build && electron-builder --linux AppImage deb pacman rpm --config.npmRebuild=false", | |||
| "build:rpm": "tsc && vite build && electron-builder --linux rpm --config.npmRebuild=false", | |||
There was a problem hiding this comment.
build:rpm script. Adds a script that runs electron-builder --linux rpm. Combined with the electron-builder.json5:82 target, this generates the RPM on demand. Make sure the publish-release workflow (or a follow-up) actually uploads it — see electron-builder.json5:82 comment.
| @@ -66,3 +66,6 @@ result-* | |||
|
|
|||
| # Auto-caption model + ORT wasm — regenerated at build by scripts/fetch-caption-model.mjs | |||
| /caption-assets/ | |||
|
|
|||
| # graphify | |||
There was a problem hiding this comment.
Stray graphify-out/ entry. Unrelated to this PR (Flatpak / RPM). Looks like it rode along on the branch. Remove it.
Flatpak artifacts that should be gitignored but aren't:
build-dir/
.flatpak-builder/
*.flatpak
repos/
f9b3759 to
5f573ea
Compare
Mundo-Dev0ps
changed the title
|
Hi @EtienneLescot, Thanks for the detailed review! As suggested, I have scoped this PR to RPM-only to keep it clean and focused:
Please take another look when you have a moment. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 88-96: Update the Debian/Ubuntu/Pop!_OS and Fedora/RHEL/CentOS
installation commands in the README to use the versioned Electron Builder
filenames, replacing the `latest` placeholder with the actual release version
while preserving the existing package extensions and install commands.
🪄 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: b2130751-5397-4a4e-966f-32230d6b80ff
📥 Commits
Reviewing files that changed from the base of the PR and between f9b37599079d24ca9e63e1a7ae9b4b38267e196b and 5f573ea7d6de9ce985da6b90066485b00ff254ae.
📒 Files selected for processing (4)
.github/workflows/build.ymlREADME.mdelectron-builder.json5package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- electron-builder.json5
- package.json
5f573ea to
af80916
Compare
|
Hi @EtienneLescot, All conflicts have been resolved by rebasing onto the latest The PR is clean, up to date, and ready for review/merge. Thanks! |
|
@Mundo-Dev0ps Hi, thank you for your PR, I am a bit busy with a big merge currently, but I will check your PR very soon. |
|
This branch carries the whole 1.8.0 series (~458 files) that has since landed on
Rather than rebasing 630 files, branch fresh from the new RPM support is still wanted, so thanks for the work — it's the packaging around it that got in the way. |
|
Pushed a clean version to One thing the original was missing: the Linux runner only installs You're credited as co-author on the commit. Reset your branch onto it to keep this PR, or say the word and I'll open it separately. |
af80916 to
f93cd25
Compare
|
Hi @EtienneLescot, Thank you so much! I have reset the branch onto your clean commit in Appreciate you adding |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/build.yml:
- Line 344: Update the Linux build command in package.json to include the rpm
target alongside AppImage, deb, and pacman, ensuring the existing
release/**/*.rpm upload pattern has a generated artifact.
🪄 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: a38f028b-8957-4bc7-83f6-33de0a4c30ed
📥 Commits
Reviewing files that changed from the base of the PR and between 5f573ea7d6de9ce985da6b90066485b00ff254ae and f93cd25.
📒 Files selected for processing (2)
.github/workflows/build.ymlelectron-builder.json5
🚧 Files skipped from review as they are similar to previous changes (1)
- electron-builder.json5
The rpm target was declared in electron-builder.json5's `linux.target` alone. `build:linux` names its targets on the command line, and the CLI list REPLACES `linux.target` instead of extending it, so CI kept building AppImage/deb/pacman and nothing else. `if-no-files-found` evaluates the union of the upload globs -- the same trap the zsync comment on that step already documents -- so `release/**/*.rpm` would have matched nothing on every release with the job green. The target had to be added to the script too; a step now asserts one artifact per format, which is what would have caught this. electron-builder's rpm target shells out to rpmbuild, which the Linux runner doesn't have: the setup step installs libarchive-tools only, for pacman's bsdtar. Without it the build fails at the packaging step rather than at config parse, so the omission is easy to miss until CI runs. `depends` replaces electron-builder's default per target rather than extending it, so the rpm list repeats that default verbatim and then adds the two entries deb and pacman already carry for their own formats: mesa-vulkan-drivers, without which the compositor finds no Vulkan ICD and the preview is unavailable, and libsecret, which is in electron-builder's deb default but not its rpm one and which safeStorage needs to encrypt the LLM credentials. The release job needs nothing further -- it uploads whatever `find artifacts -type f` returns, so only the upload-artifact glob had to learn the new extension. README and the docs said "three packages" and pinned a `-latest.deb` filename electron-builder never emits; both now match what ships. The `.rpm` row on the download page needs its own ASSET_PATTERNS entry, or the asset is published and never linked. Co-authored-by: Mundo-Dev0ps <mundodevopsxd@gmail.com>
f93cd25 to
7a682d5
Compare
|
Force-pushed your branch onto current While re-applying it I found the change was a no-op. and that CLI list replaces What's on the branch now:
You're still co-author on the commit and this is still your PR. Thanks for pushing on this — RPM was wanted, the packaging around it just needed a second pass. One caveat for whoever merges: |
2 participants
This PR adds support for packaging OpenScreen as an RPM package for Fedora/RHEL/CentOS systems via electron-builder. It also updates the CI build workflow to upload the new RPM package to GitHub Releases, adds build scripts to package.json, and includes user documentation in the README. Flatpak support was deferred to a separate PR as suggested.
Summary by CodeRabbit