Skip to content

exaile: init at 4.1.1#120761

Merged
veprbl merged 1 commit intoNixOS:masterfrom
ryneeverett:exaile-init
Sep 8, 2021
Merged

exaile: init at 4.1.1#120761
veprbl merged 1 commit intoNixOS:masterfrom
ryneeverett:exaile-init

Conversation

@ryneeverett
Copy link
Contributor

Disclaimers:

  • I haven't tested the binary built on master because I run the stable channel on my workstation and it fails with the error ImportError: /nix/store/0c7c96gikmzv87i7lv3vq5s1cmfjd6zf-glibc-2.31-74/lib/libc.so.6: version GLIBC_2.32' not found (required by /nix/store/ap4d6cqfhakl2dp6081z2c6bsdcinsha-glib-2.66.8/lib/libglib-2.0.so.0). However the binary works fine when I backport this branch to 20.09 so I believe this is just a case of glibc incompatibility.
  • I tested that enabling the optional dependencies allows the plugins to load but I did not test the functionality of all of them. Also there are a plugin or two with dependencies that aren't in nixpkgs yet so those still wont work.
  • I do find it a bit weird that a package like this would have so many optional inputs but that is my deferral to upstream's decision to implement all these features as plugins with optional dependencies. Let me know if there's a better way to handle this or if you disagree with my choices of default settings.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@legendofmiracles
Copy link
Contributor

legendofmiracles commented Apr 26, 2021

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

1 package built:
  • exaile

But running it on unstable returns:

INFO    : Loading Exaile devel...
INFO    : Using Python 3.8.8
INFO    : Using PyGObject 3.38.0
INFO    : Loading settings...
INFO    : Using Locale en_US UTF-8
INFO    : Using Mutagen 1.45.1
INFO    : Using GStreamer 1.18.2
INFO    : Reconfiguring crossfading
INFO    : Crossfade: disabled
INFO    : Using GTK+ 3.24.27
INFO    : Using GTK+ theme oomox-aaa

(exaile:365772): Gtk-WARNING **: 19:09:15.337: Could not find the icon 'starred'. The 'hicolor' theme
was not found either, perhaps you need to install it.
You can get a copy from:
	http://icon-theme.freedesktop.org/releases
WARNING : Icon "starred" not found
ERROR   : Unhandled exception
Traceback (most recent call last):
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xl/main.py", line 561, in __init__
    self.__init()
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xl/main.py", line 629, in __init
    from xlgui.widgets.info import Splash
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xlgui/widgets/info.py", line 36, in <module>
    from xlgui.widgets import playlist
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xlgui/widgets/playlist.py", line 41, in <module>
    from xlgui.widgets.common import AutoScrollTreeView
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xlgui/widgets/common.py", line 42, in <module>
    from xlgui import icons
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xlgui/icons.py", line 760, in <module>
    MANAGER = IconManager()
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xlgui/icons.py", line 536, in __init__
    self.rating_active_pixbuf = ExtendedPixbuf(
  File "/nix/store/7c1h3xqqpb4frwg7da4w0b1rg6mh8rvw-exaile-4.1.1/lib/exaile/xlgui/icons.py", line 62, in __init__
    pixbuf.get_colorspace(),
AttributeError: 'NoneType' object has no attribute 'get_colorspace'

This is inside the nix-review shell.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 26, 2021
@ryneeverett
Copy link
Contributor Author

Thanks for the review @legendofmiracles!

I can reproduce that crash with nix-shell --pure (which i'm surprised nix-review doesn't use) and I force pushed the addition of an iconTheme input which fixes the crash.

@legendofmiracles
Copy link
Contributor

legendofmiracles commented Apr 26, 2021

Seems to work now for me too.

@dschrempf
Copy link
Contributor

dschrempf commented Apr 26, 2021

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

1 package built:
  • exaile

GUI also works. I cannot say anything about using so many options.

Comment on lines 11 to 20
Copy link
Member

Choose a reason for hiding this comment

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

Should we enable them by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? The options I defaulted to false are all plugins that are disabled by default and can be enabled in the gui so the only real consequence is closure size. If they're all going to be true by default perhaps they should be renamed disableX ? false per this discussion.

Copy link
Member

Choose a reason for hiding this comment

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

@ryneeverett Adding too many options is not very productive as we are not build testing all of the combinations. A power user can always do something like exaile.override { webkitgtk = null; }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@veprbl I guess I don't really accept the premise that it's a package manager's job to test whether the optional dependencies -- as declared by upstream -- are truly optional. Also isn't your argument equally valid against any number of options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I ran some builds to compare closure size:

  • All options false -> 583mb
  • Current default options -> 832mb
  • All options true -> 1.5gb

Nearly doubling the closure size in order to install the dependencies of all the plugins (which are disabled by default) seems crazy. Especially when there's no reason to believe upstream is done adding plugins.

Also, particularly for the optional python dependencies, it seems improbable that they have any effect at build time.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I don't really accept the premise that it's a package manager's job to test whether the optional dependencies -- as declared by upstream -- are truly optional.

When you write an API with a bunch of knobs you usually want those to be tested. In the current setup there is not even a test that exile's build system accepts the libraries that we may pass to it. It will be up to a user to discover if something is broken.

Also isn't your argument equally valid against any number of options?

Not sure what you mean by that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current setup there is not even a test that exile's build system accepts the libraries that we may pass to it.

Is there a simple way to test packages with different combinations of inputs? I'm not familiar, though a hack would be to add another top level package exaileWithPlugins in which all the flags are true.

Also isn't your argument equally valid against any number of options?

Not sure what you mean by that.

I'm working from the assumption that options are not generally tested and if that's the case then it's not clear that many options are worse in principle than fewer options.

Copy link
Member

Choose a reason for hiding this comment

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

In the current setup there is not even a test that exile's build system accepts the libraries that we may pass to it.

Is there a simple way to test packages with different combinations of inputs? I'm not familiar, though a hack would be to add another top level package exaileWithPlugins in which all the flags are true.

Such package is often called exaileFull (e.g. gitFull).

Also isn't your argument equally valid against any number of options?

Not sure what you mean by that.

I'm working from the assumption that options are not generally tested and if that's the case then it's not clear that many options are worse in principle than fewer options.

More options means more combinations to test/debug, or, if you like, combinations left untested.

@ryneeverett ryneeverett force-pushed the exaile-init branch 2 times, most recently from 5a42929 to 4f5ef89 Compare April 27, 2021 17:29
@ryneeverett ryneeverett force-pushed the exaile-init branch 3 times, most recently from 3aadf93 to b827143 Compare September 7, 2021 04:13
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Sep 7, 2021
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

LGTM

@veprbl veprbl merged commit 3e9f6d7 into NixOS:master Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants