NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.99k stars 14k forks source link

python3 "Found duplicated packages in closure" #255810

Closed evrim closed 10 months ago

evrim commented 1 year ago

Describe the bug

Hi there,

I've been having difficulties with the master lately.

Basically I had this custom python3.override that applies a patch to cython = super.cython.overridePythonAttrs(old: { patches = old.patches ++ [ ./some.interpreter.hack.patch ]; }); but lately unrelated packages started failing.

For example, here is a log for pyopenssl:

/nix/store/q2zpnf017gccs9w9mb0ikdqhkf0k5v82-catch_conflicts.py:1: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  import pkg_resources
Found duplicated packages in closure for dependency 'sphinx': 
  sphinx 5.3.0 (/nix/store/jwkrn9p7i0r5kkdpc39c9ywfzbmvmvd0-python3.10-sphinx-5.3.0/lib/python3.10/site-packages)
  sphinx 5.3.0 (/nix/store/7l4cj4g6ysi1dmz2yxl6nsq8abgiipai-python3.10-sphinx-5.3.0/lib/python3.10/site-packages)
Found duplicated packages in closure for dependency 'snowballstemmer': 
  snowballstemmer 2.2.0 (/nix/store/l5246ajqbmc48q792rf9xg3sqi0syfry-python3.10-snowballstemmer-2.2.0/lib/python3.10/site-packages)
  snowballstemmer 2.2.0 (/nix/store/zcrf0d6by8dyrs1zmqwixrbkxpr2gpkk-python3.10-snowballstemmer-2.2.0/lib/python3.10/site-packages)
Found duplicated packages in closure for dependency 'PyStemmer': 
  PyStemmer 2.2.0 (/nix/store/r33m03rghvpqgz75zc8gxsgkk3w9klz2-python3.10-pystemmer-2.2.0/lib/python3.10/site-packages)
  PyStemmer 2.2.0 (/nix/store/hh3pcdmca80bdvjg38s2ri9f448wvkg5-python3.10-pystemmer-2.2.0/lib/python3.10/site-packages)

Any ideas? No idea why it happens or how to approach the problem.

thnx.

evrim commented 1 year ago

Here is for pyjwt 2.8.0:

Found duplicated packages in closure for dependency 'sphinx': 
  sphinx 5.3.0 (/nix/store/jwkrn9p7i0r5kkdpc39c9ywfzbmvmvd0-python3.10-sphinx-5.3.0/lib/python3.10/site-packages)
  sphinx 5.3.0 (/nix/store/7l4cj4g6ysi1dmz2yxl6nsq8abgiipai-python3.10-sphinx-5.3.0/lib/python3.10/site-packages)
Found duplicated packages in closure for dependency 'snowballstemmer': 
  snowballstemmer 2.2.0 (/nix/store/l5246ajqbmc48q792rf9xg3sqi0syfry-python3.10-snowballstemmer-2.2.0/lib/python3.10/site-packages)
  snowballstemmer 2.2.0 (/nix/store/zcrf0d6by8dyrs1zmqwixrbkxpr2gpkk-python3.10-snowballstemmer-2.2.0/lib/python3.10/site-packages)
Found duplicated packages in closure for dependency 'PyStemmer': 
  PyStemmer 2.2.0 (/nix/store/r33m03rghvpqgz75zc8gxsgkk3w9klz2-python3.10-pystemmer-2.2.0/lib/python3.10/site-packages)
  PyStemmer 2.2.0 (/nix/store/hh3pcdmca80bdvjg38s2ri9f448wvkg5-python3.10-pystemmer-2.2.0/lib/python3.10/site-packages)
evrim commented 1 year ago

typeguard-2.13.3, wrapt 1.14.1 have similar errors. Maybe there is a new way of overriding cython that I am not aware of.

newAM commented 1 year ago

Happens to me too from overriding certifi to inject my own cert. sphinx shows the same error you have.

newAM commented 1 year ago

Removing sphinxHook in the packages failing to build fixes my issue, but that's not very practical to do for the volume of packages I need.

evrim commented 1 year ago

I've found another way to suppress the warnings hence the errors. Apply the following to the packages that throws conflicts in your override:

   pyopenssl = super.pyopenssl.overridePythonAttrs(old: {                                                                                                                                                                                 
        catchConflicts = false;                                                                                                                                                                                                              
      });  

Obviously this doesn't fix the double packages in the closure problem. Let me see if it works.

Artturin commented 1 year ago

Maybe what you're doing isn't changing pythonForBuild too. Are you using packageOverrides like the docs show?

Please post a reproducer which can be run in the nixpkgs dir

{ pkgs ? import ./. { } }:

...

Could be because of https://github.com/NixOS/nixpkgs/commit/a3916398458f9dee7efb6cc12bc2b1d28980d38e

will have to be investigated with nix-diff

CC @kip93

kip93 commented 1 year ago

The reason for me creating that PR was because of wrapt failing to cross compile due to the sphinx hook, so I tested that it both compiled and cross compiled. Plus beautiful soup since I removed the explicit workaround. So it's not the package themselves that are failing here.

From what I can see, wrapt uses sphinx-rtd-theme, which that propagates sphinx. So does the hook. So now you have 2 inputs propagating the same dependency. That's probably where the confusion is coming from?

newAM commented 1 year ago

Please post a reproducer which can be run in the nixpkgs dir

Certainly! Sorry I should have done this with the initial post.

# foo.nix
{pkgs ? import ./. {}}: let
  python3 = pkgs.python3.override {
    packageOverrides = final: prev: {
      certifi = prev.certifi.override {
        cacert = pkgs.cacert.override {
          # openssl genrsa -out my_ca.crt 2048
          extraCertificateFiles = [./my_ca.crt];
        };
      };
    };
  };
in
  python3.pkgs.pyjwt
newAM commented 1 year ago

I bisected the failure to a3916398458f9dee7efb6cc12bc2b1d28980d38e.

Not sure how to properly fix it. For now I just reverted that commit in a fork of the nixpkgs repo and things are working again.

kip93 commented 1 year ago

Well as I said that commit is just exposing an error somewhere else not the error itself, but if you're not cross compiling reverting it won't break anything for you.

newAM commented 1 year ago

Well as I said that commit is just exposing an error somewhere else not the error itself

Do you have a solution to fix this then? I'm happy to put in time to fix this on nixpkgs, I just don't know how to fix it aside from reverting the commit.

kip93 commented 1 year ago

Haven't had the time to properly look into it, but my intuition tells me that we should remove sphinx as a propagated build input in sphinx themes. I feel like a theme is just resource files, nothing more; having sphinx there does not make sense to me.

newAM commented 1 year ago

I tried to remove sphinx from sphinx-rtd-theme, but it is not so easily done. sphinx-rtd-theme uses the sphinx logging facilities, locale, and it has a number of places where HTML generation is dependent on sphinx version. Not impossible to remove, but it would likely break locale functionality.

Additionally sphinx-rtd-theme uses sphinxcontrib-jquery which uses sphinx and has similar difficult-to-remove dependencies.

kip93 commented 1 year ago

Ok, had a little time for debugging this morning, and found something interesting. While it shows a conflict, one of them does not seem to be in the closure.

$ nix why-depends -a /nix/store/n9nb3f5py32xqbgc11png2q6drdfzy5q-python3.10-pyjwt-2.8.0.drv /nix/store/z9p4x8rw1kd99lkg70rc7crw2n70jdb4-python3.10-sphinx-5.3.0.drv
'/nix/store/n9nb3f5py32xqbgc11png2q6drdfzy5q-python3.10-pyjwt-2.8.0.drv' does not depend on '/nix/store/z9p4x8rw1kd99lkg70rc7crw2n70jdb4-python3.10-sphinx-5.3.0.drv'
$ nix why-depends -a /nix/store/n9nb3f5py32xqbgc11png2q6drdfzy5q-python3.10-pyjwt-2.8.0.drv /nix/store/4nfkn104vk6q6c1h991nxn1l6dm1833v-python3.10-sphinx-5.3.0.drv
/nix/store/n9nb3f5py32xqbgc11png2q6drdfzy5q-python3.10-pyjwt-2.8.0.drv
└───/nix/store/9cs5cgli2pf71b3gpfr7n75i7mgq1npk-python3.10-sphinx-rtd-theme-1.2.0.drv
    ├───/nix/store/4nfkn104vk6q6c1h991nxn1l6dm1833v-python3.10-sphinx-5.3.0.drv
    ├───/nix/store/993c1vcna986z0fhikqqwiqd4h9z3lxq-python3.10-sphinxcontrib-jquery-3.0.0.drv
    │   └───/nix/store/4nfkn104vk6q6c1h991nxn1l6dm1833v-python3.10-sphinx-5.3.0.drv
    └───/nix/store/q8xsvmq2hvcj6z9klp6lbpk7fp1c4419-python3.10-readthedocs-sphinx-ext-2.2.0.drv
        └───/nix/store/4nfkn104vk6q6c1h991nxn1l6dm1833v-python3.10-sphinx-5.3.0.drv

So I went ahead and tried a little something

diff --git a/pkgs/development/python-modules/pyjwt/default.nix b/pkgs/development/python-modules/pyjwt/default.nix
index cef52b754ad..4005dc840d2 100644
--- a/pkgs/development/python-modules/pyjwt/default.nix
+++ b/pkgs/development/python-modules/pyjwt/default.nix
@@ -4,9 +4,7 @@
 , cryptography
 , pytestCheckHook
 , pythonOlder
-, sphinxHook
-, sphinx-rtd-theme
-, zope_interface
+, python
 }:

 buildPythonPackage rec {
@@ -31,7 +29,7 @@ buildPythonPackage rec {
     "doc"
   ];

-  nativeBuildInputs = [
+  nativeBuildInputs = with python.pythonForBuild.pkgs; [
     sphinxHook
     sphinx-rtd-theme
     zope_interface

aaand it just works :tm:. But I'm not sure why, since unless you're cross compiling these should be the same. And indeed, they seem to be

$ nix build .#python3.pkgs.sphinx --print-out-paths
/nix/store/nscmi1vzj2m3sm99060pxw5fkl9vgd70-python3.10-sphinx-5.3.0
$ nix build .#python3.pythonForBuild.pkgs.sphinx --print-out-paths
/nix/store/nscmi1vzj2m3sm99060pxw5fkl9vgd70-python3.10-sphinx-5.3.0

Same for the hook and the theme

$ nix build .#python3.pkgs.sphinxHook --print-out-paths
/nix/store/hs8fpi0cfsqlw03q844mkmavq22z05y6-python3.10-sphinx-hook
$ nix build .#python3.pythonForBuild.pkgs.sphinxHook --print-out-paths
/nix/store/hs8fpi0cfsqlw03q844mkmavq22z05y6-python3.10-sphinx-hook
$ nix build .#python3.pkgs.sphinx-rtd-theme --print-out-paths
/nix/store/2isqzc5axx7mgwvg1lxm22f6n6xfljcm-python3.10-sphinx-rtd-theme-1.2.0
$ nix build .#python3.pythonForBuild.pkgs.sphinx-rtd-theme --print-out-paths
/nix/store/2isqzc5axx7mgwvg1lxm22f6n6xfljcm-python3.10-sphinx-rtd-theme-1.2.0

Unfortunately don't have more time today to look into this, but maybe if by the weekend noone else has chimed in I can try to find where is the phantom sphinx coming from

Artturin commented 1 year ago

The issue could be that the packageOverrides aren't getting passed to pythonForBuild

Discorevee with a quick use of the reproducer and nix-diff that the extraCertificateFiles weren't overriden

kip93 commented 1 year ago

From what I can see the overrides are passed to pythonForBuild, so the why of the certificate not being there eludes me.

What I tested and seems to work is to not propagate sphinx, since most packages (with 4 exceptions) know to not depend on the propagated sphinx anyway and included either sphinx or sphinxHook explicitly. So this patch may be a better fix than the one above

diff --git a/pkgs/development/python-modules/sphinx-rtd-theme/default.nix b/pkgs/development/python-modules/sphinx-rtd-theme/default.nix
index 613f1998121..835e74bc992 100644
--- a/pkgs/development/python-modules/sphinx-rtd-theme/default.nix
+++ b/pkgs/development/python-modules/sphinx-rtd-theme/default.nix
@@ -27,10 +27,13 @@ buildPythonPackage rec {

   propagatedBuildInputs = [
     docutils
-    sphinx
     sphinxcontrib-jquery
   ];

+  buildInputs = [
+    sphinx
+  ];
+
   nativeBuildInputs = [
     pythonRelaxDepsHook
   ];

Tested for both native and cross compilation and they both build. Not sure how to check if the certificate is properly overriden tho.

Here's the 4 exceptions that needed to be patched (testing them rn, but don't see how they could fail) ```diff diff --git a/pkgs/development/python-modules/django-payments/default.nix b/pkgs/development/python-modules/django-payments/default.nix index 577a2ca5132..d9d7a5a9489 100644 --- a/pkgs/development/python-modules/django-payments/default.nix +++ b/pkgs/development/python-modules/django-payments/default.nix @@ -9,6 +9,7 @@ , pythonOlder , requests , setuptools-scm +, sphinx , sphinx-rtd-theme , stripe , xmltodict @@ -56,7 +57,7 @@ buildPythonPackage rec { all = [ braintree /* suds-community */ mercadopago cryptography xmltodict stripe ]; braintree = [ braintree ]; cybersource = [ /* suds-community */ ]; - docs = [ sphinx-rtd-theme ]; + docs = [ sphinx sphinx-rtd-theme ]; mercadopago = [ mercadopago ]; sagepay = [ cryptography ]; sofort = [ xmltodict ]; diff --git a/pkgs/development/python-modules/imantics/default.nix b/pkgs/development/python-modules/imantics/default.nix index 3b02ac44034..05ac7299d58 100644 --- a/pkgs/development/python-modules/imantics/default.nix +++ b/pkgs/development/python-modules/imantics/default.nix @@ -3,6 +3,7 @@ , lib , numpy , opencv3 +, sphinx , sphinx-rtd-theme , lxml , xmljson @@ -22,6 +23,7 @@ buildPythonPackage rec { propagatedBuildInputs = [ numpy opencv3 + sphinx sphinx-rtd-theme lxml xmljson diff --git a/pkgs/development/python-modules/omrdatasettools/default.nix b/pkgs/development/python-modules/omrdatasettools/default.nix index 4448a686e1a..5f2b51b7701 100644 --- a/pkgs/development/python-modules/omrdatasettools/default.nix +++ b/pkgs/development/python-modules/omrdatasettools/default.nix @@ -10,6 +10,7 @@ , pillow , pytestCheckHook , scikit-image +, sphinx , sphinx-rtd-theme , sympy , pandas @@ -39,6 +40,7 @@ buildPythonPackage rec { tqdm twine sympy + sphinx sphinx-rtd-theme pandas ipython diff --git a/pkgs/development/python-modules/pydash/default.nix b/pkgs/development/python-modules/pydash/default.nix index 14e7ffdfcd6..d05f6686c30 100644 --- a/pkgs/development/python-modules/pydash/default.nix +++ b/pkgs/development/python-modules/pydash/default.nix @@ -6,6 +6,7 @@ , mock , pytestCheckHook , pythonOlder +, sphinx , sphinx-rtd-theme }: @@ -26,6 +27,7 @@ buildPythonPackage rec { nativeCheckInputs = [ invoke mock + sphinx sphinx-rtd-theme pytestCheckHook ]; ```
kip93 commented 1 year ago

Ok, the builds took a while but this does not seem to break anything that wasn't already broken (3 out of 4 failed to be cross compiled, but nothing related to sphinx and failed even before this change).

Artturin commented 1 year ago

Does the hook even have to propagate sphinx instead of using only the binary

kip93 commented 1 year ago

With the current design, hooks can't not propagate inputs (I guess it's just a matter of adding the argument, but maybe it was done this way for a reason?).

Artturin commented 1 year ago

With the current design, hooks can't not propagate inputs (I guess it's just a matter of adding the argument, but maybe it was done this way for a reason?).

I mean just using substitutions to use a absolute path to the binary

kip93 commented 1 year ago

Oh, right, that makes sense. I'll go and make that change and create a PR (against staging of course) since this would be a large change and building it all on my PC would take too long.

SuperSandro2000 commented 10 months ago

Should be a lot better with #258223