-
-
Notifications
You must be signed in to change notification settings - Fork 14.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.1 #331150
nexusmods-app: 0.4.1 -> 0.6.1 #331150
Conversation
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... https://github.com/NixOS/nixpkgs/blob/74b8ededca4e5fd116fb3312ec4cf6b12d40a2f1/pkgs/by-name/ne/nexusmods-app/package.nix#L29-L35 |
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 🤔
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 |
Given the app is still unstable/experimental, would it be reasonable to add a EDIT: I've opened #344251 to tackle this. |
e75000c
to
1c435e9
Compare
@MattSturgeon you've done a lot of work on this PR, and I want to make sure it's not being held up waiting on feedback/review. Just let me know if you feel like it's ready, or if you'd like help with anything. |
1c435e9
to
f2978bd
Compare
f2978bd
to
f6ef716
Compare
f6ef716
to
2600ef1
Compare
Define new constants: - `NEXUSMODS_APP_USE_SYSTEM_EXTRACTOR` - `INSTALLATION_METHOD_PACKAGE_MANAGER` Also: - Stop setting `APPIMAGE` We now have `INSTALLATION_METHOD_PACKAGE_MANAGER` - Set `ReadyToRun` See NixOS#331150 (comment) - Install desktop entry, appstream, & icons See https://nexus-mods.github.io/NexusMods.App/developers/Contributing/#for-package-maintainers
2600ef1
to
631c290
Compare
@SuperSandro2000 this seems to be blocked on your previous change request, but I think it's all been addressed. |
meta.homepage
for nexusmods-app is: Nexus-Mods/NexusMods.AppNotes for end-users
Caution
Before upgrading to a new version of
nexusmods-app
you will need a fresh start.If you have configuration leftover from an old version, the updated app will crash.
Tip
You can reset all modded games and wipe your nexusmods-app config using:
Tip
You may also need to remove old desktop entries to avoid other issues:
Description of changes
This PR updates to the latest version, 0.6.1.
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 #318647
Closes #329592
Related #321405
Related #317852
Add a 👍 reaction to pull requests you find important.