nexusmods-app: add a warning about its experimental state #344251
Conversation
MattSturgeon
requested review from
GGG-KILLER,
l0b0,
corngood and
SuperSandro2000
MattSturgeon
force-pushed
the
nexus-warning
branch
from
fd5132d
to
9dd0f29
Compare
|
IIRC ofborg will now complain about any usages of the package. Why not place a version file next to the database and exit if that mismatches? Or display an error with zenity? Then people can act accordingly. Maybe also rm the file but then data might be lost on accident? |
Users need to be aware that the app is experimental. In particular, they must take manual action before updating the package. The warning can be suppressed using `iKnowItsExperimental = true`.
MattSturgeon
force-pushed
the
nexus-warning
branch
from
9dd0f29
to
0fdc996
Compare
😭
Agreed, but it is a low-effort temporary solution that is better than not informing the users IMO.
This sounds more like something upstream should be doing, not us. There's more to it than just the database, upstream advises that users must also ensure all games are in an un-modded state before using a new version of the app. Versioned config was my initial proposal upstream, but their response was this will cause more problems than it solves and investing in a long-term solution isn't worth the effort when future (more stable) versions will have proper database scheme migration.
Is displaying GUI warnings common while evaluating packages? Or are you suggesting we wrap the package with a shell script that (e.g.) shows a dialogue window when the app crashes? That sounds reasonable, but not something I want to invest effort into when this issue will likely only affect us for a handful of weeks or months. |
|
This is a shitty situation, but I think maybe it's best to just let it behave the way it does upstream. https://nexus-mods.github.io/NexusMods.App/users/Uninstall/ What happens if you take their advice and do Sorry, I just realised you mentioned that in your warning message. I assume it works then? If so, I think the upstream docs explain the problem in a suitable way for nixpkgs users... |
This will remove all app state (database) and also reset any games modified using the app to their vanilla state, ready to start fresh with a new version of the app. I believe this only works correctly when run before updating the app though, since a newer version wouldn't be able to read the old database to know which games to reset. One thing it won't reset is any dynamically installed desktop entries. This will only affect users migrating from 0.4.1 though, or users migrating from a non-nixpkgs package to nixpkgs. (When not built with
Agreed. Are you suggesting having a warning that links to upstream's docs is appropriate? (As implemented in this PR) |
No, I just meant that users should use upstream docs to find out this information. If it's not important enough for upstream to handle with an in-app popup or something, then I don't think we need to go out of our way to do that for them. |
Gotcha. I agree we shouldn't be putting in too much effort here, hence why I haven't implemented anything like runtime crash detection or versioned config directories. I do see value in the low-effort warning, though, even if it is an ugly solution. My view is that users updating their app via a package manager are unlikely to be referencing upstream's docs, unless the package manager highlights to them that there is an issue worth their attention. This is in contrast to users who install the software manually, by visiting upstream and downloading (e.g.) an appimage. |
I'm not against this, I just think doing it at evaluation time has some potential problems and would be really easy to miss.
This sounds bad, but I don't understand what it means exactly. |
Is there a better place to warn? Eval time is where a deprecation warning would usually go, and I figured this was similar in spirit. I think warning at build time would not help because (hopefully) the build is usually cached, and unless Doing it at runtime seems like it'd be more effort, and depending on the implementation could also be either too obnoxious or too easily missed. Showing a dialogue at runtime when the app exits non-zero is probably the ideal, but also the highest effort? Unless there's an existing wrapper we could reference. Doing nothing is also an option, but IMO is the worst of a bad bunch 😰 |
Turns out to be the case: https://gist.github.com/GrahamcOfBorg/965b4e3eceb6437594d6aed04786f3ed I'll close this for now, it seems the consensus is we don't really want to do this. |
Description of changes
Users need to be aware that the app is experimental.
In particular, they must take manual action to wipe any state before updating the package, e.g. using
NexusMods.App uninstall-app. If this is not done, the updated app will crash while attempting to read the old database.See earlier discussion #331150 (comment)
And upstream's FAQ: https://nexus-mods.github.io/NexusMods.App/users/faq/#why-do-i-have-to-uninstall-everything-to-update-the-app
This PR adds a temporary warning when evaluating
nexusmods-appthat informs end-users of the above.The warning can be silenced by overriding the package:
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.