GNU bug report logs - #29856
[PATCH core-updates] guix: python-build-system: Modify ".py" files in-place.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29856 in the body.
You can then email your comments to 29856 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: guix-patches <at> gnu.org
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>
Subject: [PATCH core-updates] guix: python-build-system: Modify ".py" files
 in-place.
Date: Tue, 26 Dec 2017 13:21:05 +0100

* guix/build/python-build-system.scm (wrap-python-program): New variable.
(wrap-program*): New variable.
(wrap): Use wrap-program*.
---
 guix/build/python-build-system.scm | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index dd07986b9..f5f6b07f8 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -25,6 +25,7 @@
   #:use-module (guix build utils)
   #:use-module (ice-9 match)
   #:use-module (ice-9 ftw)
+  #:use-module (ice-9 rdelim)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:export (%standard-phases
@@ -184,6 +185,32 @@ when running checks after installing the package."
                          configure-flags)))
     (call-setuppy "install" params use-setuptools?)))
 
+(define (wrap-python-program file-name vars)
+  "Wrap the given program as a Python script (in-place)"
+  (match vars
+    (("PYTHONPATH" 'prefix python-path)
+     (let ((pythonish-path (string-join python-path "', '")))
+       (with-atomic-file-replacement file-name
+         (lambda (in out)
+           (display (format #f "#!~a
+import sys
+sys.path = ['~a'] + sys.path
+" (which "python") pythonish-path) out)
+           (let loop ((line (read-line in 'concat)))
+             (if (eof-object? line)
+               #t
+               (begin
+                 (display line out)
+                 (loop (read-line in 'concat)))))))))))
+
+(define (wrap-program* file-name vars)
+  "Wrap the given program.
+   If FILE-NAME is ending in '.py', wraps it in a Python way.
+   Otherwise wraps it in a Bash way."
+  (if (string-suffix? ".py" file-name)
+    (wrap-python-program file-name vars)
+    (wrap-program file-name vars)))
+
 (define* (wrap #:key inputs outputs #:allow-other-keys)
   (define (list-of-files dir)
     (map (cut string-append dir "/" <>)
@@ -209,7 +236,7 @@ when running checks after installing the package."
                         (or (getenv "PYTHONPATH") ""))))))
     (for-each (lambda (dir)
                 (let ((files (list-of-files dir)))
-                  (for-each (cut wrap-program <> var)
+                  (for-each (cut wrap-program* <> var)
                             files)))
               bindirs)))
 

Message #8 received at 29856 <at> debbugs.gnu.org (full text, mbox):

From: Leo Famulari <leo <at> famulari.name>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: 29856 <at> debbugs.gnu.org
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:
 Modify ".py" files in-place.
Date: Tue, 26 Dec 2017 14:10:13 -0500

[Message part 1 (text/plain, inline)]

On Tue, Dec 26, 2017 at 01:21:05PM +0100, Danny Milosavljevic wrote:
> * guix/build/python-build-system.scm (wrap-python-program): New variable.
> (wrap-program*): New variable.
> (wrap): Use wrap-program*.

The idea here is to avoid renaming the Python executables to .foo-real,
right?

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 29856 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Leo Famulari <leo <at> famulari.name>
Cc: 29856 <at> debbugs.gnu.org
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:
 Modify ".py" files in-place.
Date: Wed, 27 Dec 2017 01:23:26 +0100

On Tue, 26 Dec 2017 14:10:13 -0500
Leo Famulari <leo <at> famulari.name> wrote:

> On Tue, Dec 26, 2017 at 01:21:05PM +0100, Danny Milosavljevic wrote:
> > * guix/build/python-build-system.scm (wrap-python-program): New variable.
> > (wrap-program*): New variable.
> > (wrap): Use wrap-program*.  
> 
> The idea here is to avoid renaming the Python executables to .foo-real,
> right?

Yes, especially since Python itself sometimes imports those and then it's tripping over the bash script when it expected a Python script.

Message #14 received at 29856 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Leo Famulari <leo <at> famulari.name>
Cc: 29856 <at> debbugs.gnu.org
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:
 Modify	".py" files in-place.
Date: Sun, 31 Dec 2017 16:02:30 +0100

[Message part 1 (text/plain, inline)]

Danny Milosavljevic <dannym <at> scratchpost.org> writes:

> On Tue, 26 Dec 2017 14:10:13 -0500
> Leo Famulari <leo <at> famulari.name> wrote:
>
>> On Tue, Dec 26, 2017 at 01:21:05PM +0100, Danny Milosavljevic wrote:
>> > * guix/build/python-build-system.scm (wrap-python-program): New variable.
>> > (wrap-program*): New variable.
>> > (wrap): Use wrap-program*.  
>> 
>> The idea here is to avoid renaming the Python executables to .foo-real,
>> right?
>
> Yes, especially since Python itself sometimes imports those and then it's tripping over the bash script when it expected a Python script.

I wonder if this will fix <https://bugs.gnu.org/29824>.  "meson" is not
installed with a .py extension, but I guess we can call wrap-program*
on it.

Would it work to peek at the shebang instead of the file extension?

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 29856 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: 29856 <at> debbugs.gnu.org, Leo Famulari <leo <at> famulari.name>
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:
 Modify ".py" files in-place.
Date: Sun, 31 Dec 2017 18:17:55 +0100

Hi Marius,

On Sun, 31 Dec 2017 16:02:30 +0100
Marius Bakke <mbakke <at> fastmail.com> wrote:

> I wonder if this will fix <https://bugs.gnu.org/29824>.  "meson" is not
> installed with a .py extension

That bugreport sounds as if the searched-for program is "meson.py".  But I tried to install meson while having 29856 applied.  Doesn't work.

>, but I guess we can call wrap-program* on it.

If you made sure the original wrapping thing didn't run, yeah.  That's why I did such an intrusive fix in the first place.

> Would it work to peek at the shebang instead of the file extension?

Probably, but I wanted it to be dead easy for core-updates.

Not sure whether, if we examined the content, there would be any false positives, empty files, dangling symlinks, special files etc which would need special handling.  Every change there rebuilds for several hours on my machine, so I'd like a sure-to-work block even when testing :)

Message #20 received at 29856 <at> debbugs.gnu.org (full text, mbox):

From: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
To: 29856 <at> debbugs.gnu.org
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Marius Bakke <mbakke <at> fastmail.com>
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:,
 Modify ".py" files in-place.
Date: Tue, 2 Jan 2018 17:13:15 +0100

[Message part 1 (text/plain, inline)]

Hi,

I'm afraid, this patch will lead to quite some serious problems:

  * It "kills" the doc-string, which must be the very first
    expression-statement in the program. This might be rarely used, but
  * it kills "from __future__ import", which must be the first import
    statement (or even the first statement after any doc-string) to work.

Fixing these issues would require to implement a language-aware scanner,
as discussed in
<https://lists.gnu.org/archive/html/guix-devel/2017-11/msg00022.html>.

Thus I suggest aiming to implement the solution discussed in that thread
(see esp.
<https://lists.gnu.org/archive/html/guix-devel/2017-11/msg00041.html>.

Beside of this, the patch suffers from some more issues. Sorry to say :-(

  * When converting PYTHONPATH into a list of python strings, these need
    to be quoted properly.
  * The description (commit-message) of the patch is much to terse. It
    should describe the the reason and implications. Esp. it should
    describe the case this is fixing.

BTW: The first analysis in <https://bugs.gnu.org/29824> was misleading.
I commented there.

-- 
Regards
Hartmut Goebel

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

[Message part 2 (text/html, inline)]

Message #23 received at 29856 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Hartmut Goebel <h.goebel <at> crazy-compilers.com>
Cc: Marius Bakke <mbakke <at> fastmail.com>, 29856 <at> debbugs.gnu.org
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:,
 Modify ".py" files in-place.
Date: Tue, 2 Jan 2018 18:26:27 +0100

Hi Hartmut,

thanks for the review!

On Tue, 2 Jan 2018 17:13:15 +0100
Hartmut Goebel <h.goebel <at> crazy-compilers.com> wrote:

>   * it kills "from __future__ import", which must be the first import
>     statement (or even the first statement after any doc-string) to work.

... oops.

> Thus I suggest aiming to implement the solution discussed in that thread
> (see esp.
> <https://lists.gnu.org/archive/html/guix-devel/2017-11/msg00041.html>.

I like that approach. Nice...

> Beside of this, the patch suffers from some more issues. Sorry to say :-(
> 
>   * When converting PYTHONPATH into a list of python strings, these need
>     to be quoted properly.

I agree.

>   * The description (commit-message) of the patch is much to terse. It
>     should describe the the reason and implications. Esp. it should
>     describe the case this is fixing.

Sure.

Message #30 received at 29856-done <at> debbugs.gnu.org (full text, mbox):

From: Ricardo Wurmus <rekado <at> elephly.net>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: Hartmut Goebel <h.goebel <at> crazy-compilers.com>,
 Marius Bakke <mbakke <at> fastmail.com>, 29856-done <at> debbugs.gnu.org
Subject: Re: [bug#29856] [PATCH core-updates] guix: python-build-system:,
 Modify ".py" files in-place.
Date: Mon, 04 Feb 2019 08:58:21 +0100

Danny Milosavljevic <dannym <at> scratchpost.org> writes:
>
> On Tue, 2 Jan 2018 17:13:15 +0100
> Hartmut Goebel <h.goebel <at> crazy-compilers.com> wrote:
>
>>   * it kills "from __future__ import", which must be the first import
>>     statement (or even the first statement after any doc-string) to work.
>
> ... oops.
>
>> Thus I suggest aiming to implement the solution discussed in that thread
>> (see esp.
>> <https://lists.gnu.org/archive/html/guix-devel/2017-11/msg00041.html>.
>
> I like that approach. Nice...
>
>> Beside of this, the patch suffers from some more issues. Sorry to say :-(
>> 
>>   * When converting PYTHONPATH into a list of python strings, these need
>>     to be quoted properly.
>
> I agree.
>
>>   * The description (commit-message) of the patch is much to terse. It
>>     should describe the the reason and implications. Esp. it should
>>     describe the case this is fixing.
>
> Sure.

I’m closing this in favour of #29951.

Thanks!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.