Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update umu-launcher: add attrs extraLibraries & extraPkgs #375423

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

LovingMelody
Copy link
Contributor

Add extraLibraries & extraPkgs to package attrs to allow passing graphical libs. This is needed for dxvk-nvapi to work on NVIDIA cards.

Example from Steam of this being used:

extraLibraries = pkgs: let
prevLibs = if prev ? extraLibraries then prev.extraLibraries pkgs else [ ];
additionalLibs = with config.hardware.graphics;
if pkgs.stdenv.hostPlatform.is64bit
then [ package ] ++ extraPackages
else [ package32 ] ++ extraPackages32;
in prevLibs ++ additionalLibs;

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • [] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 21, 2025
@MattSturgeon

This comment was marked as resolved.

Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues from me

pkgs/by-name/um/umu-launcher/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/um/umu-launcher/package.nix Outdated Show resolved Hide resolved
@diniamo
Copy link
Contributor

diniamo commented Jan 21, 2025

@diniamo please don't 👎 without leaving a review explaining your position

I did, twice, on the original PR. Both times it was ignored, so I see no point, but let's do it anyway then.

This makes sense for game launchers, because games may require arbitrary libraries to work. But that isn't the case for Windows games, since every dependency is either bundled in wine, or can only be installed in the wine prefix imperatively. Now considering that, and the fact that umu-launcher can only launch Windows games, we get that we wouldn't normally need extra packages in the FHS env. If for some reason we do anyway, then that can be added as a withSomething option (this makes sense for the nvidia dxvk thing), or using overrideAttrs (this makes sense for the cursor fix, since it's a temporary workaround (?)).

@MattSturgeon
Copy link
Contributor

MattSturgeon commented Jan 21, 2025

I did, twice, on the original PR. Both times it was ignored

Sometimes it's easier to have these discussions on dedicated issues/PRs (like this one). My interpretation of this thread wasn't that it was ignored, but rather that discussion was postponed until after that PR was done.

Now considering that, and the fact that umu-launcher can only launch Windows games, we get that we wouldn't normally need extra packages in the FHS env.

Makes sense, sure.

If for some reason we do anyway,

However, from a pragmatic perspective, if we occasionally need extra packages anyway, why not make it easier for users to specify them?

I don't see any downsides to having the code, even if it is only occasionally useful.

then that can be added as a withSomething option (this makes sense for the nvidia dxvk thing)

Sure, if there's well known common work-arounds they could be added as withSomething or enableSomething options. I don't see that as being mutually exclusive.

@diniamo
Copy link
Contributor

diniamo commented Jan 21, 2025

if we occasionally need extra packages anyway

This is the exact reason overrideAttrs exists.

@LovingMelody
Copy link
Contributor Author

@diniamo please don't 👎 without leaving a review explaining your position

I did, twice, on the original PR. Both times it was ignored, so I see no point, but let's do it anyway then.

This makes sense for game launchers, because games may require arbitrary libraries to work. But that isn't the case for Windows games, since every dependency is either bundled in wine, or can only be installed in the wine prefix imperatively. Now considering that, and the fact that umu-launcher can only launch Windows games, we get that we wouldn't normally need extra packages in the FHS env. If for some reason we do anyway, then that can be added as a withSomething option (this makes sense for the nvidia dxvk thing), or using overrideAttrs (this makes sense for the cursor fix, since it's a temporary workaround (?)).

Nvidia can't be added as a withSomething flag because it has to be the library from your driver version. The only way to get this is to get it from config, I wish this wasn't the case, but it is.
There is an open issue for it #177533, but this is what the standard practice is now. Steam, heroic, lutris also do it this way. As mentioned in the original pr, Steam uses this to pass graphical libraries for the user when programs.steam.enable is used. This isn’t specifically for Linux native games.

@MattSturgeon
Copy link
Contributor

MattSturgeon commented Jan 21, 2025

if we occasionally need extra packages anyway

This is the exact reason overrideAttrs exists.

For the record, overrideAttrs isn't helpful in this case.

targetPkgs isn't a derivation attr, it's an argument to buildFHSEnv. Therefore it can't easily be overridden.

MattSturgeon

This comment was marked as resolved.

@LovingMelody
Copy link
Contributor Author

For the record, needs to be in the fhs environment for dxvk-nvapi to locate it.

Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Comment on lines +12 to +18
targetPkgs =
pkgs:
[
# Use umu-launcher-unwrapped from the package args, to support overriding
umu-launcher-unwrapped
]
++ extraPkgs pkgs;
Copy link
Contributor

@MattSturgeon MattSturgeon Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow nixfmt has made that ugly 💥 🚒

IDK if this is still the case, but it used to format things nicer if you made the list-literal the last part of the expression:

Suggested change
targetPkgs =
pkgs:
[
# Use umu-launcher-unwrapped from the package args, to support overriding
umu-launcher-unwrapped
]
++ extraPkgs pkgs;
targetPkgs = pkgs: extraPkgs pkgs ++ [
# Use umu-launcher-unwrapped from the package args, to support overriding
umu-launcher-unwrapped
];

No big deal, either way 🙂

@MattSturgeon MattSturgeon added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jan 25, 2025
@khaneliman khaneliman merged commit e224f1b into NixOS:master Jan 25, 2025
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants