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

fix picard crash: use consistent qt5 version #99465

Closed
wants to merge 2 commits into from

Conversation

jorsn
Copy link
Member

@jorsn jorsn commented Oct 3, 2020

Motivation for this change
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.

@jorsn jorsn requested review from FRidh, ehmry and peti October 3, 2020 11:53
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
Copy link
Member Author

jorsn commented Oct 5, 2020

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

@doronbehar
Copy link
Contributor

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 .

@doronbehar doronbehar mentioned this pull request Oct 7, 2020
10 tasks
@@ -22656,7 +22656,7 @@ in

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

picard = callPackage ../applications/audio/picard { };
picard = python3Packages.callPackage ../applications/audio/picard { };
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jorsn jorsn Oct 7, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jorsn jorsn Oct 7, 2020

Choose a reason for hiding this comment

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

Why? Why not define python applications in pythonPackages then?

Copy link
Member Author

@jorsn jorsn Oct 7, 2020

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

@jorsn jorsn Oct 7, 2020

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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
Copy link
Member Author

jorsn commented Oct 7, 2020

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
Copy link
Contributor

”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
Copy link
Member

FRidh commented Oct 7, 2020

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
Copy link
Member Author

jorsn commented Oct 7, 2020

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
Copy link
Member Author

jorsn commented Oct 7, 2020

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
Copy link
Contributor

@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
Copy link
Member

FRidh commented Oct 7, 2020

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
Copy link
Contributor

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.1

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
Copy link
Member Author

jorsn commented Oct 7, 2020

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
Copy link
Member

FRidh commented Oct 7, 2020

@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
Copy link
Member Author

jorsn commented Oct 7, 2020

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

@FRidh
Copy link
Member

FRidh commented Oct 7, 2020

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

@FRidh FRidh closed this Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

picard crashes: Could not find the Qt platform plugin "xcb"
4 participants