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 passing qt5 version to pythonInterpreters #98197

Merged
merged 1 commit into from Sep 22, 2020
Merged

fix passing qt5 version to pythonInterpreters #98197

merged 1 commit into from Sep 22, 2020

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Sep 18, 2020

fixes c88f3ad, which resulted in
qt 5.15 being used in pythonPackages, despite 5.14 being
declared, and adapts qutebrowser accordingly.

'callPackage { pkgs = pkgs // { … }; }' does not work, because
it does not take into account the recursive evaluation of nixpkgs:

pkgs/development/interpreters/python/default.nix calls
pkgs/top-level/python-packages.nix with callPackage.
Thus, even if the former gets passed the updated pkgs,
the latter always gets passed pkgs.pkgs.

For the change in the qt5 version to apply consistently, 'pkgs.extend'
must be used.

qutebrowser only used the right qt5 version (5.15) because all
pythonPackages used it anyway.

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.

Copy link
Member

@jorsn jorsn left a comment

Choose a reason for hiding this comment

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

Thank you for adopting, but apart from some spelling (pkgs: _: vs final: prev:), your solution isn't complete.

Please either change the PR according to the comments or just revive #98185 and close this one, if spelling is not too important to you. I used pkgs: _: in #98185 because _ is not used anyway, so it emphasises the overridden pkgs.
#98185 is functionally exactly what I am requesting.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
qt5 = prev.qt515;
libsForQt5 = prev.libsForQt515;
});
in with pkgs_; libsForQt5.callPackage ../applications/networking/browsers/qutebrowser { };
Copy link
Member

@jorsn jorsn Sep 18, 2020

Choose a reason for hiding this comment

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

First one unimportant thing: Why the with and not pkgs_.libsForQt5? I think with is only to be used when there's a great gain from it, because it can obfuscate where functions come from. However, there's no benefit but it doesn't hurt, I think.

Second, and importantly: This has no effect on qutebrowser. Not qutebrowser but python3 needs overridden pkgs. qutebrowser needs overridden python3, like the one I passed in #98185, not pkgs. Maybe it makes sense to override python3 in pkgs for qutebrowser, so pkgs.extend { python3 = ... }. But this means more recursive evaluations, and it's probably sufficient to just pass qutebrowser the overridden python3 = (pkgs.pythonInterpreters.override { inherit pkgs; }).python38.

Edit: This becomes inconsisten when a package calls a subpackage with callPackage and the subpackage uses pythonPackages.callPackage. But this case is probably not too frequent and it applies to every overriding. So I think, it is sufficient to handle what we have (qutebrowser) and not make qutebrowser the canonical example for everything if this requires more nixpkgs evaluations.

On the toplevel, pkgs.libsForQt5 == pkgs.libsForQt515 anyway, but to any package in python*Packages, pkgs.libsForQt5 == pkgs.libsForQt514. As qutebrowser uses PyQt5, the python packages need to be evaluated with qt5, hence with the override above.

Copy link
Member Author

@FRidh FRidh Sep 18, 2020

Choose a reason for hiding this comment

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

Why the with and not pkgs_.libsForQt5? I think with is only to be used when there's a great gain from it, because it can obfuscate where functions come from. However, there's no benefit but it doesn't hurt, I think.

Right, had to clean that up.

Second, and importantly: This has no effect on qutebrowser. Not qutebrowser but python3 needs overridden pkgs. qutebrowser needs overridden python3, like the one I passed in #98185, not pkgs

I just force-pushed a change and we now have the same store paths.

The pkgs passed to pythonInterpreters is now also extended, to ensure no other Python overrides are lost. This part I consider important.

Copy link
Member

@jorsn jorsn Sep 18, 2020

Choose a reason for hiding this comment

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

The pkgs passed to pythonInterpreters is now also extended, to ensure no other Python overrides are lost. This part I consider important.

That's good, I missed it out.

Your result is fine for me. It seems correct. Now, there's only one change I still propose:

diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 0b9170af90e..b196267a87f 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -10160,9 +10160,9 @@ in
 
   pythonInterpreters = callPackage ./../development/interpreters/python {
     # Overrides that apply to all Python interpreters
-    pkgs = pkgs.extend (pkgs: _: {
-      qt5 = pkgs.qt514;
-      libsForQt5 = pkgs.libsForQt514;
+    pkgs = pkgs.extend (final: _: {
+      qt5 = final.qt514;
+      libsForQt5 = final.libsForQt514;
     });
   };
   inherit (pythonInterpreters) python27 python36 python37 python38 python39 python3Minimal pypy27 pypy36;
@@ -22836,14 +22836,29 @@ in
   quodlibet-xine-full = quodlibet-full.override { xineBackend = true; tag = "-xine-full"; };
 
   qutebrowser = let
-    pkgs_ = pkgs.extend(_: prev: {
-      pythonInterpreters = prev.pythonInterpreters.override(oldAttrs: {
-        pkgs = oldAttrs.pkgs.extend(_: _: {
-          inherit (pkgs) qt5 libsForQt5;
-        });
+    pyver = with python3.sourceVersion; major + minor;
+    pyInts = pkgs.pythonInterpreters.override(oldAttrs: {
+      # evaluate fixed point of nixpkgs so that the qt5 version applies for all
+      # python and non-python dependencies of qutebrowser as well (PyQt5!)
+      pkgs = oldAttrs.pkgs.extend(final: _: {
+        qt5 = final.qt515;
+        libsForQt5 = final.libsForQt515;
+        pythonInterpreters = pyInts;
       });
     });
-  in pkgs_.libsForQt5.callPackage ../applications/networking/browsers/qutebrowser { };
+    # Idea:
+    # 1. use pyInts.python*.pkgs.callPackage to obtain the overridden libsForQt5
+    #    from python packages, thus keeping all overrides from pyInts/pythonInterpreters.
+    # 2. use the override* functions of the inner callPackage, so that not only
+    # libsForQt5 but the arguments of ../applications/networking/browsers/qutebrowser
+    # can be overridden.
+    # TODO: Simplify python apps/scoping+overriding.
+    f = { libsForQt5 }:
+      # attr 'pkg' is necessary because the outer callPackage would replace the
+      # override* functions of the inner callPackage.
+      { pkg = libsForQt5.callPackage ../applications/networking/browsers/qutebrowser {}; };
+    outerPkg = pyInts."python${pyver}".pkgs.callPackage f {};
+  in outerPkg.pkg;
 
   rabbitvcs = callPackage ../applications/version-management/rabbitvcs {};
 

I changed pkgs to final in the definition of pythonInterpreters, because it is clearer (you were right with this in the beginning). The second change (in the def of qutebrowser) reduces the number of recursions drastically while keeping the properties we want:

  1. All deps of qutebrowser use qt 5.15 consistently,
  2. all overrides from pythonInterpreters are kept, except for qt5 and libsForQt5,
  3. qutebrowser is overridable as usual.

The Results

  1. All three patches (d639778 (jorsn0), 25f146d844a0892588668da827d23e14071d5ce1 (fidh) and my patch proposed above (jorsn1)) yield the same store path (/nix/store/ysvpc6awl6wzgpswrgfsjzax25gcx8x7-qutebrowser-1.13.1).

  2. Number of evaluations while evaluating qutebrowser:

pkg set fidh jorsn0 jorsn1
nixpkgs 26 13 10
python*Packages 7 4 1

How did I get them?

I measured the number of recursions by adding trace markers (builtins.trace "nixfix", builtins.trace "pyfix").
Then, I ran

nix eval "$HOME/src/nix/nixpkgs#qutebrowser" --apply 'toString' |& tee <result file>

for each solution and gathered the results with

for f in *.fix.log; do
    echo $f
    echo -n 'nix: '; grep nixfix $f | wc -l
    echo -n 'py:  '; grep pyfix  $f | wc -l
    echo ''
done > fix.stats

Copy link
Member

Choose a reason for hiding this comment

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

BTW: If qutebrowser were defined in pkgs/top-level/python-packages.nix the double-callPackage trick would be superfluous.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the detailed investigation! Its interesting to see how big of an impact the chosen solution can have.

I've kept my commit mostly as it was, just using final now instead of pkgs. I am not going to take your other proposed changes though, mostly for simplicity. I find it important that there is a general and correct way for overriding, thus if the Python packages set needs a different pkgs it needs to be overridden in pkgs, and not one level down only, in pythonInterpreters.

The performance impact, while definitely relevant, is due to the general method used; nested fixed-points along with self references in the interpreter. This needs to be fixed elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks and you're welcome! I think this is a general issue with the scoping system and fixed points.

@FRidh FRidh mentioned this pull request Sep 18, 2020
10 tasks
@jorsn jorsn mentioned this pull request Sep 18, 2020
11 tasks
@veprbl veprbl linked an issue Sep 21, 2020 that may be closed by this pull request
@nyanloutre nyanloutre mentioned this pull request Sep 21, 2020
10 tasks
fixes c88f3ad, which resulted in
qt 5.15 being used in pythonPackages, despite 5.14 being
declared, and adapts qutebrowser accordingly.

'callPackage { pkgs = pkgs // { … }; }' does not work, because
it does not take into account the recursive evaluation of nixpkgs:

`pkgs/development/interpreters/python/default.nix` calls
`pkgs/top-level/python-packages.nix` with `callPackage`.
Thus, even if the former gets passed the updated `pkgs`,
the latter always gets passed `pkgs.pkgs`.

For the change in the qt5 version to apply consistently, 'pkgs.extend'
must be used.

qutebrowser only used the right qt5 version (5.15) because all
pythonPackages used it anyway.
@zimbatm
Copy link
Member

zimbatm commented Nov 18, 2020

I was looking at the number of allocations in https://hydra.nixos.org/job/nixpkgs/trunk/metrics/metric/nix-env.qa.allocations and suspect that the use pkgs.extend bumped the evaluation size of 133MB.

I left a comment on the commit itself as well but probably more people should look into it.

@jorsn
Copy link
Member

jorsn commented Nov 22, 2020

I was looking at the number of allocations in https://hydra.nixos.org/job/nixpkgs/trunk/metrics/metric/nix-env.qa.allocations and suspect that the use pkgs.extend bumped the evaluation size of 133MB.

Why does the chart end on Oct 8? If you know the allocation size after edac19f, you know if the pkgs.extend here can be responsible for the increase.

Nevertheless, pkgs.extend is probably the only way to consistently override package sets consistently in subscopes like pythonInterpreters, and qt5 versions must be consistent in a scope.

Edit: The only way within the current fixed-point+scoping system.

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.

Calibre 4.23 doesn't start (SIGABRT)
3 participants