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

  • Done
  • quality assurance status badge
Details
5 participants
  • Danny Milosavljevic
  • Hartmut Goebel
  • Leo Famulari
  • Marius Bakke
  • Ricardo Wurmus
Owner
unassigned
Submitted by
Danny Milosavljevic
Severity
normal
D
D
Danny Milosavljevic wrote on 26 Dec 2017 13:21
(address . guix-patches@gnu.org)(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
20171226122105.19156-1-dannym@scratchpost.org
* 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(-)

Toggle diff (53 lines)
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)))
L
L
Leo Famulari wrote on 26 Dec 2017 20:10
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 29856@debbugs.gnu.org)
20171226191013.GE1413@jasmine.lan
On Tue, Dec 26, 2017 at 01:21:05PM +0100, Danny Milosavljevic wrote:
Toggle quote (4 lines)
> * 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?
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEsFFZSPHn08G5gDigJkb6MLrKfwgFAlpCnpUACgkQJkb6MLrK
fwh7vw/8C8gRrXHE1ziXAB4mAn2EgJp12DvRXOoS4VUE6UjPp2dC9hd+jqjbEmbq
PV2JnGUiCyIz4I5Umk5Vxd3NdsuNeozJSHsWRVMLhsrtq+lBi8mc0XPFkWvPVPhx
YVKSHyE/urFwxTu6pyqtmKVRIJWdffWNpPL8iVP7x0OsjC/SgQ3ztfjpbctzLWEJ
a4KbSMvwY8PcDKqh+hKLrMC+BgnM86Gadj+dpyDzaI8jhMPaX50z/T6emiGS0JeH
ntlgl7zX36JjwBBSR8sczgaYjkAZTM3Vfxwnsdi/Tqil9MHI4DVwCcDfA3VPov5o
mnGWX0U+jDghmshe+5RjU/rNoQRHIeM9T9VA8wCyKKDJu25mTMzjOiff8ln2vnJL
Tr0nDBUC9OT3XTWVtGbCxVSbbxR+1K4YdimO+qxtynAheqL2VjKTvdZ9dGUwFObO
BMuqYYKymPoMjPfqSs35oGUUC9HT4xEtzwTvVIY6rh5y5dV6FVz0zTWngggTrBDr
9YlQxIap8+OoUYMuVakO6rddFzPAE0cYFItuuspriY/7Bxp5lVhmCqbL06jQJQyg
4y76TVFVGQZH/e6QLv3Yz6tx6PWjtoF1Suy+G/ef+5CqKau8qfA8B5RY1E1ErD+r
I/rcdaw7YOrY6ujy9YBhZTFPOa+BMH/VYcqWu/gorUQMQLBesLI=
=aE4o
-----END PGP SIGNATURE-----


D
D
Danny Milosavljevic wrote on 27 Dec 2017 01:23
(name . Leo Famulari)(address . leo@famulari.name)(address . 29856@debbugs.gnu.org)
20171227012326.5aa8a762@scratchpost.org
On Tue, 26 Dec 2017 14:10:13 -0500
Leo Famulari <leo@famulari.name> wrote:

Toggle quote (8 lines)
> 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.
M
M
Marius Bakke wrote on 31 Dec 2017 16:02
Re: [bug#29856] [PATCH core-updates] guix: python-build-system: Modify ".py" files in-place.
(address . 29856@debbugs.gnu.org)
87d12vaqk9.fsf@fastmail.com
Danny Milosavljevic <dannym@scratchpost.org> writes:

Toggle quote (13 lines)
> 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?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAlpI/AYACgkQoqBt8qM6
VPrrHQf+KiR+FFQjWUMT1Klcqz+LzuZehrNcUwxFUeBapTRPn8IlZKo47EMD09Lb
v2N6ODeE2F8A4+xX35PrD0dZTTnHNGpSmYoePMO6SyPI0GHq6cAMsXASpWKPQJDy
cB3QsN4MG7IBhmIPf22VKESudzuLHBOZmmuG5CLYo6dfFdYhcj6chh6I6wDvqOoc
oxcxbfwyjKI058TIFaQb4OYFfam5LkAtKmm5epQ351ttKPpD8e29aOkpV4WpzN6G
v7qHClHE3+p/caNSTYAxAKVlUBI1a06poea1imUU6XR6eVpAqdgT+5OgGzT+y+rk
Fw8M4naroPxuRkbobfc0qUd8YVmAiw==
=unNI
-----END PGP SIGNATURE-----

D
D
Danny Milosavljevic wrote on 31 Dec 2017 18:17
Re: [bug#29856] [PATCH core-updates] guix: python-build-system: Modify ".py" files in-place.
(name . Marius Bakke)(address . mbakke@fastmail.com)
20171231181755.7fe64e7e@scratchpost.org
Hi Marius,

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

Toggle quote (3 lines)
> 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.

Toggle quote (2 lines)
>, 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.

Toggle quote (2 lines)
> 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 :)
H
H
Hartmut Goebel wrote on 2 Jan 2018 17:13
Re: [bug#29856] [PATCH core-updates] guix: python-build-system:, Modify ".py" files in-place.
(address . 29856@debbugs.gnu.org)
86b45cc4-f8ef-95da-87f7-397db19c424e@crazy-compilers.com
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

Thus I suggest aiming to implement the solution discussed in that thread
(see esp.

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: file
D
D
Danny Milosavljevic wrote on 2 Jan 2018 18:26
(name . Hartmut Goebel)(address . h.goebel@crazy-compilers.com)
20180102182627.16566e3c@scratchpost.org
Hi Hartmut,

thanks for the review!

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

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

... oops.

Toggle quote (4 lines)
> 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...

Toggle quote (5 lines)
> 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.

Toggle quote (4 lines)
> * 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.
R
R
Ricardo Wurmus wrote on 4 Feb 2019 08:56
control message for bug #29856
(address . control@debbugs.gnu.org)
168b7821874.2bca155b400047763.-3085144038901161023@zoho.com
tags 29856 wontfix
R
R
Ricardo Wurmus wrote on 4 Feb 2019 08:58
Re: [bug#29856] [PATCH core-updates] guix: python-build-system:, Modify ".py" files in-place.
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
87pns8htnm.fsf@elephly.net
Danny Milosavljevic <dannym@scratchpost.org> writes:
Toggle quote (28 lines)
>
> 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!
Closed
?