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

DoneSubmitted by Danny Milosavljevic.
Details
5 participants
  • Danny Milosavljevic
  • Hartmut Goebel
  • Leo Famulari
  • Marius Bakke
  • Ricardo Wurmus
Owner
unassigned
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.scmindex 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-----
iQIzBAABCAAdFiEEsFFZSPHn08G5gDigJkb6MLrKfwgFAlpCnpUACgkQJkb6MLrKfwh7vw/8C8gRrXHE1ziXAB4mAn2EgJp12DvRXOoS4VUE6UjPp2dC9hd+jqjbEmbqPV2JnGUiCyIz4I5Umk5Vxd3NdsuNeozJSHsWRVMLhsrtq+lBi8mc0XPFkWvPVPhxYVKSHyE/urFwxTu6pyqtmKVRIJWdffWNpPL8iVP7x0OsjC/SgQ3ztfjpbctzLWEJa4KbSMvwY8PcDKqh+hKLrMC+BgnM86Gadj+dpyDzaI8jhMPaX50z/T6emiGS0JeHntlgl7zX36JjwBBSR8sczgaYjkAZTM3Vfxwnsdi/Tqil9MHI4DVwCcDfA3VPov5omnGWX0U+jDghmshe+5RjU/rNoQRHIeM9T9VA8wCyKKDJu25mTMzjOiff8ln2vnJLTr0nDBUC9OT3XTWVtGbCxVSbbxR+1K4YdimO+qxtynAheqL2VjKTvdZ9dGUwFObOBMuqYYKymPoMjPfqSs35oGUUC9HT4xEtzwTvVIY6rh5y5dV6FVz0zTWngggTrBDr9YlQxIap8+OoUYMuVakO6rddFzPAE0cYFItuuspriY/7Bxp5lVhmCqbL06jQJQyg4y76TVFVGQZH/e6QLv3Yz6tx6PWjtoF1Suy+G/ef+5CqKau8qfA8B5RY1E1ErD+rI/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 -0500Leo 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 notinstalled 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/AYACgkQoqBt8qM6VPrrHQf+KiR+FFQjWUMT1Klcqz+LzuZehrNcUwxFUeBapTRPn8IlZKo47EMD09Lbv2N6ODeE2F8A4+xX35PrD0dZTTnHNGpSmYoePMO6SyPI0GHq6cAMsXASpWKPQJDycB3QsN4MG7IBhmIPf22VKESudzuLHBOZmmuG5CLYo6dfFdYhcj6chh6I6wDvqOocoxcxbfwyjKI058TIFaQb4OYFfam5LkAtKmm5epQ351ttKPpD8e29aOkpV4WpzN6Gv7qHClHE3+p/caNSTYAxAKVlUBI1a06poea1imUU6XR6eVpAqdgT+5OgGzT+y+rkFw8M4naroPxuRkbobfc0qUd8YVmAiw===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 +0100Marius 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 inhttps://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.
-- RegardsHartmut 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 +0100Hartmut 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
?
Your comment

This issue is archived.

To comment on this conversation send email to 29856@debbugs.gnu.org