spot icon indicating copy to clipboard operation
spot copied to clipboard

Support Meson buildtype "plain"

Open tomfitzhenry opened this issue 2 years ago • 7 comments

Describe the bug On my NixOS Pinephone, I realised spot CPU usage was ~70%, and this was fixed by setting a Meson buildtype of "release", as described in spot's README.

NixOS sets buildtype to "plain" by default, per Meson's advice in https://mesonbuild.com/Running-Meson.html

[plain:] no extra build flags are used, even for compiler warnings, useful for distro packagers and other cases where you need to specify all arguments by yourself

It'd be great if spot supported the Meson buildtype "plain", so that distributions produced automagically optimised builds.

Until then, I have raised https://github.com/NixOS/nixpkgs/pull/138412 for NixOS to specify buildtype "release".

To Reproduce Steps to reproduce the behavior:

  1. Compile with -Dbuildtype=plain
  2. Play music
  3. Observe CPU usage is 70%

Expected behavior CPU usage is 10%

General information:

  • Distribution: NixOS
  • Installation method [e. g. built from source, installed from Flathub...]: built from source
  • Version [e.g. 0.1.0]: 0.2.0
  • Device used [e. g. desktop, phone...]: Pinephone

tomfitzhenry avatar Sep 18 '21 21:09 tomfitzhenry

I think we have to solve this on our side, by passing the correct flags to rustc using environment variables.

jtojnar avatar Sep 18 '21 22:09 jtojnar

That seems reasonable, feel free to open a PR with the needed changes -- I haven't had a proper look but I don't think it involves big changes

xou816 avatar Sep 19 '21 03:09 xou816

I think we have to solve this on our side, by passing the correct flags to rustc using environment variables.

I agree with @jtojnar - Meson's "plain" build type does in fact what asked, it doesn't pass build flags automatically. If you wan't a release build with optimizations enabled and you don't wan't to manually pass them with env vars, you should use the "release" build type, that does everything for you :)

Edit: Also because if Spot was to manually pass some optimizing flags with the "plain" build type, things could break for people that expect the proper behaviour (e.g. other distributions)

Tachi107 avatar Dec 03 '21 18:12 Tachi107

Hi!

I am hesitant to say this as it basically gives me more work, but I tend to agree that the plain buildtype should use the release build profile.

It seems to be intended for distro packagers, in which case a debug build is rarely ever useful. I don't think we'd have too much breakage to deal with changing the defaults/plain buildtype from debug to release! The packages that I know of specify the buildtype explicitly already.

Is it still needed though, does NixOS still package spot?

xou816 avatar Dec 05 '21 22:12 xou816

Is it still needed though, does NixOS still package spot?

NixOS does still package spot, but we set the release type to "release", so this issue does not impact NixOS users: https://github.com/NixOS/nixpkgs/blob/83c81e104525d215650d784169ff85afa6eaa247/pkgs/applications/audio/spot/default.nix#L64-L65

I raised this issue as a low-priority FYI, re distributions typically (?) expecting plain:

  • Arch Linux note being unable to use their meson wrapper, which assumes plain: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=spot-client#n24
  • Alpine: https://git.alpinelinux.org/aports/tree/community/spot/APKBUILD#n44

(See https://repology.org/project/spot-client/versions for all distros that package spot.)

tomfitzhenry avatar Dec 05 '21 22:12 tomfitzhenry

Thank you for pointing out the relevant lines!

I wonder how much of this is from poor communication on my end.

Seeing this for instance:

because upstream recommends using --buildtype 'release'

Maybe it's not always been the case, but the default buildtype atm is release, so configuring the target with the defaults might be fine.

Now if some distros explicitly pass buildtype=plain then yeah there is something to do :/ I am not fond of having to maintain both release and plain buildtypes, but I'll see what I can do

xou816 avatar Dec 05 '21 23:12 xou816

I am hesitant to say this as it basically gives me more work, but I tend to agree that the plain buildtype should use the release build profile.

It seems to be intended for distro packagers, in which case a debug build is rarely ever useful. I don't think we'd have too much breakage to deal with changing the defaults/plain buildtype from debug to release! The packages that I know of specify the buildtype explicitly already.

Distros already have to specifically specify --buildtype=plain, it's not a "default" (the default is debug). You could change the default build type to release by adding it to the default_options of the project() call, so that Spot will build will all optimizations by default. If you handle plain in special ways, users won't be able to avoid special flags, breaking some environments :/

When setting the build type to release, the only thing that happens is that Meson sets optimization=3 and debug=false, and it's no different that compiling with the plain build type and manually passing optimization flags through env vars (or at least this is true for C/C++, but I also believe with rustc)

Edit: ops, just saw the last message, and the default build type is already release. Sorry, I'm on my phone and I didn't saw the other comments 😅

Edit 2: anyway, if you ask for plain it's because you can't rely on release for some reason, and making plain the same thing as release will break something somewhere

Tachi107 avatar Dec 06 '21 06:12 Tachi107