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

osu-lazer{,-bin}: 2024.817.0 -> 2024.906.1 #340125

Merged
merged 6 commits into from
Sep 7, 2024

Conversation

gepbird
Copy link
Contributor

@gepbird gepbird commented Sep 6, 2024

Description of changes

Closes #340082.

Release: https://github.com/ppy/osu/releases/tag/2024.906.1

osu! migrated from squirrel to velopack, which offers auto update on linux, I'll see if it will cause any issue on nix, drafting for now.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@gepbird
Copy link
Contributor Author

gepbird commented Sep 6, 2024

osu-lazer-bin may have issues when a newer version gets released, see https://docs.velopack.io/packaging/operating-systems/linux#updating.
Even though we're unpacking their AppImage, I'm a little worried the updater will ask try to replace something in /nix/store which will fail, and the user won't be able to open the app.

@thiagokokada @stepbrobd Do you think we should hold this package back until there's a new release to test the updater, or should we risk the potential breakage and merge it?

Best would testing the updater now but I'm not sure how to do that when there are no newer releases.

@gepbird gepbird marked this pull request as ready for review September 6, 2024 16:07
@alikindsys
Copy link
Contributor

alikindsys commented Sep 6, 2024

I assumed that the change from Squirrel to Velopack would lead to at least one hotfix being pushed to fix bugs caused by it, and so far I was able to track:

So we're pretty much guaranteed a 906.2 or a 90X.1 version to test if the new autoupdater breaks NixOS. We should wait that out and test it, since it could break if it tried to modify /nix and that would be a worse experience then having a slightly outdated version.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Sep 6, 2024
@thiagokokada
Copy link
Contributor

It is highly unlikely that the updater could mess the files in /nix/store. The files inside the store are set to be immutable, so unless the updater remove this (that I find highly unlikely), the updater can't overwrite the files and will probably fail.

@gepbird
Copy link
Contributor Author

gepbird commented Sep 6, 2024

Good to know we can expect a few hotfixes soon, thanks for linking those P0 issues!

It is highly unlikely that the updater could mess the files in /nix/store. The files inside the store are set to be immutable, so unless the updater remove this (that I find highly unlikely), the updater can't overwrite the files and will probably fail.

I know that velopack (or anything other than nix builders) can't write to /nix/store, I'm worried about about the game not starting due to the update failing.
Take discord for example, by default it doesn't let you use the app if you don't have the latest version, I've seen this frustrating message many times on other distros, and in that case and this case the user can't really update the application.

@stepbrobd
Copy link
Member

stepbrobd commented Sep 6, 2024

Could we wrap the program and set OSU_EXTERNAL_UPDATE_PROVIDER to some string to disable osu! autoupdate?

Reference: https://github.com/ppy/osu/blob/04d133832f5d4d245392f51c3e71bfc818d655d4/osu.Desktop/OsuGameDesktop.cs#L98-L106

Edit: Instead of wrapping the program, patching the desktop file would be a better approach: OSU_EXTERNAL_UPDATE_PROVIDER=1 osu!

@stepbrobd
Copy link
Member

Relavent: ppy/osu#15642

@peppy
Copy link

peppy commented Sep 7, 2024

Just to clarify, OSU_EXTERNAL_UPDATE_PROVIDER is what you want if you want us to do no auto updating on your behalf. I'd assume you already have this set, but if not you should definitely add it.

@smoogipoo
Copy link

smoogipoo commented Sep 7, 2024

cc @caesay : We have our own flag, but in general, how does the updater handle unpacked AppImages (flatpak/etc)? Does it no-op if it detects that?

@gepbird
Copy link
Contributor Author

gepbird commented Sep 7, 2024

Edit: Instead of wrapping the program, patching the desktop file would be a better approach: OSU_EXTERNAL_UPDATE_PROVIDER=1 osu!

@stepbrobd Is there a specific reason patching the desktop file is better other than less code? Some people (including myself) don't use desktop files and just run the binary via rofi/dmenu/terminal.

Also thanks for confirming OSU_EXTERNAL_UPDATE_PROVIDER=1 variable does exactly what we want :)

pkgs/games/osu-lazer/bin.nix Outdated Show resolved Hide resolved
@caesay
Copy link

caesay commented Sep 7, 2024

@smoogipoo to answer your question, to disable velopack you should NOT call VelopackApp builder functions, which assume updates are handled by velopack. I assume you can check for the existance of your custom flag and then conditionally call it. Velopack also does not support updating extracted AppImage applications on Linux.

. ${makeWrapper}/nix-support/setup-hook
mv -v $out/bin/${pname} $out/bin/osu!
wrapProgram $out/bin/osu! \
--set OSU_EXTERNAL_UPDATE_PROVIDER 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be working:
2024-09-07-180646_hyprshot

@thiagokokada
Copy link
Contributor

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

2 packages built:
  • osu-lazer
  • osu-lazer-bin

@stepbrobd
Copy link
Member

stepbrobd commented Sep 7, 2024

Do we need to set the environment variable for Darwin systems as well?

I did some digging and this is what I have (not tested):

patchPhase = ''
  runHook prePatch
  ${xcbuild}/bin/plutil -replace LSEnvironment -json '{"OSU_EXTERNAL_UPDATE_PROVIDER":"1"}' 'osu!.app/Contents/Info.plist'
  runHook postPatch
'';

Ref: https://superuser.com/questions/476752/setting-environment-variables-in-os-x-for-gui-applications

Update:

The patch can be applied to Info.plist no problem but osu can't launch after the modification with the following error:

The application cannot be opened for an unexpected reason, error=Error Domain=RBSRequestErrorDomain Code=5 "Launch failed." UserInfo={NSLocalizedFailureReason=Launch failed., NSUnderlyingError=0x600001dee040 {Error Domain=NSPOSIXErrorDomain Code=153 "Unknown error: 153" UserInfo={NSLocalizedDescription=Launchd job spawn failed}}}

I don't know how to approach this

@thiagokokada
Copy link
Contributor

Do we need to set the environment variable for Darwin systems as well?

I think you can ignore Darwin for now, but if you want to try maybe we can wrap the binary like we do in Linux?

@stepbrobd
Copy link
Member

Result of nixpkgs-review pr 340125 run on aarch64-darwin 1

1 package built:
  • osu-lazer-bin

@thiagokokada thiagokokada merged commit 7f90211 into NixOS:master Sep 7, 2024
24 of 26 checks passed
@gepbird gepbird deleted the osu-lazer-2024.906.1 branch September 7, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: osu-lazer 2024.817.0 → 2024.906.1
7 participants