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

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

[Ludovic Courtès]

Hi!

guix-commits@gnu.org skribis:

> apteryx pushed a commit to branch cu/farewell-to-pythonpath
> in repository guix.

Woow!

> commit c3c5bdd2b0ea4588ecba424fdbf6df101d967d3d
> Author: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> AuthorDate: Fri Jan 22 09:34:33 2021 -0500
>
>     build/python: Add a new guix-pythonpath procedure.
>     
>     This procedure will be used in place where the static "PYTHONPATH" string used
>     to be.
>     
>     * guix/build/python-build-system.scm (guix-pythonpath): New procedure.

[...]

> +(define (guix-pythonpath inputs)
> +  "Derive the GUIX_PYTHONPATH_X_Y string , with the major and minor version
> +numbers substituted for X and Y, respectively."
> +  (let ((python (assoc-ref inputs "python")))
> +    (string-append "GUIX_PYTHONPATH_"
> +                   (string-replace-substring (python-version python) "." "_"))))

One pattern used elsewhere, notably for ‘GUIX_LOCPATH’, is to still keep
a single environment variable (that’d be ‘GUIX_PYTHONPATH’ here) but to
ensure that only MAJOR.MINOR sub-directories are taken into
consideration.

IOW, instead of having GUIX_PYTHONPATH_3_8 and GUIX_PYTHONPATH_2_7,
you’d have just GUIX_PYTHONPATH but Python 3.8 would only care about
3.8/ sub-directories while 2.7 would only care about 2.7/
sub-directories.

That’s perhaps easier to document and to deal with generally (you only
need to look at one specific environment variable).

Does that make sense?  I don’t know if it’s applicable in this case
though.

(I didn’t follow the PYTHONPATH discussion so apologies if this comment
is off!)

Anyhow, thanks for this long awaited change!

Ludo’.

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

[Maxim Cournoyer]

Hi Ludovic!

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

> Hi!
>
> guix-commits@gnu.org skribis:
>
>> apteryx pushed a commit to branch cu/farewell-to-pythonpath
>> in repository guix.
>
> Woow!
>
>> commit c3c5bdd2b0ea4588ecba424fdbf6df101d967d3d
>> Author: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> AuthorDate: Fri Jan 22 09:34:33 2021 -0500
>>
>>     build/python: Add a new guix-pythonpath procedure.
>>     
>>     This procedure will be used in place where the static "PYTHONPATH" string used
>>     to be.
>>     
>>     * guix/build/python-build-system.scm (guix-pythonpath): New procedure.
>
> [...]
>
>> +(define (guix-pythonpath inputs)
>> +  "Derive the GUIX_PYTHONPATH_X_Y string , with the major and minor version
>> +numbers substituted for X and Y, respectively."
>> +  (let ((python (assoc-ref inputs "python")))
>> +    (string-append "GUIX_PYTHONPATH_"
>> +                   (string-replace-substring (python-version python) "." "_"))))
>
> One pattern used elsewhere, notably for ‘GUIX_LOCPATH’, is to still keep
> a single environment variable (that’d be ‘GUIX_PYTHONPATH’ here) but to
> ensure that only MAJOR.MINOR sub-directories are taken into
> consideration.
>
> IOW, instead of having GUIX_PYTHONPATH_3_8 and GUIX_PYTHONPATH_2_7,
> you’d have just GUIX_PYTHONPATH but Python 3.8 would only care about
> 3.8/ sub-directories while 2.7 would only care about 2.7/
> sub-directories.
>
> That’s perhaps easier to document and to deal with generally (you only
> need to look at one specific environment variable).
>
> Does that make sense?  I don’t know if it’s applicable in this case
> though.

One of the limitations lifted by this change is the limitation of
running a single version of Python per profile.  As mentioned in past
discussions, it's not trivial ensuring that packages installed won't end
up pulling various versions of Python in a same profile, so it's better
to prepared for it :-).

Using a non-versioned GUIX_PYTHONPATH would not allow for this, unless
I'm missing something.

Thanks!

Maxim

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

[Ludovic Courtès]

Hi Maxim,

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

>>> +(define (guix-pythonpath inputs)
>>> +  "Derive the GUIX_PYTHONPATH_X_Y string , with the major and minor version
>>> +numbers substituted for X and Y, respectively."
>>> +  (let ((python (assoc-ref inputs "python")))
>>> +    (string-append "GUIX_PYTHONPATH_"
>>> +                   (string-replace-substring (python-version python) "." "_"))))
>>
>> One pattern used elsewhere, notably for ‘GUIX_LOCPATH’, is to still keep
>> a single environment variable (that’d be ‘GUIX_PYTHONPATH’ here) but to
>> ensure that only MAJOR.MINOR sub-directories are taken into
>> consideration.
>>
>> IOW, instead of having GUIX_PYTHONPATH_3_8 and GUIX_PYTHONPATH_2_7,
>> you’d have just GUIX_PYTHONPATH but Python 3.8 would only care about
>> 3.8/ sub-directories while 2.7 would only care about 2.7/
>> sub-directories.
>>
>> That’s perhaps easier to document and to deal with generally (you only
>> need to look at one specific environment variable).
>>
>> Does that make sense?  I don’t know if it’s applicable in this case
>> though.
>
> One of the limitations lifted by this change is the limitation of
> running a single version of Python per profile.  As mentioned in past
> discussions, it's not trivial ensuring that packages installed won't end
> up pulling various versions of Python in a same profile, so it's better
> to prepared for it :-).
>
> Using a non-versioned GUIX_PYTHONPATH would not allow for this, unless
> I'm missing something.

Sure, I definitely agree.

I’m advocating for a versioned environment variable too, but I’m
suggesting that versioning should not be part of the variable name.

In the ‘GUIX_LOCPATH’ example I gave, locale data is looked up not
directly in the search path entries, but instead in those entries
prefixed by the libc version.  See ‘glibc-versioned-locpath.patch’.

If possible, the proposed ‘GUIX_PYTHONPATH’ variable could work
similarly: “/MAJOR.MINOR” would automatically be appended to each search
path entry.

That way, both Python 2.7 and 3.8 can populate the same ‘GUIX_LOCPATH’
variable, and eventually 2.7 only picks 2.7 packages and 3.8 only picks
3.8 packages.

Does that make more sense?

Thanks,
Ludo’.

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

[Maxim Cournoyer]

Hi Ludo,

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

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>>> +(define (guix-pythonpath inputs)
>>>> +  "Derive the GUIX_PYTHONPATH_X_Y string , with the major and minor version
>>>> +numbers substituted for X and Y, respectively."
>>>> +  (let ((python (assoc-ref inputs "python")))
>>>> +    (string-append "GUIX_PYTHONPATH_"
>>>> +                   (string-replace-substring (python-version python) "." "_"))))
>>>
>>> One pattern used elsewhere, notably for ‘GUIX_LOCPATH’, is to still keep
>>> a single environment variable (that’d be ‘GUIX_PYTHONPATH’ here) but to
>>> ensure that only MAJOR.MINOR sub-directories are taken into
>>> consideration.
>>>
>>> IOW, instead of having GUIX_PYTHONPATH_3_8 and GUIX_PYTHONPATH_2_7,
>>> you’d have just GUIX_PYTHONPATH but Python 3.8 would only care about
>>> 3.8/ sub-directories while 2.7 would only care about 2.7/
>>> sub-directories.
>>>
>>> That’s perhaps easier to document and to deal with generally (you only
>>> need to look at one specific environment variable).
>>>
>>> Does that make sense?  I don’t know if it’s applicable in this case
>>> though.
>>
>> One of the limitations lifted by this change is the limitation of
>> running a single version of Python per profile.  As mentioned in past
>> discussions, it's not trivial ensuring that packages installed won't end
>> up pulling various versions of Python in a same profile, so it's better
>> to prepared for it :-).
>>
>> Using a non-versioned GUIX_PYTHONPATH would not allow for this, unless
>> I'm missing something.
>
> Sure, I definitely agree.
>
> I’m advocating for a versioned environment variable too, but I’m
> suggesting that versioning should not be part of the variable name.
>
> In the ‘GUIX_LOCPATH’ example I gave, locale data is looked up not
> directly in the search path entries, but instead in those entries
> prefixed by the libc version.  See ‘glibc-versioned-locpath.patch’.
>
> If possible, the proposed ‘GUIX_PYTHONPATH’ variable could work
> similarly: “/MAJOR.MINOR” would automatically be appended to each search
> path entry.
>
> That way, both Python 2.7 and 3.8 can populate the same ‘GUIX_LOCPATH’
> variable, and eventually 2.7 only picks 2.7 packages and 3.8 only picks
> 3.8 packages.
>
> Does that make more sense?

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.

Thanks for the suggestion!

Maxim

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

[Ludovic Courtès]

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

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

Awesome, thanks for taking the time to change all this.

Ludo’.

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

[Hartmut Goebel]

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)?

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.

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

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.

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.

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


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

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

major_minor = '{}.{}'.format(*sys.version_info)

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

sys.path[index:index] = matching_sites


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.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

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

[Maxim Cournoyer]

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

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

[Hartmut Goebel]

Hi schrieb Maxim,
> Sorry for the delay.

No problem, I reward this with another delay ;-) (Just kidding ;-)


> Hartmut Goebel <h.goebel@crazy-compilers.com> writes:
>
>> 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?  

I can't tell for sure. (I rinember to have seen a note about .pth still
being used in some setuptool-tick, but can't find it now.) Anyhow, since
site.py still supports it, I would prefer to be on the save side and
support it, to.

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

site.addsitedir takes an option second arguments where the paths are
collected into.


>> 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]:

IC.


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

Please add some comments. I had a hard time understanding it - and I was
not even sure I understood, see my question (1).


Another point, which came into my mind just now: Do virtuall
environments still work as expected? (With --system-site-packages,
packages in the profile are available, but venv-packages overwrite.
Without ----system-site-packages packages in the profile are *not*
available.)


-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

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

[Maxim Cournoyer]

Hi Harmut!

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

> Hi schrieb Maxim,
>> Sorry for the delay.
>
> No problem, I reward this with another delay ;-) (Just kidding ;-)
>
>
>> Hartmut Goebel <h.goebel@crazy-compilers.com> writes:
>>
>>> 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?
>
> I can't tell for sure. (I rinember to have seen a note about .pth still
> being used in some setuptool-tick, but can't find it now.) Anyhow, since
> site.py still supports it, I would prefer to be on the save side and
> support it, to.
>
>> 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.
>
> site.addsitedir takes an option second arguments where the paths are
> collected into.
>
>
>>> 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]:
>
> IC.
>
>
>>> 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.
>
> Please add some comments. I had a hard time understanding it - and I was
> not even sure I understood, see my question (1).

I'm spread thin right now, so if you could prepare a patch addressing
the above for me to review, that'd be much appreciated.  Otherwise I'll
get to it, but it won't be before some time.

> Another point, which came into my mind just now: Do virtuall
> environments still work as expected? (With --system-site-packages,
> packages in the profile are available, but venv-packages overwrite.
> Without ----system-site-packages packages in the profile are *not*
> available.)

Yes!  This was the main motivation behind this change.

Thank you,

Maxim

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

[Hartmut Goebel]

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

Am 14.03.21 um 01:58 schrieb Maxim Cournoyer:
>>>> 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.
>> Please add some comments. I had a hard time understanding it - and I was
>> not even sure I understood, see my question (1).
> I'm spread thin right now, so if you could prepare a patch addressing
> the above for me to review, that'd be much appreciated.  Otherwise I'll
> get to it, but it won't be before some time.
>
Sorry, I'm too short in time ATM - and I'm afraid this will not change
anytime soon. Just take my questions as a guidance what the comments
should answer to make others (me :-) understand what is going on.

Many thanks.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |


[-- Attachment #2: Type: text/html, Size: 1897 bytes --]