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

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 { };

This comment has been minimized.

@jorsn

jorsn Sep 18, 2020
Member

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.

This comment has been minimized.

@FRidh

FRidh Sep 18, 2020
Author Member

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.

This comment has been minimized.

@jorsn

jorsn Sep 18, 2020
Member

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), 25f146d (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

This comment has been minimized.

@jorsn

jorsn Sep 18, 2020
Member

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

This comment has been minimized.

@FRidh

FRidh Sep 22, 2020
Author Member

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.

This comment has been minimized.

@jorsn

jorsn Sep 22, 2020
Member

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

@FRidh FRidh force-pushed the FRidh:qt5 branch 2 times, most recently from a30a147 to 25f146d Sep 18, 2020
@FRidh FRidh mentioned this pull request Sep 18, 2020
6 of 10 tasks complete
@jorsn jorsn mentioned this pull request Sep 18, 2020
6 of 11 tasks complete
@veprbl veprbl linked an issue that may be closed by this pull request Sep 21, 2020
@nyanloutre nyanloutre mentioned this pull request Sep 21, 2020
4 of 10 tasks complete
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.
@FRidh FRidh force-pushed the FRidh:qt5 branch from 25f146d to 626ce32 Sep 22, 2020
@FRidh FRidh merged commit 7abb57c into NixOS:master Sep 22, 2020
2 of 3 checks passed
2 of 3 checks passed
tests tests
Details
action
Details
Wait for ofborg This failed status will be cleared when ofborg finishes eval.
Details
@erictapen erictapen mentioned this pull request Sep 26, 2020
0 of 10 tasks complete
@doronbehar doronbehar mentioned this pull request Oct 7, 2020
4 of 10 tasks complete
@FRidh FRidh mentioned this pull request Oct 7, 2020
99 of 186 tasks complete
@zimbatm
Copy link
Member

@zimbatm 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 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
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.