-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: 0.4.1 -> 0.6.0 #331150
base: master
Are you sure you want to change the base?
nexusmods-app: 0.4.1 -> 0.6.0 #331150
Conversation
touch $out | ||
''; | ||
list-tools = runCommand "${pname}-test-list-tools" { } '' | ||
${nexusmods-app}/bin/${nexusmods-app.meta.mainProgram} list-tools | ||
${nexusmods-app}/bin/${meta.mainProgram} list-tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metaMainprogram should not really be used like this. It falls under the same category as pname. Just write the bvinary name out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind doing this, but could you clarify the reason why this is bad practice?
From my perspective using meta.mainProgram
here is DRY-er. Personally I'd go one step further and use lib.getExe
, which does the same thing under the hood.
38ad575
to
8f105b9
Compare
803f31a
to
d365cd6
Compare
The tests pass if I keep the patch that replaces the bundled 7zz with ours. This implies that maybe Either that, or I'm not setting the Or perhaps the wrapper PATH isn't used when running the tests? I still can't start the GUI, but the CLI commands seem to work. I haven't looked into patching/copying the |
d365cd6
to
a2c9f64
Compare
a2c9f64
to
74b8ede
Compare
I'm able to build, and I can run CLI commands such as I get no errors on stdout/stderr, however logs are printed to
Full log: nexusmods.app.main.current.log There's other errors in the log relating to XDG stuff, however the big issue seems to be GUI resources being missing:
I guess it could relate to not using the full sln here, however 0.4.1 seems to work fine even with that change... nixpkgs/pkgs/by-name/ne/nexusmods-app/package.nix Lines 29 to 35 in 74b8ede
|
I got some time to kill. I'm not sure about the rest yet, but I can at least try fix up |
This should fix the 7z issue upstream. |
[ | ||
# From https://github.com/Nexus-Mods/NexusMods.App/blob/v0.5.3/src/NexusMods.App/app.pupnet.conf#L38 | ||
"--property:Version=${version}" | ||
"--property:TieredCompilation=true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also want the ReadyToRun
flag here; it improves startup times, especially in something as JIT heavy at startup as NMA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed ReadyToRun because we didn't have crossgen2 support in nix before, I believe after the changes by @corngood that might've been solved though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note @GGG-KILLER previously advised against this (#318647 (comment)):
This makes it so that the code is pre-compiled to the target runtime, given that we don't add the framework's RID unless it's a self-contained build, I don't recommend setting this.
...but the nexus folks claim it has some performance benefits.
I'm not sure what is meant by "framework's RID" so I'm not in a position to make an educated decision.
I think, per your own comment Nexus-Mods/NexusMods.App#1866 (comment) adding ICU might be recommended here; if it's not transitively available already. |
As for the cause of the issue; I'm not really sure myself. Normally this sort of thing can happen if you build an Avalonia .NET package with trimming (Or NativeAOT, which requires trimming). If some assembly isn't correctly marked for trimming, or excluded from the 'trimmer', this sort of thing can happen. I think instead of looking at runtime errors, it may be better to grab a log of the
|
74b8ede
to
cc5e82d
Compare
dotnetBuildFlags = | ||
let | ||
# From https://nexus-mods.github.io/NexusMods.App/developers/Contributing/#for-package-maintainers | ||
# TODO: add DefineConstants support to buildDotnetModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel like we need to specially add support for this when we already have a general build flag input.
From my experience it's rare to see any programs using DefineConstant to configure how they're built, usually they use MSBuild properties which is the idiomatic way to enable and disable things in the .NET world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually they use MSBuild properties which is the idiomatic way to enable and disable things in the .NET world.
The upstream project is still in a somewhat "experimental" state, so perhaps they'd be willing to migrate to a more idiomatic method then.
I'm somewhat unfamiliar with dotnet, so this is a useful insight. I'll have to go read some docs 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to avoid build constants in the actual App as much as possible.
They're only really being used when it's desirable for package maintainers to have slightly different default runtime behaviour. Everything else is configurable at runtime.
For instance, for AppImage & Package Manager builds we add an extra message telling users to update using the System, rather than via the App. For manual installs we don't show update dialogs on the other hand.
It is possible to assign a MSBuild property other than DefineConstants
here, or something like AdditionalDefineConstants
that would just append to the existing constants. However at the end of the day you'll always be making some minor change. Be it adding a few characters to the build command, dropping a file, or replacing an enum value in the source to set some default behaviour at compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that, usually, what projects do is have some msbuild property that then defines the constant, something like this in the .csproj
:
<PropertyGroup Condition="$(UseSystemExtractor) == '1'">
<DefineConstants>$(DefineConstants);NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR</DefineConstants>
</PropertyGroup>
and then when building you'd do dotnet build -c Release -p:UseSystemExtractor=1
.
So I don't know if it's worth adding DefineConstants
support in buildDotnetModule
for something that only one app will use most likely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I've done it myself this way before too. I'd be happy to provide a constant like this upstream if the others there wouldn't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need to change this now unless if there's an actual need for it, as it's been done like this already it'll just generate CI churn and problems for other packagers.
I'm just speaking on behalf of the .NET team in NixOS with relation to adding a builtin option for doing this, there's nothing inherently wrong with the way this has been done upstream, it's just not conventional so I think there's other things we can direct our effort towards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a need to change this now unless if there's an actual need for it, as it's been done like this already it'll just generate CI churn and problems for other packagers.
I think it's still early enough in nexusmods-app's life that it shouldn't really be an issue for other downstream packagers.
Some of the issues @Sewer56 encountered while investigating Nexus-Mods/NexusMods.App#1919 sound like they stemmed from us explicitly passing DefineConstants
on the CLI. Long-term it may be better to have custom property groups, if so.
It's certainly easier to pass many "-p:{}"
flags instead of joining a list of constants together using "%3B"
(escaped ";"
)...
But I don't want to over-blow the issue; we can work with whichever implementation upstream prefers 😀
[ | ||
# From https://github.com/Nexus-Mods/NexusMods.App/blob/v0.5.3/src/NexusMods.App/app.pupnet.conf#L38 | ||
"--property:Version=${version}" | ||
"--property:TieredCompilation=true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed ReadyToRun because we didn't have crossgen2 support in nix before, I believe after the changes by @corngood that might've been solved though.
@Sewer56 I'm looking at the build logs currently, and it seems like it's processing the XAML files:
I've also found the following line in the build logs but it seems unrelated:
This is the full log if anyone would like to take a look at it: https://gist.github.com/GGG-KILLER/1cd7370246aa02b6bc27eefc694543d0 I also don't get any errors when running the program but the UI still doesn't show up: |
|
||
runtimeInputs = [ | ||
_7zz | ||
desktop-file-utils | ||
]; | ||
|
||
runtimeDeps = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replying to #331150 (comment) by @Sewer56:
I think, per your own comment Nexus-Mods/NexusMods.App#1866 (comment) adding ICU might be recommended here; if it's not transitively available already.
It looks like ICU is made available at runtime on the LD_LIBRARY_PATH
by buildDotnetModule:
nixpkgs/pkgs/build-support/dotnet/build-dotnet-module/default.nix
Lines 187 to 192 in 8fe18a3
makeWrapperArgs = args.makeWrapperArgs or [ ] ++ [ | |
"--prefix" | |
"LD_LIBRARY_PATH" | |
":" | |
"${dotnet-sdk.icu}/lib" | |
]; |
Doesn't look like it's made available at build time though (e.g. via buildInputs
or nativeBuildInputs
), but that might be done elsewhere 🤔
e34f505
to
7075251
Compare
I noticed that even when installing the package persistently (e.g. using No logs are produced either. Running it from a terminal works as expected. I haven't had chance to look into this properly, and based on #318567 this may not be a new issue. I wonder if anyone has any ideas though? |
Looks like the .desktop entry that's being created is pointing to an unwrapped dotnet app.
During bisect steps I also glanced over nexusmods-app source code and noticed that it has built-in .desktop generation functionality, that runs on app startup automatically. So it may be the case that correct desktop entry from build time is overridden with incorrect one during startup. |
It seems like it's pointing to the wrong binary, it should be pointing to the one inside
Maybe this is another thing we'll have to patch, it's worth investigating how it detects which path it puts in the desktop entries though |
So the desktop entry written in That one should be using |
I believe this might be for the other desktop entries that make up the file associations. Those are the only ones written at runtime from what I remember. |
I believe upstream combined both desktop entries together in one of the last few releases, so there should only be one desktop entry. According to their packaging docs, it is our responsibility to install the desktop entry, but maybe there's an upstream bug and they're also still installing one 😕 I'll look again when I get back to my desk |
Here is the logic for creating desktop files - https://github.com/Nexus-Mods/NexusMods.App/blob/v0.6.0/src/NexusMods.CrossPlatform/ProtocolRegistration/ProtocolRegistrationLinux.cs . From the context looks like that it's meant to be used for mime handler registration. |
It seems this might be a side effect of no longer setting |
Yeah, looks like the correct desktop entry is installed to Setting APPIMAGE should partially resolve, in that the correct exe would be used, but according to #308324 we should usually leverage the PATH instead of specifying abslute paths into the nix store. See my comment here #331150 (comment). That'd only be a workaround anyway, since the app shouldn't be installing any .desktop file to begin with... The docs say this shouldn't happen when we build using
And the template that we use in |
Ah, my mistake. The desktop entry in Removing the file causes the new one to be used, which works correctly. Running I don't know if we want to do any cleanup for users updating from 0.4 to 0.6, as they will have both a desktop entry and an app config that will cause issues. I can't think of a good way to do such cleanup without potentially causing other issues... |
Yeah, I guess this is an unfortunate side-effect of not having install/uninstall scripts. Maybe our best option would be to add a module for nexusmods-app that adds a few activation scripts. Otherwise the best option is to just document that these manual migration steps are necessary as upstream haven't added anything to handle these version migrations on their own. |
- `NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR` - `INSTALLATION_METHOD_PACKAGE_MANAGER` Backport a fix for `NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR` not applying to the tests.
I don't think it's needed anymore, now we have INSTALLATION_METHOD_PACKAGE_MANAGER
1087f53
to
e75000c
Compare
For users updating there's a few things that need to be done:
Details
I tested whether
Its unfortunate that this all needs to be done, and it's made more awkward by the fact that most users won't know the app will get updated when they bump their system config, let alone that these steps are required. Note The help text for I think we need to accept that the app is still "unstable" and new versions have breaking changes. 😓 Does this mean we should be retaining old versions we've packaged to allow users to pin a version they use ( |
Having fixed versions is unnecessary imo, if people want to fix versions they can override the package to use older versions or "vendor" the older version into their config. |
Where should this kinda thing be documented? Is |
Apologies for conflicting this. This is what I was thinking about how to deal with it: |
I honestly have no idea, maybe someone else with more knowledge can tell you. I'd go with the NixOS Wiki personally because I don't usually read |
meta.homepage
for nexusmods-app is: Nexus-Mods/NexusMods.Appmeta.changelog
for nexusmods-app is: Nexus-Mods/NexusMods.App@v0.5.3
(release)Description of changes
The main change is updating to the latest version (0.5.3), however additional cleanup and reformatting using nixfmt (rfc166 style) has also been done.
This is WIP and still has a few issues to resolve (help needed):Most issues are now resolved, just needs some cleanup and polish:Upstream fix Fix --no-build publishing when axaml compiler is used AvaloniaUI/Avalonia#16835
Upstream issue Tests fail when using
NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR
Nexus-Mods/NexusMods.App#1836Fixed: Building NMA with System Extractor Nexus-Mods/NexusMods.App#1919
_7zz
tonativeCheckInputs
.desktop
filesThings done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)cc @l0b0 @corngood @NickCao @GGG-KILLER @erri120
Closes #317852
Closes #318647
Closes #329592
Closes #321405
Add a 👍 reaction to pull requests you find important.