nexusmods-app: refactor the unfree variant #331355

MattSturgeon · 2024-07-31T19:25:00Z

Description of changes

Reformat with nixfmt
Cherry picked some fixes by @corngood.
If dotnet: use unpacked packages in store #327651 is merged before this, they can be dropped in a rebase.
Removed the enableUnfree argument; instead users should override the _7zz argument.
Refactor nexusmods-app-unfree's implementation to use override instead of callPackage
Set nexusmods-app-unfree's pname using overrideAttrs
Add a long description, which varies depending on _7zz.meta.unfree
- This attempts to warn users that the free variant doesn't support RAR archive mods
- It also documents how to enable RAR support

Merging this early should hopefully make #331150 easier to work on and review.

See also: #327651 #331150
Closes #321405

cc @l0b0 @corngood @GGG-KILLER @erri120

Things done

Add a 👍 reaction to pull requests you find important.

pkgs/by-name/ne/nexusmods-app/package.nix

MattSturgeon · 2024-07-31T19:32:28Z

pkgs/by-name/ne/nexusmods-app/package.nix

@@ -108,7 +112,6 @@ buildDotnetModule rec {
  };


How should the tests reference the package's exe without explicitly referencing pkgs.nexusmods-app?

Presumably this may use the wrong derivation when we're building an overridden package?

Seems most other packages just set nativeBuildInputs = [ nexusmods-app ] in the test derivations.

Maybe the -unfree override in all-packages.nix should also be overriding the nexusmods-app argument passed in?

I'm going to look at these examples using the finalAttrs pattern tomorrow if I get chance

Sorry, I responded to the other thread pretty much saying what you said here.

corngood · 2024-07-31T23:38:22Z

Sorry for the conflict, but it's trivial and you should be able to rebase now to drop the change you cherry-picked.

corngood · 2024-08-01T01:18:03Z

Removed the enableUnfree argument; instead users should override the _7zz argument.

What's the rationale for this? It feels like it leaks a bunch of details about what makes the package free vs. unfree into all-packages.nix. enableUnfree seems to be used in a few other places.

MattSturgeon · 2024-08-01T11:51:57Z

Removed the enableUnfree argument; instead users should override the _7zz argument.

What's the rationale for this? It feels like it leaks a bunch of details about what makes the package free vs. unfree into all-packages.nix. enableUnfree seems to be used in a few other places.

My thought process was that it's not really the nexusmods-app that is free/unfree, but rather its dependency.

I haven't looked too hard, but I assume enableUnfree is normally used when the package itself has an optional unfree feature or multiple optional unfree dependencies?

My assumption was that it'd be better to remove the abstraction, exposing the details to end-users who wanted to override the package themselves.

I'm happy to add it back, since it is probably a simpler interface for end-users. Although saying that, nexusmods-app-unfree itself is intended to be the "simple" interface.

While I've been using the nix language and module systems for a bit, this is my first time packaging something, so I appreciate all the feedback and guidance I can get!

If we add the enableUnfree argument back, should it be done in such a way that users can still provide an unfree _7zz without us then overriding its enableUnfree argument?

e.g. pkgs.nexusmods-app.override { _7zz = pkgs._7zz.override { enableUnfree = true; }; } wouldn't work without this PR, because nexusmods-app's enableUnfree argument would internally override _7zz's back to false.

corngood · 2024-08-01T12:13:25Z

What if we did:

  nexusmods-app-unfree = callPackage ../by-name/ne/nexusmods-app/package.nix {
    pname = "nexusmods-app-unfree";
    _7zz = _7zz.override { enableUnfree = true; };
  };

and add pname ? "nexusmods-app" to the package?

I don't really have a strong opinion about this. There are various examples that support any of these methods:

  openocd-rp2040 = openocd.overrideAttrs (old: {
    pname = "openocd-rp2040";
    src = fetchFromGitHub {
      owner = "raspberrypi";
      repo = "openocd";
      rev = "4d87f6dcae77d3cbcd8ac3f7dc887adf46ffa504";
      hash = "sha256-bBqVoHsnNoaC2t8hqcduI8GGlO0VDMUovCB0HC+rxvc=";
      # openocd disables the vendored libraries that use submodules and replaces them with nix versions.
      # this works out as one of the submodule sources seems to be flakey.
      fetchSubmodules = false;
    };
    nativeBuildInputs = old.nativeBuildInputs ++ [
      autoreconfHook
    ];
  });

  firefox-bin = wrapFirefox firefox-bin-unwrapped {
    pname = "firefox-bin";
  };

A couple of other minor things:

I think we can drop pkgs. since there's a with pkgs; at the top
Should we use _7zz-rar instead of doing the override? If rar support is what we're looking for, then I think it matches the intent better. Imagine there's an alternate implementation of 7zz with rar support in the future.

MattSturgeon · 2024-08-01T12:43:23Z

What if we [used callPackage] and add pname ? "nexusmods-app" to the package?

I considered adding a pname argument to the package but wasn't sure if that was the "done thing" instead of using overrideAttrs. I agree that chaining both override and overrideAttrs is kinda messy though!

Is it better to use callPackage or override for package variants? I'd assumed the latter was preferred.

I think we can drop pkgs. since there's a with pkgs; at the top

Thanks, I didn't spot that!

Should we use _7zz-rar instead of doing the override? If rar support is what we're looking for, then I think it matches the intent better. Imagine there's an alternate implementation of 7zz with rar support in the future.

Yes, I think so. I don't think either me or @l0b0 spotted that package when initially adding nexusmods-app.

Should nexusmods-app-unfree be renamed nexusmods-app-rar in line with that? We probably wouldn't need an alias for very long given how new the package is.

MattSturgeon · 2024-08-01T12:53:59Z

Looking at _7zz and _7zz-rar, both have the same pname; maybe it isn't important to change the pname for the nexusmods-app variant that has RAR support?

https://github.com/nixos/nixpkgs/blob/master/pkgs/top-level/all-packages.nix#L1466-L1467

corngood · 2024-08-01T13:30:25Z

Should nexusmods-app-unfree be renamed nexusmods-app-rar in line with that? We probably wouldn't need an alias for very long given how new the package is.

I think probably not. It only really exists as a variant because there's no free way to support rar. In the event that that changes, we'd probably just deprecate -unfree.

I considered adding a pname argument to the package but wasn't sure if that was the "done thing" instead of using overrideAttrs

One minor argument against overrideAttrs is that if the package ever switched to use name instead of pname, the override would have no effect. Having it in the package args is a hint that it's overridden. There are a few examples around if you search for pname ? ", e.g. pname ? "gnuradio".

maybe it isn't important to change the pname for the nexusmods-app variant that has RAR support?

I think this could also be fine.

Is it better to use callPackage or override for package variants? I'd assumed the latter was preferred.

I would have actually thought the former, because of how splicing works in callPackage. Don't take my word for it though, because I don't fully understand all of the details. Also, the differences are probably not a practical concern for this package.

There's so much variety in all-packages that realistically any of these options are probably okay.

MattSturgeon · 2024-08-01T14:56:56Z

@corngood thanks for your prompt reviews and helpful feedback!

MattSturgeon · 2024-08-01T18:28:03Z

I noticed the tests are waiting to run on darwin, however this package shouldn't be available on darwin since upstream don't (yet) support it.

The package does define meta.platforms = lib.platforms.linux.

Is that a packaging issue on my end, or do the tests always queue for all systems?

corngood · 2024-08-01T20:19:16Z

Is that a packaging issue on my end, or do the tests always queue for all systems?

I'm not 100% sure, but my guess is it's not fully checking metadata before queueing and will just end up doing nothing.

l0b0 · 2024-08-01T20:06:11Z

pkgs/by-name/ne/nexusmods-app/package.nix

+  libX11,
+  nexusmods-app,
+  runCommand,
+  pname ? "nexusmods-app",


Why would I want to optionally change the package name to an arbitrary string, when in reality there's only two useful package names? I'm pretty sure the original pattern of using enableUnfree (or something like it) is how it's done elsewhere. Having to override _7zz is much more technically difficult than setting enableUnfree = true. And finally, enableUnfree means we can easily add more optional unfree components if necessary without changing the function signature.

Regarding pname as a package argument: that was discussed in a few comments above, mostly from here onwards: #331355 (comment)

Regarding the motivation for dropping enableUnfree, see this comment: #331355 (comment)

Having to override _7zz is much more technically difficult than setting enableUnfree = true.

I don't agree that _7zz = pkgs._7zz-rar is significantly more difficult than enableUnfree = true.

I also prefer how the former is intrinsically clear about its purpose.

At first glance, the later could be interpreted as any number of unfree things being enabled.

And finally, enableUnfree means we can easily add more optional unfree components if necessary without changing the function signature.

Maybe I'll eat my words, but I don't see that as a likely scenario. Nexusmods-app is committed to being an open source project with open source dependencies.

It is just unfortunate that one optional feature of one dependency currently requires an unfree license.

If there were multiple unfree features to be enabled, then an enableUnfree argument would be more useful, I agree. If that happens we can revisit this 😀

I also prefer how the former is intrinsically clear about its purpose.

I prefer how the latter is more clear about the thing I care about. But of course people care about different things. 😁

I still prefer the original design, but I'll let other reviewers decide.

pkgs/by-name/ne/nexusmods-app/package.nix

- Format using nixfmt - Apply other minor formatting tweaks - Simplify `--filter` test flag definition

We still need to fix the tests to use `finalAttrs`, but that requires changes to `buildDotnetModule`.

Use `override` to pass a `_7zz` with RAR support to the package, instead of having an `enableUnfree` package argument. Use a different `pname` on the unfree package. Use `_7zz.meta.unfree` internally.

pkgs/by-name/ne/nexusmods-app/package.nix

SuperSandro2000 · 2024-08-03T17:25:34Z

pkgs/by-name/ne/nexusmods-app/package.nix

+
+            You can also enable unrar support manually, by overriding the `_7zz` used:
+
+            ```nix
+            pkgs.nexusmods-app.override {
+              _7zz = pkgs._7zz-rar;
+            }
+            ```


Suggested change

You can also enable unrar support manually, by overriding the `_7zz` used:

```nix

pkgs.nexusmods-app.override {

_7zz = pkgs._7zz-rar;

}

```

What's the point off this?

More documentation is usually better, right? It sheds some light on what the "unfree" version is actually doing and empowers users to customize the package to their liking.

If you still think this is too verbose for the longDescription, at least we still have the paragraph above regarding nexusmods-app-unfree.

SuperSandro2000 · 2024-08-03T17:26:33Z

pkgs/by-name/ne/nexusmods-app/package.nix

-  executables = [
-    nexusmods-app.meta.mainProgram
-  ];
+  executables = [ meta.mainProgram ];


Suggested change

executables = [ meta.mainProgram ];

executables = [ "NexusMods.App" ];

Thanks for the feedback, but could you clarify the motivation?

When I discussed this earlier (#331355 (comment)) it was thought that using meta.mainProgram via rec or finalAttrs would be fine.

@SuperSandro2000

You asked for the same thing in #331150 (comment), but never explained your reasoning.

SuperSandro2000 · 2024-08-03T17:26:42Z

pkgs/by-name/ne/nexusmods-app/package.nix

-    "--prefix PATH : ${lib.makeBinPath [desktop-file-utils]}"
-    "--set APPIMAGE $out/bin/${meta.mainProgram}" # Make associating with nxm links work on Linux
+    "--prefix PATH : ${lib.makeBinPath [ desktop-file-utils ]}"
+    "--set APPIMAGE ${placeholder "out"}/bin/${meta.mainProgram}" # Make associating with nxm links work on Linux


Suggested change

"--set APPIMAGE ${placeholder "out"}/bin/${meta.mainProgram}" # Make associating with nxm links work on Linux

"--set APPIMAGE ${placeholder "out"}/bin/NexusMods.App" # Make associating with nxm links work on Linux

MattSturgeon · 2024-08-05T18:45:46Z

Is that a packaging issue on my end, or do the tests always queue for all systems?

I'm not 100% sure, but my guess is it's not fully checking metadata before queueing and will just end up doing nothing.

Maybe we can use meta.hydraPlatforms for this 🤔

EDIT: Nevermind, to quote the docs:

meta.hydraPlatforms defaults to the value of meta.platforms. Thus, the only reason to set it is if you want hydra.nixos.org to build the package on a subset of meta.platforms, or not at all.

MattSturgeon · 2024-08-14T09:42:13Z

I'm conscious that the nexus team plan to have another release out in a week or so, it'd be nice to get cleanup and refactoring out of the way so that we can refocus on getting the package updated and working.

Other than taking advantage of finalAttrs attr fixing that buildDotnetModule recently gained support for, how would you recommend we go about addressing the feedback so that we can move forward?

Should I adapt this PR to use finalAttrs-style arguments? I guess that may resolve some of @SuperSandro2000's feedback 🤔

@corngood @l0b0 @SuperSandro2000

corngood · 2024-08-14T12:29:11Z

You've posted reasonable responses to all the comments and there have been no responses, so I'm happy to merge this when you think it's ready.

Should I adapt this PR to use finalAttrs-style arguments?

Your call. I don't mind if it's done separately. omnisharp-roslyn also needs a fix.

Add a long description that documents whether or not RAR archive mods are supported and how to enable support.

MattSturgeon · 2024-08-14T13:43:54Z

You've posted reasonable responses to all the comments and there have been no responses, so I'm happy to merge this when you think it's ready.

Should I adapt this PR to use finalAttrs-style arguments?

Your call. I don't mind if it's done separately. omnisharp-roslyn also needs a fix.

I've applied the changes from #331355 (comment), but otherwise I think the feedback is either personal preferences, or best addressed while fixing the recursive definitions to use finalAttrs.

I think that's best done in a fresh PR, so if you're happy, I'm happy. Let's get this merged!

corngood · 2024-08-14T14:22:12Z

Result of nixpkgs-review pr 331355 run on x86_64-linux 1

2 packages built:

nexusmods-app
nexusmods-app-unfree

MattSturgeon requested review from l0b0, corngood, GGG-KILLER and SuperSandro2000 July 31, 2024 19:25

MattSturgeon mentioned this pull request Jul 31, 2024

nexusmods-app: 0.4.1 -> 0.6.1 #331150

Merged

20 tasks

MattSturgeon commented Jul 31, 2024

View reviewed changes

pkgs/by-name/ne/nexusmods-app/package.nix Outdated Show resolved Hide resolved

MattSturgeon commented Jul 31, 2024

View reviewed changes

ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 31, 2024

MattSturgeon force-pushed the nexus-refactor-unfree branch from 90a4960 to 8b35bbb Compare July 31, 2024 23:38

MattSturgeon mentioned this pull request Aug 1, 2024

buildDotnetModule: add finalAttrs support #331398

Merged

13 tasks

ofborg bot added the 10.rebuild-linux: 1 label Aug 1, 2024

MattSturgeon force-pushed the nexus-refactor-unfree branch 2 times, most recently from bbe7f8f to 05ee324 Compare August 1, 2024 01:06

ofborg bot removed the 10.rebuild-linux: 1 label Aug 1, 2024

MattSturgeon force-pushed the nexus-refactor-unfree branch from 05ee324 to e4f0cf7 Compare August 1, 2024 14:32

corngood approved these changes Aug 1, 2024

View reviewed changes

l0b0 reviewed Aug 1, 2024

View reviewed changes

nexusmods-app: reformat

7166d52

- Format using nixfmt - Apply other minor formatting tweaks - Simplify `--filter` test flag definition

MattSturgeon added 2 commits August 2, 2024 15:22

nexusmods-app: cleanup tests & mainProgram refs

e54e9fa

We still need to fix the tests to use `finalAttrs`, but that requires changes to `buildDotnetModule`.

nexusmods-app: refactor "unfree" variant

1148136

Use `override` to pass a `_7zz` with RAR support to the package, instead of having an `enableUnfree` package argument. Use a different `pname` on the unfree package. Use `_7zz.meta.unfree` internally.

MattSturgeon force-pushed the nexus-refactor-unfree branch from e4f0cf7 to 79d0527 Compare August 2, 2024 14:25

ofborg bot requested a review from l0b0 August 2, 2024 15:05

SuperSandro2000 reviewed Aug 3, 2024

View reviewed changes

pkgs/by-name/ne/nexusmods-app/package.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed Aug 3, 2024

View reviewed changes

nexusmods-app: add long description

07db828

Add a long description that documents whether or not RAR archive mods are supported and how to enable support.

MattSturgeon force-pushed the nexus-refactor-unfree branch from 6190a7d to 07db828 Compare August 14, 2024 13:39

corngood merged commit 5088777 into NixOS:master Aug 14, 2024
35 of 38 checks passed

MattSturgeon deleted the nexus-refactor-unfree branch August 14, 2024 14:59

MattSturgeon mentioned this pull request Aug 21, 2024

nexusmods-app: use finalAttrs #336221

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nexusmods-app: refactor the unfree variant #331355

nexusmods-app: refactor the unfree variant #331355

MattSturgeon commented Jul 31, 2024 •

edited by corngood

Loading

MattSturgeon Jul 31, 2024

MattSturgeon Jul 31, 2024

MattSturgeon Jul 31, 2024

corngood Jul 31, 2024

corngood commented Jul 31, 2024

corngood commented Aug 1, 2024 •

edited

Loading

MattSturgeon commented Aug 1, 2024

corngood commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024 •

edited

Loading

corngood commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024

corngood commented Aug 1, 2024

l0b0 Aug 1, 2024

MattSturgeon Aug 1, 2024 •

edited

Loading

l0b0 Aug 1, 2024

SuperSandro2000 Aug 3, 2024

MattSturgeon Aug 3, 2024

SuperSandro2000 Aug 3, 2024

MattSturgeon Aug 3, 2024

corngood Aug 3, 2024

SuperSandro2000 Aug 3, 2024

MattSturgeon commented Aug 5, 2024 •

edited

Loading

MattSturgeon commented Aug 14, 2024

corngood commented Aug 14, 2024

MattSturgeon commented Aug 14, 2024

corngood commented Aug 14, 2024

	executables = [ meta.mainProgram ];
	executables = [ "NexusMods.App" ];

	"--set APPIMAGE ${placeholder "out"}/bin/${meta.mainProgram}" # Make associating with nxm links work on Linux
	"--set APPIMAGE ${placeholder "out"}/bin/NexusMods.App" # Make associating with nxm links work on Linux

nexusmods-app: refactor the unfree variant #331355

nexusmods-app: refactor the unfree variant #331355

Conversation

MattSturgeon commented Jul 31, 2024 • edited by corngood Loading

Description of changes

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corngood commented Jul 31, 2024

corngood commented Aug 1, 2024 • edited Loading

MattSturgeon commented Aug 1, 2024

corngood commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024 • edited Loading

corngood commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024

MattSturgeon commented Aug 1, 2024

corngood commented Aug 1, 2024

Choose a reason for hiding this comment

MattSturgeon Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattSturgeon commented Aug 5, 2024 • edited Loading

MattSturgeon commented Aug 14, 2024

corngood commented Aug 14, 2024

MattSturgeon commented Aug 14, 2024

corngood commented Aug 14, 2024

MattSturgeon commented Jul 31, 2024 •

edited by corngood

Loading

corngood commented Aug 1, 2024 •

edited

Loading

MattSturgeon commented Aug 1, 2024 •

edited

Loading

MattSturgeon Aug 1, 2024 •

edited

Loading

MattSturgeon commented Aug 5, 2024 •

edited

Loading