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

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

[Danny Milosavljevic]

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

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

[Leo Famulari]

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

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Danny Milosavljevic]

On Tue, 26 Dec 2017 14:10:13 -0500
Leo Famulari <leo@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.

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

[Marius Bakke]

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

Danny Milosavljevic <dannym@scratchpost.org> writes:

> On Tue, 26 Dec 2017 14:10:13 -0500
> Leo Famulari <leo@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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Danny Milosavljevic]

Hi Marius,

On Sun, 31 Dec 2017 16:02:30 +0100
Marius Bakke <mbakke@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 :)

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

[Hartmut Goebel]

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

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@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |


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

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

[Danny Milosavljevic]

Hi Hartmut,

thanks for the review!

On Tue, 2 Jan 2018 17:13:15 +0100
Hartmut Goebel <h.goebel@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.

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

[Ricardo Wurmus]

Danny Milosavljevic <dannym@scratchpost.org> writes:
>
> On Tue, 2 Jan 2018 17:13:15 +0100
> Hartmut Goebel <h.goebel@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!