[00/14] Change python-build-system (fixes bug 20765)

Message ID 87bmz5ih9u.fsf@ike.i-did-not-set--mail-host-address--so-tickle-me
State New
Headers

Commit Message

Marius Bakke Sept. 30, 2016, 2:39 p.m. UTC
  Hartmut Goebel <h.goebel@crazy-compilers.com> writes:

> Am 28.09.2016 um 17:54 schrieb Marius Bakke:
>> Or push a branch somewhere?
>
> Branch is now available at 
> <https://gitlab.com/htgoebel/guix/tree/python-build-system>

Thanks a lot for doing this!

After adding a couple of patches I'm able to build many python packages.
Patch #2 mostly emulates NixOS "shim" setup.py[0], required for packages
using distutils instead of setuptools.

Some packages really don't like the new configure flags however (scons).
Perhaps we should have them as default, but if #:configure-flags is set,
let them be overridden?

Also some packages are missing a dependency on "python-py"[1].

Perhaps we can set up a Hydra channel to deal with the fallout?

Cheers,
Marius

0: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/python/run_setup.py

1: https://pypi.python.org/pypi/py/1.4.31
  

Comments

Hartmut Goebel Oct. 2, 2016, 9:05 a.m. UTC | #1
Am 30.09.2016 um 16:39 schrieb Marius Bakke:
> After adding a couple of patches I'm able to build many python packages.

Thanks for sharing. Sorry that you need #1 at all. I should have tested
my code another time prior to posting the patch.

> Patch #2 mostly emulates NixOS "shim" setup.py[0], required for packages
> using distutils instead of setuptools.

We should use the same code here pip uses [1,2], which is:

                "import setuptools, tokenize;__file__=%r;"
                "exec(compile(getattr(tokenize, 'open',
open)(__file__).read()"
                ".replace('\\r\\n', '\\n'), __file__, 'exec'))" %
self.setup_py

The main difference to your code is the use of token.open (Open a file
in read only mode using the encoding detected by detect_encoding() [3])
which we should use. Otherwise we will get encoding errors somewhen.

[1] https://github.com/pypa/pip/blob/8.1.2/pip/utils/setuptools_build.py
[2] https://github.com/pypa/pip/blob/8.1.2/pip/req/req_install.py#L849
[3] https://docs.python.org/3/library/tokenize.html#tokenize.open

> Some packages really don't like the new configure flags however (scons).
> Perhaps we should have them as default, but if #:configure-flags is set,
> let them be overridden?

Uff, this tricked me. I digged into scons and found it is redefining the
commands, much like setuptools do. For scons we could change the
setup.py, but where for sure are other packages having the same problem.
And there *may* be packages witch are incompatible with setuptools, too
(even if they should not).

I'm twofold regarding the behaviour of #:configure-flag. Emptying them
as soon as one passes some options flags is different to what other
builders are doing. E.g. gnu-build-system *adds* any option passed. So a
somewhat better solution would be to put
--single-version-external-managed and --root into the default
config-flags. But then if one needs to add options, this will become
complicated.

What about adding a flag #;use-setuptools, which defaults to #t?


> Also some packages are missing a dependency on "python-py"[1].
This is very curious, since python-setuptools does not have python-py as
input nor does it contain this package. Can you give an example please?


> Perhaps we can set up a Hydra channel to deal with the fallout?

Tonight Leo wrote that he will set up one.
  
Hartmut Goebel Oct. 2, 2016, 12:14 p.m. UTC | #2
Hi,

Am 02.10.2016 um 11:05 schrieb Hartmut Goebel:
> What about adding a flag #;use-setuptools, which defaults to #t?

I just pushed to <https://gitlab.com/htgoebel/guix/tree/python-build-system>
- Marius' changes
- updated shim-code
- Add option "#:use-setuptools?" (default true).
- Fix for scons (as an example)

git fetch https://gitlab.com/htgoebel/guix.git python-build-system
  
Ludovic Courtès Oct. 2, 2016, 2:24 p.m. UTC | #3
Hi!

Marius Bakke <mbakke@fastmail.com> skribis:

> Hartmut Goebel <h.goebel@crazy-compilers.com> writes:
>
>> Am 28.09.2016 um 17:54 schrieb Marius Bakke:
>>> Or push a branch somewhere?
>>
>> Branch is now available at 
>> <https://gitlab.com/htgoebel/guix/tree/python-build-system>
>
> Thanks a lot for doing this!
>
> After adding a couple of patches I'm able to build many python packages.
> Patch #2 mostly emulates NixOS "shim" setup.py[0], required for packages
> using distutils instead of setuptools.
>
> Some packages really don't like the new configure flags however (scons).
> Perhaps we should have them as default, but if #:configure-flags is set,
> let them be overridden?

In general it seems like a good pattern.

> Also some packages are missing a dependency on "python-py"[1].
>
> Perhaps we can set up a Hydra channel to deal with the fallout?

I haven’t really followed, but if the two of you agree on that, that’s
definitely something we can do!

> From a12000dd320cebeb920a4f790fe9206a2b6bda41 Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> Date: Thu, 29 Sep 2016 18:29:21 +0100
> Subject: [PATCH 1/2] guix: python-build-system: fix configure flag append
>  (followup to dba07a8d1)
>
> ---
>  guix/build/python-build-system.scm | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Seems reasonable (with proper commit log).

> From 84fa3e8be3d3d868ddb9278a96807086415b754d Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> Date: Thu, 29 Sep 2016 18:41:35 +0100
> Subject: [PATCH 2/2] guix: python-build-system: Import setuptools before
>  calling `setup.py'.
>
> This is needed for packages using "distutils" instead of "setuptools"
> since the former does not understand the
> "--single-version-externally-managed" flag. Also export __file__ since
> it will be unset when setup.py is called from python "exec".

Please move this explanation as a comment right above the newly-added
code.

> * guix/build/python-build-system.scm (call-setuppy): extend
> "python setup.py" call to import setuptools, export __file__, and
> call setup.py from setuptools python environment.

It might be worth adding a comment on the general context in the code
here.

Regardless, it’s a good idea to get feedback from Hartmut and whoever
else knows more about Python than myself before committing.  :-)

Speaking of which, could you create an account on savannah.gnu.org and
send your account name to the folks marked as “project admins” on
<https://savannah.gnu.org/projects/guix/>?  Please send it as an
OpenPGP-signed message, using the OpenPGP key that you will then use to
sign commits.  Also, make sure to upload said key to pgp.mit.edu and to
add it on Savannah too.  If anything is unclear, let me know.  :-)

TIA!

Ludo’.
  
Hartmut Goebel Oct. 2, 2016, 5:31 p.m. UTC | #4
Am 02.10.2016 um 16:24 schrieb Ludovic Courtès:
>
>> Subject: [PATCH 2/2] guix: python-build-system: Import setuptools before
>>  calling `setup.py'.
>>
>> This is needed for packages using "distutils" instead of "setuptools"
>> since the former does not understand the
>> "--single-version-externally-managed" flag. Also export __file__ since
>> it will be unset when setup.py is called from python "exec".
> Please move this explanation as a comment right above the newly-added
> code.

@marius: I already added this comment, see the fixups on my branch at
gitlab (which I'm going to clean up when we all agree on the result).
  
Leo Famulari Oct. 2, 2016, 6:01 p.m. UTC | #5
On Sun, Oct 02, 2016 at 07:31:01PM +0200, Hartmut Goebel wrote:
> Am 02.10.2016 um 16:24 schrieb Ludovic Courtès:
> >
> >> Subject: [PATCH 2/2] guix: python-build-system: Import setuptools before
> >>  calling `setup.py'.
> >>
> >> This is needed for packages using "distutils" instead of "setuptools"
> >> since the former does not understand the
> >> "--single-version-externally-managed" flag. Also export __file__ since
> >> it will be unset when setup.py is called from python "exec".
> > Please move this explanation as a comment right above the newly-added
> > code.
> 
> @marius: I already added this comment, see the fixups on my branch at
> gitlab (which I'm going to clean up when we all agree on the result).

Let us know when the two of you think the code is ready to be tested on
Hydra, and I will set it up.
  
Marius Bakke Oct. 2, 2016, 7:25 p.m. UTC | #6
On 2 October 2016 19:01:38 BST, Leo Famulari <leo@famulari.name> wrote:
>On Sun, Oct 02, 2016 at 07:31:01PM +0200, Hartmut Goebel wrote:
>> Am 02.10.2016 um 16:24 schrieb Ludovic Courtès:
>> >
>> >> Subject: [PATCH 2/2] guix: python-build-system: Import setuptools
>before
>> >>  calling `setup.py'.
>> >>
>> >> This is needed for packages using "distutils" instead of
>"setuptools"
>> >> since the former does not understand the
>> >> "--single-version-externally-managed" flag. Also export __file__
>since
>> >> it will be unset when setup.py is called from python "exec".
>> > Please move this explanation as a comment right above the
>newly-added
>> > code.
>> 
>> @marius: I already added this comment, see the fixups on my branch at
>> gitlab (which I'm going to clean up when we all agree on the result).
>
>Let us know when the two of you think the code is ready to be tested on
>Hydra, and I will set it up.

Just FYI: I will be offline for a couple of days while waiting for a newly ordered ARM chromebook (Asus C201).

If anyone wants to work on core/libreboot meanwhile that would be great ;-) 

I'll address the points raised Thursday or so. It makes sense to either apply these on core-updates or wait until it's merged before creating a channel/branch for Hydra IMO.
  
Hartmut Goebel Oct. 3, 2016, 9:31 a.m. UTC | #7
Am 30.09.2016 um 16:39 schrieb Marius Bakke:
> Also some packages are missing a dependency on "python-py"[1].

I found an example: python-pytest-cov fails in the "check" phase. This
is an undocumented requirement for the tests, resp. this is an
install-requirement for pytest, but pytest has "python-py" defined as a
normal input.

I also discovered that prior to my changes a .pth file was created in
some case where now it is not - as far as I've seen. I still need to
investigate this in detail.

I'll report when I'm done with this. Probably I will need to rework the
core of the patch-set.
  
Hartmut Goebel Oct. 4, 2016, 9:39 p.m. UTC | #8
Am 30.09.2016 um 16:39 schrieb Marius Bakke:

> Also some packages are missing a dependency on "python-py"[1].

FYI: I found an example: python-pytest-cov fails in the "check" phase.
This is an undocumented requirement for the tests, resp. this is an
install-requirement for pytest, but pytest has "python-py" defined as a
normal input.

I investigated this and found that the existing build-system was kind of
broken :-( Prior to my changes, a .pth file was created in
in many cases - while now it is never creates.

(For those not common to Python: .pth-files are an old way to specify
additional directories to be added to sys.path (PYTHONPATH). .pth-file
were rarely used until setuptools and easy-install came up. And now
since pip is the recommended install method, they are bound to vanish
again.)

This .pth file did not only point to the .egg-directory (or file, in
case of a zipped .egg), but also to the .eggs of the requirement of the
current package. In the case of pytest, the .pth file looked something
like thus:

   ./pytest-1.2.3.egg
   /gnu/store/…-python-py-3.2.1/lib/python3.4/site-packages/py-3.2.1.egg

Thus the .pth-file did "propagate" the inputs.

Due to my change the .pth-file is gone now, and thus all required inputs
need to be declared as propageded-inputs as uix.

I'm working on this, but it requires to have a look at many of the
packages' source to do it right and will take some days to finish.
  
Hartmut Goebel Oct. 10, 2016, 1:24 p.m. UTC | #9
Hi Leo,
> Let us know when the two of you think the code is ready to be tested on
> Hydra, and I will set it up.

I now have a patch-set ready for testing. The patches will still need
some clean-up, but the result should be okay so far.

Please set up a Hydra channel so we can test cross building.

My WIP is still available at <https://gitlab.com/htgoebel/guix/tree/python-build-system>
  
Leo Famulari Oct. 12, 2016, 2:29 p.m. UTC | #10
On Mon, Oct 10, 2016 at 03:24:24PM +0200, Hartmut Goebel wrote:
> Hi Leo,
> > Let us know when the two of you think the code is ready to be tested on
> > Hydra, and I will set it up.
> 
> I now have a patch-set ready for testing. The patches will still need
> some clean-up, but the result should be okay so far.
> 
> Please set up a Hydra channel so we can test cross building.
> 
> My WIP is still available at <https://gitlab.com/htgoebel/guix/tree/python-build-system>

I've added you to the Guix group on Savannah, so you should be able to
push a branch called 'wip-python-build-system'.

Please read the file 'HACKING' carefully, and ask if you are unsure
about anything.

I'll set up a Hydra jobset for the branch once it has been created.

Welcome! I'm looking forward to seeing this new Python build system in
action.

[0]
It can be called 'python-build-system' if you don't expect to rebase or
break the history of the branch. We use the 'wip-' prefix to signal that
clones of the branch may become invalid due to rewritten history.
  
Andreas Enge Oct. 12, 2016, 2:37 p.m. UTC | #11
On Wed, Oct 12, 2016 at 10:29:54AM -0400, Leo Famulari wrote:
> I'll set up a Hydra jobset for the branch once it has been created.

Maybe it would be good to wait until core-updates is merged? Hydra is
currently busy building that. Also, there is still the wip-python branch
on hydra, maybe this could then be deleted, or commits cherry-picked?

Andreas
  
Leo Famulari Oct. 12, 2016, 2:51 p.m. UTC | #12
On Wed, Oct 12, 2016 at 04:37:06PM +0200, Andreas Enge wrote:
> On Wed, Oct 12, 2016 at 10:29:54AM -0400, Leo Famulari wrote:
> > I'll set up a Hydra jobset for the branch once it has been created.
> 
> Maybe it would be good to wait until core-updates is merged? Hydra is
> currently busy building that. Also, there is still the wip-python branch
> on hydra, maybe this could then be deleted, or commits cherry-picked?

Yes, I was going to coordinate with the rest of the admins to schedule
this Python rebuild. I think it should happen after this core-updates
cycle.

The wip-python branch is on hold until we have Python 3.5 in master. I
found some significant differences in how Python 3.4 and 3.5 affect
building, and I didn't want to juggle three different Python package
graphs: current master, current core-updates, and wip-pytohn.

The core Python packages are *extremely* tangled up in each other. Help
wanted, and I do plan to tackle it after the next release, and
potentially after this new build system is merged.

It depends on how quickly the new build system looks ready for master.
We could even restart wip-python on wip-python-build-system, since I
didn't make concrete progress on wip-python, unfortunately.
  
Vincent Legoll Oct. 13, 2016, 8:17 a.m. UTC | #13
Hello,

On Wed, Oct 12, 2016 at 4:51 PM, Leo Famulari <leo@famulari.name> wrote:
> The core Python packages are *extremely* tangled up in each other. Help
> wanted

Can you explain a bit what that would involve, what one can do to help, etc...
  

Patch

From 84fa3e8be3d3d868ddb9278a96807086415b754d Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Thu, 29 Sep 2016 18:41:35 +0100
Subject: [PATCH 2/2] guix: python-build-system: Import setuptools before
 calling `setup.py'.

This is needed for packages using "distutils" instead of "setuptools"
since the former does not understand the
"--single-version-externally-managed" flag. Also export __file__ since
it will be unset when setup.py is called from python "exec".

* guix/build/python-build-system.scm (call-setuppy): extend
"python setup.py" call to import setuptools, export __file__, and
call setup.py from setuptools python environment.
---
 guix/build/python-build-system.scm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index d07b83f..4362c48 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -42,7 +42,11 @@ 
       (begin
          (format #t "running \"python setup.py\" with command ~s and parameters ~s~%"
                 command params)
-         (zero? (apply system* "python" "setup.py" command params)))
+         (zero? (apply system* "python"
+                       "-c" (string-append "import setuptools;"
+                                           "__file__='setup.py';"
+                                           "exec(open('setup.py').read())")
+                       command params)))
       (error "no setup.py found")))
 
 (define* (build #:rest empty)
-- 
2.10.0