bug#52269: [core-updates-frozen] Some Python packages relying on .pth are broken

bug#52269: [core-updates-frozen] Some Python packages relying on .pth are broken

[Maxim Cournoyer]

Hello Guix,

This was already something Harmut noted during their review of the
site.py loader (that it should honor .pth files), but at the time I
wasn't aware of a Python package that still made use of that mechanism
and thought it was legacy.

To my dismay it seems to be used by the tool 'pdbpp', which is an
improved pdb (debugger) for Python; using core-updates-frozen I noticed
that it was no longer in use; looking at its installed files I see:

--8<---------------cut here---------------start------------->8---
pdbpp_hijack_pdb.pth
--8<---------------cut here---------------end--------------->8---

So I'm guessing that because the new loader doesn't handle .pth files
its "hijacking" technique doesn't work.

Unfortunately touching this site.py file would causes a massive rebuild
(of the whole Python world).  Hopefully this use of .pth is a rare
occurrence and can be worked around.

Thanks,

Maxim

bug#52269: [PATCH core-updates-frozen] sitecustomize does not honor .pth files

[Maxim Cournoyer]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

tags 52269 patch
thanks

Hi!

The following patch fixes it.  I used site.addsitedir but ensured the
correct ordering of sys.path (we need to make the Guix-installed
packages appear before Python's own site-packages directory otherwise we
wouldn't be able to override its bundled packages such as 'pip').

Here's how I tested:

Copy the sitecustomize.py file to the current directory, then:

--8<---------------cut here---------------start------------->8---
$ pip --version
pip 21.1.3 from $HOME/.guix-profile/lib/python3.9/site-packages/pip (python 3.9)

$ guix show python-pip | recsel -p version
version: 20.2.4
--8<---------------cut here---------------end--------------->8---

Ensure installed pip still overrides Python's own.  PYTHONPATH=. forces
the sitecustomize.py file in the CWD to take precedence over the one
currently installed along Python.

--8<---------------cut here---------------start------------->8---
$ guix shell --pure python python-pip python-pdbpp
[env]$ PYTHONPATH=. python3 -c 'import pip; print(pip.__version__)'
20.2.4
--8<---------------cut here---------------end--------------->8---

Next I created a dummy script to trigger run pdb:

#file: test.py
print('hello')
import pdb; pdb.set_trace()

--8<---------------cut here---------------start------------->8---
$ guix shell --pure python python-pip python-pdbpp
[env]$ python3 test.py
hello
> /tmp/toto/test.py(7)<module>()
-> exit(1)
(Pdb)
--8<---------------cut here---------------end--------------->8---

This is the current bug; this is the regular Pdb, not Pdbpp.  Let's
force our revised sitecustomize.py file:

--8<---------------cut here---------------start------------->8---
$ PYTHONPATH=. python3 test.py
hello
[0] > /tmp/toto/test.py(7)<module>()
-> exit(1)
(Pdb++)
--8<---------------cut here---------------end--------------->8---

Better!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-sitecustomize.py-Honor-.pth-files.patch --]
[-- Type: text/x-patch, Size: 2208 bytes --]

From 762357609270ab016236d22999ae5cfc3fe4ff28 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Fri, 3 Dec 2021 22:36:26 -0500
Subject: [PATCH] sitecustomize.py: Honor .pth files.

Fixes <https://issues.guix.gnu.org/52269>.

* gnu/packages/aux-files/python/sitecustomize.py: Use site.addsitedirs to add
the site directories; this takes care of the .pth files.  Make sure the added
items still appear before Python's own 'site-packages' directory.
---
 .../aux-files/python/sitecustomize.py         | 24 ++++++++++++++-----
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/gnu/packages/aux-files/python/sitecustomize.py b/gnu/packages/aux-files/python/sitecustomize.py
index 71e328b9ac..bdaaa8e9e2 100644
--- a/gnu/packages/aux-files/python/sitecustomize.py
+++ b/gnu/packages/aux-files/python/sitecustomize.py
@@ -18,6 +18,7 @@
 # along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import site
 import sys
 
 # Commentary:
@@ -47,9 +48,20 @@ all_sites_norm = [os.path.normpath(p) for p in all_sites_raw]
 matching_sites = [p for p in all_sites_norm
                   if p.endswith(site_packages_prefix)]
 
-# Insert sites matching the current version into sys.path, right before
-# Python's own site.  This way, the user can override the libraries provided
-# by Python itself.
-sys_path_absolute = [os.path.realpath(p) for p in sys.path]
-index = sys_path_absolute.index(python_site)
-sys.path[index:index] = matching_sites
+if not matching_sites:
+    exit(0)
+
+# Deduplicate the entries, append them to sys.path, and handle any .pth files
+# they contain.
+for s in matching_sites:
+    site.addsitedir(s)
+
+# Move the entries that were appended to sys.path in front of Python's own
+# site-packages directory.  This enables Guix packages to override Python's
+# bundled packages, such as 'pip'.
+python_site_index = sys.path.index(python_site)
+new_site_start_index = sys.path.index(matching_sites[0])
+if python_site_index < new_site_start_index:
+    sys.path = (sys.path[:python_site_index]
+                + sys.path[new_site_start_index:]
+                + sys.path[python_site_index:new_site_start_index])
-- 
2.34.0

bug#52269: [PATCH] sitecustomize.py: Honor .pth files.

[Lars-Dominik Braun]

Hi Maxim,

> +if not matching_sites:
> +    exit(0)
are you sure about using `exit()` here? sitecustomize.py is imported
during startup and this would simply quit the Python interpreter if
GUIX_PYTHONPATH is not set, wouldn’t it? (Can’t test the change
unfortunately, because it’s a massive rebuild.)

> +# Move the entries that were appended to sys.path in front of Python's own
> +# site-packages directory.  This enables Guix packages to override Python's
> +# bundled packages, such as 'pip'.
> +python_site_index = sys.path.index(python_site)
> +new_site_start_index = sys.path.index(matching_sites[0])
> +if python_site_index < new_site_start_index:
> +    sys.path = (sys.path[:python_site_index]
> +                + sys.path[new_site_start_index:]
> +                + sys.path[python_site_index:new_site_start_index])
This is unrelated to the pdb issue, right? I see that it’s necessary
right now, but as suggested in #46848 I’d prefer unbundling
setuptools/pip from python. (I’ll send a v3 of the patchset at some
point.)

Cheers,
Lars

bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files

[Ludovic Courtès]

Hello Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>>From 762357609270ab016236d22999ae5cfc3fe4ff28 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Fri, 3 Dec 2021 22:36:26 -0500
> Subject: [PATCH] sitecustomize.py: Honor .pth files.
>
> Fixes <https://issues.guix.gnu.org/52269>.
>
> * gnu/packages/aux-files/python/sitecustomize.py: Use site.addsitedirs to add
> the site directories; this takes care of the .pth files.  Make sure the added
> items still appear before Python's own 'site-packages' directory.

I had completely overlooked this patch.

Lars had useful comments about it.

Do we need to address this before we merge ‘core-updates-frozen’ into
‘master’?

If so, what changes need to be made to the patch before it can be
applied?

TIA!

Ludo’.

bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files

[Maxim Cournoyer]

Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

> Hello Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>>From 762357609270ab016236d22999ae5cfc3fe4ff28 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> Date: Fri, 3 Dec 2021 22:36:26 -0500
>> Subject: [PATCH] sitecustomize.py: Honor .pth files.
>>
>> Fixes <https://issues.guix.gnu.org/52269>.
>>
>> * gnu/packages/aux-files/python/sitecustomize.py: Use site.addsitedirs to add
>> the site directories; this takes care of the .pth files.  Make sure the added
>> items still appear before Python's own 'site-packages' directory.
>
> I had completely overlooked this patch.
>
> Lars had useful comments about it.
>
> Do we need to address this before we merge ‘core-updates-frozen’ into
> ‘master’?

The only reason I'm on the fence about it is that it causes a big
rebuild.  But rebuilding aside, I believe it'd be nice to have it in.
I've only spotted one package affected so far (python-pdbpp), but there
may be others.

> If so, what changes need to be made to the patch before it can be
> applied?

I'll try having a look today.

Thanks,

Maxim

bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files

[Maxim Cournoyer]

[-- Attachment #1: Type: text/plain, Size: 3518 bytes --]

Hello,

Lars-Dominik Braun <lars@6xq.net> writes:

> Hi Maxim,
>
>> +if not matching_sites:
>> +    exit(0)
> are you sure about using `exit()` here? sitecustomize.py is imported
> during startup and this would simply quit the Python interpreter if
> GUIX_PYTHONPATH is not set, wouldn’t it? (Can’t test the change
> unfortunately, because it’s a massive rebuild.)

You can test it by placing the new sitecustomize.py file in the current
directory, and then:

$ guix shell python-wrapper python-pdbpp

[env]$ $ PYTHONPATH=. GUIX_PYTHONPATH= python sample.py

where sample.py contains something like:

--8<---------------cut here---------------start------------->8---
__import__("pdb").set_trace()

print('hello')
--8<---------------cut here---------------end--------------->8---

Indeed, when GUIX_PYTHONPATH is unset or matching_sites is empty, it
exit with 0 as you expected:

--8<---------------cut here---------------start------------->8---
$ PYTHONPATH=. GUIX_PYTHONPATH= python sample.py
Fatal Python error: init_import_site: Failed to import the site module
Python runtime state: initialized
Traceback (most recent call last):
  File "/gnu/store/p5fgysbcnnp8b1d91mrvjvababmczga0-python-3.9.6/lib/python3.9/site.py", line 589, in <module>
    main()
  File "/gnu/store/p5fgysbcnnp8b1d91mrvjvababmczga0-python-3.9.6/lib/python3.9/site.py", line 582, in main
    execsitecustomize()
  File "/gnu/store/p5fgysbcnnp8b1d91mrvjvababmczga0-python-3.9.6/lib/python3.9/site.py", line 521, in execsitecustomize
    import sitecustomize
  File "/home/maxim/proj/kinova/kts_robot/sitecustomize.py", line 52, in <module>
    exit(0)
  File "/gnu/store/p5fgysbcnnp8b1d91mrvjvababmczga0-python-3.9.6/lib/python3.9/_sitebuiltins.py", line 26, in __call__
    raise SystemExit(code)
SystemExit: 0
--8<---------------cut here---------------end--------------->8---

After the proposed change:

--8<---------------cut here---------------start------------->8---
[env]$ PYTHONPATH=. GUIX_PYTHONPATH= python sample.py
> /home/maxim/proj/kinova/kts_robot/sample.py(5)<module>()
-> print('hello')
--8<---------------cut here---------------end--------------->8---

There's no longer pdbpp because of clearing GUIX_PYTHONPATH but at least
it doesn't crash :-).

>> +# Move the entries that were appended to sys.path in front of Python's own
>> +# site-packages directory.  This enables Guix packages to override Python's
>> +# bundled packages, such as 'pip'.
>> +python_site_index = sys.path.index(python_site)
>> +new_site_start_index = sys.path.index(matching_sites[0])
>> +if python_site_index < new_site_start_index:
>> +    sys.path = (sys.path[:python_site_index]
>> +                + sys.path[new_site_start_index:]
>> +                + sys.path[python_site_index:new_site_start_index])
> This is unrelated to the pdb issue, right? I see that it’s necessary
> right now, but as suggested in #46848 I’d prefer unbundling
> setuptools/pip from python. (I’ll send a v3 of the patchset at some
> point.)

Previously the Guix-provided paths were directly spliced at the right
location; now using 'site.addsitedir' simply appends them, which
requires manual fiddling afterward.

I agree that after it's un-bundled it shouldn't be necessary anymore, but
let's keep this change for core-updates along work on the 517
python-build-system (I'll try having a look to it after the next release
it out -- ping me otherwise).

Thank you,

Maxim

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-sitecustomize.py-Honor-.pth-files.patch --]
[-- Type: text/x-patch, Size: 2241 bytes --]

From 49f0d2a493b868b9414ea10c7a676cf8404e1bca Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Fri, 3 Dec 2021 22:36:26 -0500
Subject: [PATCH] sitecustomize.py: Honor .pth files.

Fixes <https://issues.guix.gnu.org/52269>.

* gnu/packages/aux-files/python/sitecustomize.py: Use site.addsitedirs to add
the site directories; this takes care of the .pth files.  Make sure the added
items still appear before Python's own 'site-packages' directory.
---
 .../aux-files/python/sitecustomize.py         | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/gnu/packages/aux-files/python/sitecustomize.py b/gnu/packages/aux-files/python/sitecustomize.py
index 71e328b9ac..e2348e0356 100644
--- a/gnu/packages/aux-files/python/sitecustomize.py
+++ b/gnu/packages/aux-files/python/sitecustomize.py
@@ -18,6 +18,7 @@
 # along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import site
 import sys
 
 # Commentary:
@@ -47,9 +48,18 @@ all_sites_norm = [os.path.normpath(p) for p in all_sites_raw]
 matching_sites = [p for p in all_sites_norm
                   if p.endswith(site_packages_prefix)]
 
-# Insert sites matching the current version into sys.path, right before
-# Python's own site.  This way, the user can override the libraries provided
-# by Python itself.
-sys_path_absolute = [os.path.realpath(p) for p in sys.path]
-index = sys_path_absolute.index(python_site)
-sys.path[index:index] = matching_sites
+if matching_sites:
+    # Deduplicate the entries, append them to sys.path, and handle any
+    # .pth files they contain.
+    for s in matching_sites:
+        site.addsitedir(s)
+
+    # Move the entries that were appended to sys.path in front of
+    # Python's own site-packages directory.  This enables Guix
+    # packages to override Python's bundled packages, such as 'pip'.
+    python_site_index = sys.path.index(python_site)
+    new_site_start_index = sys.path.index(matching_sites[0])
+    if python_site_index < new_site_start_index:
+        sys.path = (sys.path[:python_site_index]
+                    + sys.path[new_site_start_index:]
+                    + sys.path[python_site_index:new_site_start_index])
-- 
2.34.0

bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files

[Lars-Dominik Braun]

Hi Maxim,

> You can test it by placing the new sitecustomize.py file in the current
> directory, and then:
that works, thanks!

> I agree that after it's un-bundled it shouldn't be necessary anymore, but
> let's keep this change for core-updates along work on the 517
> python-build-system (I'll try having a look to it after the next release
> it out -- ping me otherwise).
Sure.

> +    # Move the entries that were appended to sys.path in front of
> +    # Python's own site-packages directory.  This enables Guix
> +    # packages to override Python's bundled packages, such as 'pip'.
> +    python_site_index = sys.path.index(python_site)
> +    new_site_start_index = sys.path.index(matching_sites[0])
One more nitpick: list.index() will raise a ValueError if the requested
value does not exist. I believe setting GUIX_PYTHONPATH=/nonexistent
will trigger this.

Cheers,
Lars

bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files

[Maxim Cournoyer]

Hello!

Lars-Dominik Braun <lars@6xq.net> writes:

> Hi Maxim,
>
>> You can test it by placing the new sitecustomize.py file in the current
>> directory, and then:
> that works, thanks!
>
>> I agree that after it's un-bundled it shouldn't be necessary anymore, but
>> let's keep this change for core-updates along work on the 517
>> python-build-system (I'll try having a look to it after the next release
>> it out -- ping me otherwise).
> Sure.
>
>> +    # Move the entries that were appended to sys.path in front of
>> +    # Python's own site-packages directory.  This enables Guix
>> +    # packages to override Python's bundled packages, such as 'pip'.
>> +    python_site_index = sys.path.index(python_site)
>> +    new_site_start_index = sys.path.index(matching_sites[0])
> One more nitpick: list.index() will raise a ValueError if the requested
> value does not exist. I believe setting GUIX_PYTHONPATH=/nonexistent
> will trigger this.

It doesn't break when I try it here:

$ PYTHONPATH=. GUIX_PYTHONPATH=/nonexistent python sample.py

Also, messing with GUIX_PYTHONPATH is something users shouldn't do
unless they really know what they are doing, in my opinion.  It's
intended as Guix's own mechanism to discover Python packages.  Users can
and should still use PYTHONPATH if they want to mess with Python's
module search path.

Thank you!

Maxim

bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files

[Maxim Cournoyer]

Hi,

I've cherry-picked this commit to the version-1.4.0 branch.  I'll amass
some fixes there and then later have Cuirass build it.

Closing.

Thanks for the review!

Maxim