fix picard crash: use consistent qt5 version #99465

jorsn · 2020-10-03T11:52:32Z

Motivation for this change

fix picard crashes: Could not find the Qt platform plugin "xcb" #99458 picard crashes: Could not find the Qt platform plugin "xcb"
picard used qt-5.15 while its dependency pyqt5 uses q-5.14
the license was gpl2 while upstream states ”GPL 2.0 or later“ (https://picard.musicbrainz.org/)

Things done

pkgs/applications/audio/picard/default.nix

according to its home page https://picard.musicbrainz.org/, picard is licensed under gpl2+

fixes NixOS#99458 picard used qt-5.15 while its dependency pyqt5 uses q-5.14

jorsn · 2020-10-05T22:08:02Z

Since this is now almost trivial, is there any reason not to commit this?

doronbehar · 2020-10-07T09:48:26Z

picard used qt-5.15 while its dependency pyqt5 uses qt-5.14

Hey and thanks for the patch! I've just encountered a similar issue with gnuradio at #99685 (comment) . I'm a little bit confused. How does your patch regarding the python3Packages thing solve the issue? See also: #99937

FYI pyqt5 needs update (done at #99933 ) but this won't solve #99937 .

FRidh · 2020-10-07T11:10:58Z

pkgs/top-level/all-packages.nix

@@ -22656,7 +22656,7 @@ in

  pianobooster = qt5.callPackage ../applications/audio/pianobooster { };

-  picard = callPackage ../applications/audio/picard { };
+  picard = python3Packages.callPackage ../applications/audio/picard { };


please don't use this pattern, it does not compose. If Qt has a set of packages, and Python, which callPackage to use?

So @FRidh you suggest to leave it with callPackage? I kind of like this because this is an elegant way to tell the expression to use whatever qt5 is used by python3Packages.

please don't use this pattern, it does not compose. If Qt has a set of packages, and Python, which callPackage to use?
– @FRidh

Nothing composes here. You have to be explicit in some cases. With my solution, you have to be explicit in less cases.

The question is not resolved by using libsForQt5*.callPackage or callPackage ({ qt5, ... }: ... ). Also, you'd have to find and change every package using python and qt again when python is updated to use qt515, which is not necessary if you use python*Packages.callPackage.

Somehow, you must prioritize which package set chooses the other package sets.
In general it is more likely that python is overriding qt than qt overriding python. So, the only quick and not completely dirty solution I see is exactly this pattern.

So @FRidh you suggest to leave it with callPackage? I kind of like this because this is an elegant way to tell the expression to use whatever qt5 is used by python3Packages.

Always use the callPackage from the set you are in.

Why? Why not define python applications in pythonPackages then?

Using the callPackage of a sub-scope is officially recommended by the nixpkgs manual:

Adding an application to Nixpkgs. Add a Qt application to all-packages.nix using libsForQt5.callPackage instead of the usual callPackage. The former ensures that all dependencies are built with the same version of Qt.
– https://nixos.org/manual/nixpkgs/stable/#sec-language-qt

Edit: One could even say: For an interpreted program, always use the callPackages from the scope of your interpreter, which should provide consistent dependencies (otherwise it's a bug).

you'd have to find and change every package using python and qt again when python is updated to use qt515, which is not necessary if you use python*Packages.callPackage

Is this a pracitcal scenario? All the packages will build successfully but produce runtime errors!

Always use the callPackage from the set you are in.

@FRidh I'm afraid that's a hard requirement to comply to. To my view it contradicts a different requirements that you (pythonPackages' maintainers) also have - which is "don't put python applications in pkgs/top-level/python-modules/ and pkgs/top-level/python-packages.nix".

Right. Applications are in top-level, libraries in python-packages.nix. Libraries have Python packages as parameters, applications do not. The messy part is when a package is to be used as both, in which case library usage takes precedence, and one uses foo = with python3Packages; toPythonApplication mypackage;.

doronbehar · 2020-10-07T11:20:24Z

I think that if we switch to python3Packages.callPackage, we might as well do this:

diff --git i/pkgs/applications/audio/picard/default.nix w/pkgs/applications/audio/picard/default.nix
index 647a4db7827..f3b7a7bcfa2 100644
--- i/pkgs/applications/audio/picard/default.nix
+++ w/pkgs/applications/audio/picard/default.nix
@@ -1,15 +1,19 @@
-{ stdenv, pythonPackages, fetchFromGitHub, gettext, chromaprint, qt5
+{ stdenv
+, fetchFromGitHub
+, buildPythonApplication
+, gettext
+, qt5
+, pyqt5_with_qtmultimedia
+, pyqt5
+, mutagen
+, chromaprint
+, discid
+, dateutil
 , enablePlayback ? true
 , gst_all_1
 }:
 
-let
-  pyqt5 = if enablePlayback then
-    pythonPackages.pyqt5_with_qtmultimedia
-  else
-    pythonPackages.pyqt5
-  ;
-in pythonPackages.buildPythonApplication rec {
+buildPythonApplication rec {
   pname = "picard";
   version = "2.4.4";
 
@@ -31,8 +35,8 @@ in pythonPackages.buildPythonApplication rec {
     ]
   ;
 
-  propagatedBuildInputs = with pythonPackages; [
-    pyqt5
+  propagatedBuildInputs = [
+    (if enablePlayback then pyqt5_with_qtmultimedia else pyqt5)
     mutagen
     chromaprint
     discid

jorsn · 2020-10-07T12:05:05Z

I think that if we switch to python3Packages.callPackage, we might as well do this:
– @doronbehar

”This“ breaks the package: You'd have to substitute pyqt5 in the whole package definition. So it's better to choose one time in the let which pyqt5 to use and use that in the whole package. This can be done either by using pythonPackages, which is always the current python if you are called by python*.callPackage', or by defining myPyqt5 = …` or similarly and using it in the whole package. The first change is smaller than the second, and thus I chose it in the end.

doronbehar · 2020-10-07T13:10:40Z

”This“ breaks the package: You'd have to substitute pyqt5 in the whole package definition.

It wouldn't have been that bad if pyqt5 was evaluated only once inside the arguments, but it isn't.

Most of the diff's idea was to let the scope of python3Packaes.callPackage tell the package what are the other python modules etc. So here's a version of it with a let pyqt' = ; in:

diff --git i/pkgs/applications/audio/picard/default.nix w/pkgs/applications/audio/picard/default.nix
index 647a4db7827..a200cc2f811 100644
--- i/pkgs/applications/audio/picard/default.nix
+++ w/pkgs/applications/audio/picard/default.nix
@@ -1,15 +1,23 @@
-{ stdenv, pythonPackages, fetchFromGitHub, gettext, chromaprint, qt5
+{ stdenv
+, fetchFromGitHub
+, buildPythonApplication
+, gettext
+, qt5
+, pyqt5_with_qtmultimedia
+, pyqt5
+, mutagen
+, chromaprint
+, discid
+, dateutil
 , enablePlayback ? true
 , gst_all_1
 }:
 
 let
-  pyqt5 = if enablePlayback then
-    pythonPackages.pyqt5_with_qtmultimedia
-  else
-    pythonPackages.pyqt5
-  ;
-in pythonPackages.buildPythonApplication rec {
+  pyqt5' = if enablePlayback then pyqt5_with_qtmultimedia else pyqt5;
+in
+
+buildPythonApplication rec {
   pname = "picard";
   version = "2.4.4";
 
@@ -21,7 +29,7 @@ in pythonPackages.buildPythonApplication rec {
   };
 
   nativeBuildInputs = [ gettext qt5.wrapQtAppsHook qt5.qtbase ]
-    ++ stdenv.lib.optionals (pyqt5.multimediaEnabled) [
+    ++ stdenv.lib.optionals (pyqt5'.multimediaEnabled) [
       qt5.qtmultimedia.bin
       gst_all_1.gstreamer
       gst_all_1.gst-vaapi
@@ -31,8 +39,8 @@ in pythonPackages.buildPythonApplication rec {
     ]
   ;
 
-  propagatedBuildInputs = with pythonPackages; [
-    pyqt5
+  propagatedBuildInputs = [
+    pyqt5'
     mutagen
     chromaprint
     discid
@@ -48,7 +56,7 @@ in pythonPackages.buildPythonApplication rec {
   preFixup = ''
     makeWrapperArgs+=("''${qtWrapperArgs[@]}")
   ''
-    + stdenv.lib.optionalString (pyqt5.multimediaEnabled) ''
+    + stdenv.lib.optionalString (pyqt5'.multimediaEnabled) ''
       makeWrapperArgs+=(--prefix GST_PLUGIN_SYSTEM_PATH_1_0 : "$GST_PLUGIN_SYSTEM_PATH_1_0")
     ''
   ;

This can be done either by using pythonPackages, which is always the current python if you are called by python*.callPackage'

TBH my only objection is that I find it a bit uglier, but it shouldn't be a blocker :).

FRidh · 2020-10-07T13:13:06Z

I think that if we switch to python3Packages.callPackage, we might as well do this:

No. Don't pass in individual Python packages outside the Python packages set. This is done because it is a requirement that all packages come from the same package set.

jorsn · 2020-10-07T13:17:51Z

TBH my only objection is that I find it a bit uglier, but it shouldn't be a blocker :).

Since your fixed solution is functionally equivalent to mine, it's a matter of taste and of patch size, I think. I don't mind another taste, since we have general issues here. Patch size matters in case we must change lots of packages.

jorsn · 2020-10-07T13:20:08Z

This is done because it is a requirement that all packages come from the same package set.

This is the same issue as which callPackage to use. If you use python*Packages.callPackage in the end, I don't see any problem in using @doronbehr's proposal. In the end it is the same as inheriting pythonPackages.

doronbehar · 2020-10-07T13:28:49Z

@FRidh then what solution do you suggest? Use callPackage with { qt5 = qt514;}? That would break once the pythonInterpreters will not be overridden. The current state of things is that an entire set is overridden, the override is almost completely hidden from packagers, and everyone are expected to know about the override and comply with {qt5 = qt514;} when needed? This is very uncooperative, not to mention the fact that it seems as if qutebrowser is the only python package that really needs qt514 and can't use qt515? Are we overriding the entire pythonInterpreters and breaking other packagers only to spare an override from 1 package?

FRidh · 2020-10-07T13:36:12Z

This is very uncooperative, not to mention the fact that it seems as if qutebrowser is the only python package that really needs qt514 and can't use qt515? Are we overriding the entire pythonInterpreters and breaking other packagers only to spare an override from 1 package?

I do not know what version is best used for the set, or why 5.14 was chosen at the time. Investigating now, It seems it was done ~~because of feedback here #97586 (comment).~~ https://github.com/NixOS/nixpkgs/pull/97586/files#diff-2afbba83e1b14f9e0c304c769831a0e2L17

doronbehar · 2020-10-07T13:52:57Z

I do not know what version is best used for the set, or why 5.14 was chosen at the time. Investigating now, It seems it was done because of feedback here #97586 (comment).

This is a disastrous example of a common bad habit in Nixpkgs - not committing to updates fully.¹

Nixpkgs wouldn't have progressed anywhere if updates would have been blocked like that forever. How long did we expect to leave that override there? The fact that it's breaking packages is only a side effect of the real issue. The issue is that a common dependency is updated, and it's natural for the update to break other packages, but after someone cries "Hey this broke this and that for me 😭" we block the update for everyone. The natural thing to do is to fix these packages individually and not block the update for a whole set!

Tremendous efforts are needed if such commits stay for long, examples include #89264 & #89731 & #86808 .

jorsn · 2020-10-07T13:53:58Z

why 5.14 was chosen at the time. Investigating now, It seems it was done because of feedback here #97586 (comment).

This makes no sense. Python doesn't use 5.14 because qtbrowser uses 5.15. Also, #97586 was after the change to use 5.15 (see #99458 (comment)).

FRidh · 2020-10-07T14:00:48Z

@doronbehar I agree, but it is also understandable. These Nixpkgs-wide changes are massive. Its near impossible to make all the required changes and test them as a single person. In this case, it seems like it would have been good to have had communicated clearer that there was "some debt".

This makes no sense. Python doesn't use 5.14 because qtbrowser uses 5.15. Also, #97586 was after the change to use 5.15 (see #99458 (comment)).

Before then it was already pinned https://github.com/NixOS/nixpkgs/pull/97586/files#diff-2afbba83e1b14f9e0c304c769831a0e2L17 Indeed, that's what you referred to in your comment!

jorsn · 2020-10-07T14:42:07Z

Ah, sorry! So I was finally also confused ;)

FRidh · 2020-10-07T14:48:13Z

Pushed bafbb05 to master. Closing this now in favor of https://github.com/NixOS/nixpkgs/pull/99956/commits.

jorsn requested review from FRidh, ehmry and peti October 3, 2020 11:53

jorsn commented Oct 3, 2020

View reviewed changes

pkgs/applications/audio/picard/default.nix Outdated Show resolved Hide resolved

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Oct 3, 2020

jorsn added 2 commits October 3, 2020 23:29

picard: apply new licensing convention

bafbb05

according to its home page https://picard.musicbrainz.org/, picard is licensed under gpl2+

picard: use consistent qt5 version

a0285a2

fixes NixOS#99458 picard used qt-5.15 while its dependency pyqt5 uses q-5.14

jorsn force-pushed the pythonPackages branch from e979a6e to a0285a2 Compare October 3, 2020 21:33

doronbehar mentioned this pull request Oct 7, 2020

Python packages depending on qt5, use qt514 while all-packages.nix defaults to qt515, causing incompatibilities. #99937

Closed

doronbehar mentioned this pull request Oct 7, 2020

gnuradio: rewrite #99685

Merged

10 tasks

FRidh requested changes Oct 7, 2020

View reviewed changes

FRidh closed this Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix picard crash: use consistent qt5 version #99465

fix picard crash: use consistent qt5 version #99465

jorsn commented Oct 3, 2020

jorsn commented Oct 5, 2020

doronbehar commented Oct 7, 2020

FRidh Oct 7, 2020

doronbehar Oct 7, 2020

jorsn Oct 7, 2020 •

edited

Loading

FRidh Oct 7, 2020

jorsn Oct 7, 2020 •

edited

Loading

jorsn Oct 7, 2020 •

edited

Loading

jorsn Oct 7, 2020 •

edited

Loading

doronbehar Oct 7, 2020

FRidh Oct 7, 2020

doronbehar commented Oct 7, 2020

jorsn commented Oct 7, 2020

doronbehar commented Oct 7, 2020

FRidh commented Oct 7, 2020

jorsn commented Oct 7, 2020

jorsn commented Oct 7, 2020

doronbehar commented Oct 7, 2020

FRidh commented Oct 7, 2020 •

edited

Loading

doronbehar commented Oct 7, 2020

jorsn commented Oct 7, 2020

FRidh commented Oct 7, 2020

jorsn commented Oct 7, 2020

FRidh commented Oct 7, 2020

fix picard crash: use consistent qt5 version #99465

fix picard crash: use consistent qt5 version #99465

Conversation

jorsn commented Oct 3, 2020

Motivation for this change

Things done

jorsn commented Oct 5, 2020

doronbehar commented Oct 7, 2020

FRidh Oct 7, 2020

Choose a reason for hiding this comment

doronbehar Oct 7, 2020

Choose a reason for hiding this comment

jorsn Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

FRidh Oct 7, 2020

Choose a reason for hiding this comment

jorsn Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

jorsn Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

jorsn Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

doronbehar Oct 7, 2020

Choose a reason for hiding this comment

FRidh Oct 7, 2020

Choose a reason for hiding this comment

doronbehar commented Oct 7, 2020

jorsn commented Oct 7, 2020

doronbehar commented Oct 7, 2020

FRidh commented Oct 7, 2020

jorsn commented Oct 7, 2020

jorsn commented Oct 7, 2020

doronbehar commented Oct 7, 2020

FRidh commented Oct 7, 2020 • edited Loading

doronbehar commented Oct 7, 2020

jorsn commented Oct 7, 2020

FRidh commented Oct 7, 2020

jorsn commented Oct 7, 2020

FRidh commented Oct 7, 2020

jorsn Oct 7, 2020 •

edited

Loading

jorsn Oct 7, 2020 •

edited

Loading

jorsn Oct 7, 2020 •

edited

Loading

jorsn Oct 7, 2020 •

edited

Loading

FRidh commented Oct 7, 2020 •

edited

Loading