[core-updates-frozen] sitecustomize.py does not honor .pth files

  • Done
  • quality assurance status badge
Details
3 participants
  • Lars-Dominik Braun
  • Ludovic Courtès
  • Maxim Cournoyer
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
important
M
M
Maxim Cournoyer wrote on 4 Dec 2021 03:59
[core-updates-frozen] Some Python packages relying on .pth are broken
(name . bug-guix)(address . bug-guix@gnu.org)
871r2taxcm.fsf@gmail.com
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:

Toggle snippet (3 lines)
pdbpp_hijack_pdb.pth

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
M
M
Maxim Cournoyer wrote on 4 Dec 2021 04:15
control message for bug #52269
(address . control@debbugs.gnu.org)
87v9059i2i.fsf@gmail.com
retitle 52269 [core-updates-frozen] sitecustomize.py does not honor .pth files
quit
M
M
Maxim Cournoyer wrote on 4 Dec 2021 06:36
Re: bug#52269: [PATCH core-updates-frozen] sitecustomize does not honor .pth files
87mtlh9bjc.fsf@gmail.com
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:

Toggle snippet (7 lines)
$ 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

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.

Toggle snippet (5 lines)
$ guix shell --pure python python-pip python-pdbpp
[env]$ PYTHONPATH=. python3 -c 'import pip; print(pip.__version__)'
20.2.4

Next I created a dummy script to trigger run pdb:

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

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

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

Toggle snippet (7 lines)
$ PYTHONPATH=. python3 test.py
hello
[0] > /tmp/toto/test.py(7)<module>()
-> exit(1)
(Pdb++)

Better!
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.


* 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(-)

Toggle diff (41 lines)
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
L
L
Lars-Dominik Braun wrote on 6 Dec 2021 09:42
Re: [PATCH] sitecustomize.py: Honor .pth files.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 52269@debbugs.gnu.org)
Ya3NEAZXT5U0bPSH@noor.fritz.box
Hi Maxim,

Toggle quote (2 lines)
> +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.)

Toggle quote (9 lines)
> +# 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
L
L
Ludovic Courtès wrote on 13 Dec 2021 11:12
Re: bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
875yrs3jb7.fsf_-_@gnu.org
Hello Maxim,

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

Toggle quote (11 lines)
>>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’.
L
L
Ludovic Courtès wrote on 13 Dec 2021 11:12
control message for bug #52269
(address . control@debbugs.gnu.org)
874k7c3jaq.fsf@gnu.org
severity 52269 important
quit
M
M
Maxim Cournoyer wrote on 13 Dec 2021 15:10
Re: bug#52269: [core-updates-frozen] sitecustomize.py does not honor .pth files
(name . Ludovic Courtès)(address . ludo@gnu.org)
87czm0y4rl.fsf@gmail.com
Hi Ludovic,

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

Toggle quote (22 lines)
> 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.

Toggle quote (3 lines)
> 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
M
M
Maxim Cournoyer wrote on 13 Dec 2021 20:04
(name . Lars-Dominik Braun)(address . lars@6xq.net)(address . 52269@debbugs.gnu.org)
87ee6gcomm.fsf_-_@gmail.com
Hello,

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

Toggle quote (9 lines)
> 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:

Toggle snippet (5 lines)
__import__("pdb").set_trace()

print('hello')

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

Toggle snippet (17 lines)
$ 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

After the proposed change:

Toggle snippet (5 lines)
[env]$ PYTHONPATH=. GUIX_PYTHONPATH= python sample.py
> /home/maxim/proj/kinova/kts_robot/sample.py(5)<module>()
-> print('hello')

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

Toggle quote (14 lines)
>> +# 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
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.


* 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(-)

Toggle diff (39 lines)
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
L
L
Lars-Dominik Braun wrote on 16 Dec 2021 10:48
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 52269@debbugs.gnu.org)
YbsLh6wCsM+GzD7/@noor.fritz.box
Hi Maxim,

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

Toggle quote (4 lines)
> 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.

Toggle quote (5 lines)
> + # 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
M
M
Maxim Cournoyer wrote on 17 Dec 2021 15:41
(name . Lars-Dominik Braun)(address . lars@6xq.net)(address . 52269@debbugs.gnu.org)
87h7b770po.fsf@gmail.com
Hello!

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

Toggle quote (21 lines)
> 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
M
M
Maxim Cournoyer wrote on 17 Dec 2021 16:02
(name . Lars-Dominik Braun)(address . lars@6xq.net)(address . 52269-done@debbugs.gnu.org)
87czlv6zrn.fsf@gmail.com
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
Closed
?