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

nexusmods-app: refactor the unfree variant #331355

Merged
merged 4 commits into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 90 additions & 60 deletions pkgs/by-name/ne/nexusmods-app/package.nix
Original file line number Diff line number Diff line change
@@ -1,26 +1,21 @@
{ _7zz
, buildDotnetModule
, copyDesktopItems
, desktop-file-utils
, dotnetCorePackages
, fetchFromGitHub
, fontconfig
, lib
, libICE
, libSM
, libX11
, nexusmods-app
, runCommand
, enableUnfree ? false # Set to true to support RAR format mods
{
_7zz,
buildDotnetModule,
copyDesktopItems,
desktop-file-utils,
dotnetCorePackages,
fetchFromGitHub,
fontconfig,
lib,
libICE,
libSM,
libX11,
nexusmods-app,
runCommand,
pname ? "nexusmods-app",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MattSturgeon MattSturgeon Aug 1, 2024

Choose a reason for hiding this comment

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

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 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}:
let
_7zzWithOptionalUnfreeRarSupport = _7zz.override {
inherit enableUnfree;
};
in
buildDotnetModule rec {
pname = "nexusmods-app";

inherit pname;
version = "0.4.1";

src = fetchFromGitHub {
Expand All @@ -39,9 +34,7 @@ buildDotnetModule rec {
projectFile = "src/NexusMods.App/NexusMods.App.csproj";
testProjectFile = "NexusMods.App.sln";

nativeBuildInputs = [
copyDesktopItems
];
nativeBuildInputs = [ copyDesktopItems ];

nugetDeps = ./deps.nix;

Expand All @@ -54,15 +47,15 @@ buildDotnetModule rec {
'';

postPatch = ''
ln --force --symbolic "${lib.getExe _7zzWithOptionalUnfreeRarSupport}" src/ArchiveManagement/NexusMods.FileExtractor/runtimes/linux-x64/native/7zz
ln --force --symbolic "${lib.getExe _7zz}" src/ArchiveManagement/NexusMods.FileExtractor/runtimes/linux-x64/native/7zz

# for some reason these tests fail (intermittently?) with a zero timestamp
touch tests/NexusMods.UI.Tests/WorkspaceSystem/*.verified.png
'';

makeWrapperArgs = [
"--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
MattSturgeon marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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

];

runtimeDeps = [
Expand All @@ -72,52 +65,62 @@ buildDotnetModule rec {
libX11
];

executables = [
nexusmods-app.meta.mainProgram
];
executables = [ meta.mainProgram ];
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SuperSandro2000

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


doCheck = true;

dotnetTestFlags = [
"--environment=USER=nobody"
(lib.strings.concatStrings [
(
"--filter="
(lib.strings.concatStrings (lib.strings.intersperse "&" ([
"Category!=Disabled"
"FlakeyTest!=True"
"RequiresNetworking!=True"
"FullyQualifiedName!=NexusMods.UI.Tests.ImageCacheTests.Test_LoadAndCache_RemoteImage"
"FullyQualifiedName!=NexusMods.UI.Tests.ImageCacheTests.Test_LoadAndCache_ImageStoredFile"
] ++ lib.optionals (! enableUnfree) [
"FullyQualifiedName!=NexusMods.Games.FOMOD.Tests.FomodXmlInstallerTests.InstallsFilesSimple_UsingRar"
])))
])
+ lib.strings.concatStringsSep "&" (
[
"Category!=Disabled"
"FlakeyTest!=True"
"RequiresNetworking!=True"
"FullyQualifiedName!=NexusMods.UI.Tests.ImageCacheTests.Test_LoadAndCache_RemoteImage"
"FullyQualifiedName!=NexusMods.UI.Tests.ImageCacheTests.Test_LoadAndCache_ImageStoredFile"
]
++ lib.optionals (!_7zz.meta.unfree) [
"FullyQualifiedName!=NexusMods.Games.FOMOD.Tests.FomodXmlInstallerTests.InstallsFilesSimple_UsingRar"
]
)
)
];

passthru = {
tests = {
serve = runCommand "${pname}-test-serve" { } ''
${nexusmods-app}/bin/${nexusmods-app.meta.mainProgram}
touch $out
'';
help = runCommand "${pname}-test-help" { } ''
${nexusmods-app}/bin/${nexusmods-app.meta.mainProgram} --help
touch $out
'';
associate-nxm = runCommand "${pname}-test-associate-nxm" { } ''
${nexusmods-app}/bin/${nexusmods-app.meta.mainProgram} associate-nxm
touch $out
'';
list-tools = runCommand "${pname}-test-list-tools" { } ''
${nexusmods-app}/bin/${nexusmods-app.meta.mainProgram} list-tools
touch $out
'';
};
tests =
let
runTest =
name: script:
runCommand "${pname}-test-${name}"
{
# TODO: use finalAttrs when buildDotnetModule has support
nativeBuildInputs = [ nexusmods-app ];
}
''
${script}
touch $out
'';
in
{
serve = runTest "serve" ''
NexusMods.App
'';
help = runTest "help" ''
NexusMods.App --help
'';
associate-nxm = runTest "associate-nxm" ''
NexusMods.App associate-nxm
'';
list-tools = runTest "list-tools" ''
NexusMods.App list-tools
'';
};
updateScript = ./update.bash;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


meta = {
description = "Game mod installer, creator and manager";
mainProgram = "NexusMods.App";
homepage = "https://github.com/Nexus-Mods/NexusMods.App";
changelog = "https://github.com/Nexus-Mods/NexusMods.App/releases/tag/${src.rev}";
Expand All @@ -127,5 +130,32 @@ buildDotnetModule rec {
MattSturgeon
];
platforms = lib.platforms.linux;
description = "Game mod installer, creator and manager";
longDescription = ''
A mod installer, creator and manager for all your popular games.

Currently experimental and undergoing active development,
new releases may include breaking changes!

${
if _7zz.meta.unfree then
''
This "unfree" variant includes support for mods packaged as RAR archives.
''
else
''
It is strongly recommended that you use the "unfree" variant of this package,
which provides support for mods packaged as RAR archives.

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

```nix
pkgs.nexusmods-app.override {
_7zz = pkgs._7zz-rar;
}
```
Comment on lines +149 to +156
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

''
}
'';
MattSturgeon marked this conversation as resolved.
Show resolved Hide resolved
};
}
5 changes: 3 additions & 2 deletions pkgs/top-level/all-packages.nix
Original file line number Diff line number Diff line change
Expand Up @@ -18792,8 +18792,9 @@ with pkgs;

nix-build-uncached = callPackage ../development/tools/misc/nix-build-uncached { };

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

nmrpflash = callPackage ../development/embedded/nmrpflash { };
Expand Down