Re: 03/163: build/python: Add a new guix-pythonpath procedure.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi Hartmut,

Sorry for the delay.

Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Hi Maxim,
>
> many thanks for picking up this issue.
>> Indeed, I thought about the possibility to filter the GUIX_PYTHONPATH
>> entries based on their version at runtime after I wrote my initial
>> reply.  It makes life easier.  I've updated the
>> cu/farewell-to-pythonpath branch with this new way of doing things.
>
> I had a look at the first changes (only) and have some remarks:
>
> 1) Did I understand this correctly: `sitecustomize.py` is filtering
> GUIX_PYTHONPATH for all parts containing
> "/lib/pythonX.Y/site-packages" (with X.Y being Major.Minor)?

Yes.

> 2) This does not remove duplicates and does not honor .pth files in
> the respective directories - which might still be used. Thus
> site.addsitedir() should be called for adding the paths. This also
> takes care about duplicates.

I confess I didn't pay attention to .pth files, which mostly seemed like
legacy cruft to me; are they still used in the context of PEP 517 and
modern Python packaging?  The problem with calling site.addsitedir is
that it simply appends to sys.path.  We want to splice in the content of
GUIX_PYTHONPATH at a controlled location.

> 3) Empty part (…::…) are not handled.

GUIX_PYTHONPATH is not intended for end users; users can and should
still use PYTHONPATH, if they may.  For that reason, I think it's
preferable to keep it as lean as it can be, without burdening it with
unnecessary checks.  Empty sites also don't break anything, should they
appear for any reason:

$ PYTHONPATH="::::$PYTHONPATH" python -c "print('hello\n')"
hello

> 4) Since PYTHONPATH is evaluated prior to importing sitecustomize, any
> sitecustominze.py in the user's path will overwrite our file, thus
> inhibiting our paths to be added. Not sure this is what we want in Guix.

I asked guidance on the #python channel on freenode and was recommended
to use sitecustomize.py for this purpose; reading the doc here seems to
confirm our usage of it is as intended [0]:

    [...] After these path manipulations, an attempt is made to import a
    module named sitecustomize, which can perform arbitrary
    site-specific customizations. It is typically created by a system
    administrator in the site-packages directory.

The last sentence hints at that this is intended for the Python
installation side of things rather than end users.  I've never seen a
Python package with a sitecustomize.py at its root; if I did, I would
consider it bad practice.

> 5) When implementing the logic into site.py, the code could be
> simplified, Eg. You could patch a "getguixsitepackages" (based on
> getsitepackages) and a "addguixsitepackages" (based on
> addguixsitepackages) into site.py. Downside is that maybe we need
> different patches for different Python versions. Benefit is simpler
> code - no need to search the correct position in the list.

I initially went that route, but ended up choosing sitecustomize.py as
it is intended for this purpose and should be more robust in the face of
changes to Python's site.py module.

> 6) Please add some more comments to the code explaining the idea.

I was under the impression the code was concise enough to forego with
verbose explanations; I'd rather keep it this way.  But as the
implementer perhaps I'm not the right person to "see" this!  I would be
happy to consider a patch adding a few comments based on your fresh
perspective, if you think it is worth it.

> Some nitpicking:
>
>> python_root = os.path.realpath(sys.executable).split('/bin/')[0]
>
> There already is sys.prefix, which is also the base for the entry in
> sys.path (see top of site.py

Oh, true!

>> major_minor = '{}.{}'.format(sys.version_info[0], sys.version_info[1])
>
> major_minor = '{}.{}'.format(*sys.version_info)

Neat trick; although the above is a bit more explicit.

>>sys.path = sys.path[:index] + matching_sites + sys.path[index:]
>
> sys.path[index:index] = matching_sites

Neat also!  Thank you for pointing it out.

> I suggest using os.path.join(), os.path.split(), os.pathsep, etc. to
> be forward-compatible. Imagine we want to port Guix to another
> platform with different separators.

If Guix is ever ported to a system not using ':' as the path separator,
it'd be a concern for the search path specification that emits
GUIX_PYTHONPATH rather than here, no?

Thanks for the comments and showing the neat little Python tricks above;
I'd be happy to review a patch if you produce one with these changes and
extra explanatory comments.

Maxim

[0]  https://docs.python.org/3/library/site.html