Add feature flags for RS3 and HDOSAndRuneLite as well as .desktop and icon file support #1
Conversation
WalkthroughThe changes in the pull request focus on the Changes
Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
pkgs/by-name/bo/bolt-launcher/package.nix (1)
146-146: Consider supporting multiple icon sizesThe icon is currently hardcoded to 256x256. Consider supporting multiple standard icon sizes (16x16, 32x32, 48x48, 128x128) for better desktop integration across different environments.
Also applies to: 149-150
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkgs/by-name/bo/bolt-launcher/package.nix(4 hunks)
🔇 Additional comments (4)
pkgs/by-name/bo/bolt-launcher/package.nix (4)
31-35: LGTM: Well-structured optional feature flags
The new parameters effectively implement the feature flags as proposed, making RS3 and HDOS/RuneLite support optional. The jdk17 dependency is properly made optional with a null default.
196-203: LGTM: Proper desktop integration setup
The desktop file and icon installation is implemented correctly using symbolic links, which is the preferred approach in Nix.
209-213: LGTM: Clear and informative package metadata
The description and longDescription effectively communicate the package's features and optional capabilities.
187-194: Review security implications of OpenSSL 1.1
The RS3 support requires openssl_1_1, which might have security implications as it's an older version. Please verify if RS3 is compatible with newer OpenSSL versions.
#!/bin/bash
# Check if RS3 client works with newer OpenSSL versions
rg -A 5 "openssl" .
| @@ -29,6 +28,9 @@ | |||
| cups, | |||
| systemd, | |||
| buildFHSEnv, | |||
| enableRS3 ? false, | |||
| enableHDOSAndRuneLite ? false, | |||
There was a problem hiding this comment.
A bit on the fence about making both of them default to false, as the only part of the package that causes issues is Runescape 3 (additionally, it will be a bit awkward if the user launches the program and ends up not being able to launch any of the games).
I think we could either enable enableHDOSAndRuneLite by default, or add an assertion to check if both are disabled. That way, the user doesn't end up with a package they cannot use (as this might be confusing if the user doesn't get any messages).
There was a problem hiding this comment.
Totally agree, any preference about which of those you want to pursue? I imagine most users will likely be looking for HDOSAndRuneLite, but if you think that is too opinionated of an approach we could do the assertion instead?
There was a problem hiding this comment.
Looking at the bigger picture though you did hit on something good there at the end, how do we communicate the options as clearly as possible to people? I don't have a ton of experience here, if there is something better than using the 'longDescription' field for that I'm up for it.
There was a problem hiding this comment.
I'd be honestly more enclined towards leaving enableHDOSandRuneLite enabled by default, and disabling enableRS3.
This is the simpler solution that I feel will inconvenience the smallest amount of users. We can also ask on the main PR for more pointers :) but I'd say for now we can just disable enableRS3 and enable HDOS/Runelite.
EDIT: I'd also remove the enableRuneLite flag, as the only reason we have it in the first place is because of the enableRS3 flag, so I'd just remove the conditional Java packaging and just keep the enableRS3 one.
There was a problem hiding this comment.
Yeah that sounds good to me, change made 👍🏻
There was a problem hiding this comment.
Thanks a lot! Everything seems to work fine. Merging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkgs/by-name/bo/bolt-launcher/package.nix (2)
Line range hint
109-116: Make jdk17 and JAVA_HOME conditional on enableHDOSAndRuneLiteThe jdk17 dependency and JAVA_HOME setting should only be included when HDOSAndRuneLite support is enabled.
Apply these changes:
buildInputs = [ mesa xorg.libX11 xorg.libxcb libarchive libz cef - jdk17 - ]; + ] ++ lib.optionals enableHDOSAndRuneLite [ + jdk17 + ]; # ... later in postFixup ... makeWrapper "$out/opt/bolt-launcher/bolt" "$out/bin/${finalAttrs.pname}-${finalAttrs.version}" \ - --set JAVA_HOME ${jdk17} + ${lib.optionalString enableHDOSAndRuneLite "--set JAVA_HOME ${jdk17}"}Also applies to: 142-142
205-208: Enhance longDescription with more details about feature flagsThe current longDescription could be more informative about the available feature flags and their implications.
Consider this enhanced description:
longDescription = '' - Bolt Launcher supports HDOS/RuneLite by default with an optional feature flag for RS3 (enableRS3). + Bolt Launcher is configured with the following feature flags: + - HDOS/RuneLite support is enabled by default (toggle with enableHDOSAndRuneLite) + - RS3 support is optional and disabled by default (enable with enableRS3) + + Note: Enabling RS3 support will pull in additional dependencies including OpenSSL 1.1. '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkgs/by-name/bo/bolt-launcher/package.nix(5 hunks)
🧰 Additional context used
📓 Learnings (1)
pkgs/by-name/bo/bolt-launcher/package.nix (2)
Learnt from: tomking
PR: nezia1/nixpkgs#1
File: pkgs/by-name/bo/bolt-launcher/package.nix:109-120
Timestamp: 2024-12-06T17:14:06.219Z
Learning: When `enableHDOSAndRuneLite` is enabled in `pkgs/by-name/bo/bolt-launcher/package.nix`, it's acceptable to not add an assertion for `jdk17` being provided.
Learnt from: tomking
PR: nezia1/nixpkgs#1
File: pkgs/by-name/bo/bolt-launcher/package.nix:153-165
Timestamp: 2024-12-06T17:13:43.231Z
Learning: In Nix package definitions (e.g., `package.nix` files), including the version number in the binary name within the desktop entry's `Exec` field is acceptable because Nix ensures that desktop entries and binaries are updated consistently upon package updates, preventing issues with desktop integration.
🔇 Additional comments (4)
pkgs/by-name/bo/bolt-launcher/package.nix (4)
145-161: LGTM! Desktop integration looks good
The desktop entry configuration is well-structured and follows Nix conventions. The versioned binary name in the exec field is acceptable as Nix ensures consistent updates.
192-199: LGTM! Desktop file installation looks good
The implementation correctly creates the necessary directories and links the desktop files and icons from the bolt package.
184-190: Verify security implications of using openssl_1_1
While the conditional RS3 dependencies are implemented correctly, using OpenSSL 1.1.x might expose users to security vulnerabilities as this version is no longer maintained.
Let's check for known vulnerabilities:
#!/bin/bash
# Description: Check for security advisories for OpenSSL 1.1.x
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "openssl") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'23-23:
Add missing enableHDOSAndRuneLite flag and make jdk17 conditional
The PR objectives mention feature flags for both RS3 and HDOSAndRuneLite, but only enableRS3 is implemented. Additionally, jdk17 should be conditional based on HDOSAndRuneLite support.
Apply these changes:
jdk17,
pango,
# ... other dependencies ...
makeDesktopItem,
copyDesktopItems,
enableRS3 ? false,
+ enableHDOSAndRuneLite ? true,Also applies to: 32-34
⛔ Skipped due to learnings
Learnt from: tomking
PR: nezia1/nixpkgs#1
File: pkgs/by-name/bo/bolt-launcher/package.nix:109-120
Timestamp: 2024-12-06T17:14:06.219Z
Learning: When `enableHDOSAndRuneLite` is enabled in `pkgs/by-name/bo/bolt-launcher/package.nix`, it's acceptable to not add an assertion for `jdk17` being provided.
Added bolt-launcher, an alternative launcher for Runescape 3 / Old School Runescape. https://github.com/Adamcake/Bolt/releases/tag/0.9.0 bolt-launcher: add plugin loader for rs3 Building the app with luajit, which allows for Runescape 3 plugin loading. bolt-launcher: add runescape 3 dependencies Added Runescape 3 dependencies inside the buildFHSEnv, so that it can be used by the binary (downloaded from the internet by bolt-launcher itself, hence why the fhs env is needed here). bolt-launcher: fix dependency issues Added libbolt-plugin.so into $out/lib, which allows the program to use the Runescape 3 plugin loader. Also updated the mainProgram as well as runScript so that the program can be ran from nix run instead of just being able to be ran manually. Add feature flags for RS3 and HDOSAndRuneLite as well as .desktop and icon file support (#1) * Add enable flags for RS3 and HDOSAndRuneLite * Add .desktop and icon * Fix formatting w/ nixfmt * Remove enableHDOSAndRuneLite feature flag * Fix formatting Co-authored-by: Thomas King <[email protected]>
This PR adopts @Adamcake 's approach to optional dependencies used in the AUR package. This avoids having a compromised library as a requirement.
This PR also adds support for creating and linking/copying a .desktop file and corresponding icon. This has the effect of causing the package to show up in app launchers when added to a system.
Summary by CodeRabbit
New Features
Improvements