#!/usr/bin/env perl

#!/usr/bin/env perl

[Andreas Enge]

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

Hello,

the lonely "perl" in a file containing the subject line survives in the 
patch-shebang phase. Right now, I am patching it by hand to replace the 
complete line by the path to perl for the curl package; would it make sense 
to add a general automatism?

Andreas

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

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> the lonely "perl" in a file containing the subject line survives in the 
> patch-shebang phase. Right now, I am patching it by hand to replace the 
> complete line by the path to perl for the curl package; would it make sense 
> to add a general automatism?

Yes, probably, because the current behavior doesn’t help at all.

The function you want to patch ;-) is ‘patch-shebang’ in
build/utils.scm.  Since that entails a full rebuild, that’s something
for a new ‘core-updates’ branch.

Ludo’.

Re: #!/usr/bin/env perl

[Andreas Enge]

[-- Attachment #1: Type: Text/Plain, Size: 400 bytes --]

Am Samstag, 26. Januar 2013 schrieb Ludovic Courtès:
> The function you want to patch ;-) is ‘patch-shebang’ in
> build/utils.scm.  Since that entails a full rebuild, that’s something
> for a new ‘core-updates’ branch.

Please find attached a patch for review. I tested it on a few example 
files, but not yet with a real packet - I am waiting for the full rebuild 
;-)

Andreas

[-- Attachment #2: 0001-Patch-shebang-Handle-usr-bin-env-command.patch --]
[-- Type: text/x-patch, Size: 3477 bytes --]

From b4763457c1127a3c910dc3d28f1ce933175a8be0 Mon Sep 17 00:00:00 2001
From: Andreas Enge <andreas@enge.fr>
Date: Fri, 22 Feb 2013 23:00:41 +0100
Subject: [PATCH] Patch-shebang: Handle "#!/usr/bin/env command"

* guix/build/utils.scm (patch-shebang): Handle replacement of
   "#!.*/env CMD ARGS" by "#!/nix/store/path/.../to/CMD ARGS".
---
 guix/build/utils.scm |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 6921e31..bab82fe 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -426,7 +426,7 @@ bytes transferred and the continuation of the transfer as a thunk."
          (stat:mtimensec stat)))
 
 (define patch-shebang
-  (let ((shebang-rx (make-regexp "^[[:blank:]]*([[:graph:]]+)(.*)$")))
+  (let ((shebang-rx (make-regexp "^[[:blank:]]*([[:graph:]]+)[[:blank:]]*([[:graph:]]*)(.*)$")))
     (lambda* (file
               #:optional
               (path (search-path-as-string->list (getenv "PATH")))
@@ -465,16 +465,27 @@ FILE are kept unchanged."
                (let ((line (false-if-exception (read-line p))))
                  (and=> (and line (regexp-exec shebang-rx line))
                         (lambda (m)
-                          (let* ((cmd (match:substring m 1))
-                                 (bin (search-path path (basename cmd))))
+                          (let* ((first (match:substring m 1))
+                                 (second (match:substring m 2))
+                                 (third (match:substring m 3))
+                                 (has-env (string=? (string-pad first 4) "/env"))
+                                 (cmd (if has-env second (basename first)))
+                                 (bin (search-path path cmd)))
                             (if bin
-                                (if (string=? bin cmd)
+                                (if (string=? bin first)
                                     #f            ; nothing to do
-                                    (begin
-                                      (format (current-error-port)
-                                              "patch-shebang: ~a: changing `~a' to `~a'~%"
-                                              file cmd bin)
-                                      (patch p bin (match:substring m 2))))
+                                    (if has-env
+                                      (begin
+                                        (format (current-error-port)
+                                                "patch-shebang: ~a: changing `~a' to `~a'~%"
+                                                file (string-append first " " second) bin)
+                                        (patch p bin third))
+                                      (begin 
+                                        (format (current-error-port)
+                                                "patch-shebang: ~a: changing `~a' to `~a'~%"
+                                                file first bin)
+                                        (patch p bin
+                                          (string-append " " second third)))))
                                 (begin
                                   (format (current-error-port)
                                           "patch-shebang: ~a: warning: no binary for interpreter `~a' found in $PATH~%"
-- 
1.7.10.4

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> Am Samstag, 26. Januar 2013 schrieb Ludovic Courtès:
>> The function you want to patch ;-) is ‘patch-shebang’ in
>> build/utils.scm.  Since that entails a full rebuild, that’s something
>> for a new ‘core-updates’ branch.
>
> Please find attached a patch for review. I tested it on a few example 
> files, but not yet with a real packet - I am waiting for the full rebuild 
> ;-)

Heh, thanks.

> --- a/guix/build/utils.scm
> +++ b/guix/build/utils.scm
> @@ -426,7 +426,7 @@ bytes transferred and the continuation of the transfer as a thunk."
>           (stat:mtimensec stat)))
>  
>  (define patch-shebang
> -  (let ((shebang-rx (make-regexp "^[[:blank:]]*([[:graph:]]+)(.*)$")))
> +  (let ((shebang-rx (make-regexp "^[[:blank:]]*([[:graph:]]+)[[:blank:]]*([[:graph:]]*)(.*)$")))

I think it should be a + after the second [[:blank:]], because there
should be space between the interpreter name and its argument.

>      (lambda* (file
>                #:optional
>                (path (search-path-as-string->list (getenv "PATH")))
> @@ -465,16 +465,27 @@ FILE are kept unchanged."
>                 (let ((line (false-if-exception (read-line p))))
>                   (and=> (and line (regexp-exec shebang-rx line))
>                          (lambda (m)
> -                          (let* ((cmd (match:substring m 1))
> -                                 (bin (search-path path (basename cmd))))
> +                          (let* ((first (match:substring m 1))
> +                                 (second (match:substring m 2))
> +                                 (third (match:substring m 3))

What about more descriptive names like ‘interp’, ‘arg1’, and ‘rest’?

> +                                 (has-env (string=? (string-pad first 4) "/env"))

Rather (has-env? (string-suffix? "/env" interp)).

> +                                    (if has-env
> +                                      (begin

Please align with the ‘h’ here...

> +                                        (patch p bin
> +                                          (string-append " " second third)))))

... and with the ‘p’ here.

Modulo these details, looks good to me, so feel free to push!

Thanks,
Ludo’.

Re: #!/usr/bin/env perl

[Andreas Enge]

Am Samstag, 23. Februar 2013 schrieb Ludovic Courtès:
> > (make-regexp
> > "^[[:blank:]]*([[:graph:]]+)[[:blank:]]*([[:graph:]]*)(.*)$")))
> 
> I think it should be a + after the second [[:blank:]], because there
> should be space between the interpreter name and its argument.

Sometimes, there is no space at all: In
#!/usr/bin/perl
for instance, when there is no argument. Then the "+" version would fail. 
With "*", it works as expected.

> What about more descriptive names like ‘interp’, ‘arg1’, and ‘rest’?

Maybe. I did not choose descriptive names because the real interpreter is 
sometimes the first, sometimes the second piece. But your suggestion looks 
good.

> Rather (has-env? (string-suffix? "/env" interp)).

Ok, thanks.

> > +                                    (if has-env
> > +                                      (begin
> Please align with the ‘h’ here...
> > +                                        (patch p bin
> > +                                          (string-append " " second
> > third)))))
> ... and with the ‘p’ here.

Ok. The annoying thing with this way of indenting is that lines become very 
long very quickly; I like keeping them at less than 80 characters.

Andreas

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> Am Samstag, 23. Februar 2013 schrieb Ludovic Courtès:
>> > (make-regexp
>> > "^[[:blank:]]*([[:graph:]]+)[[:blank:]]*([[:graph:]]*)(.*)$")))
>> 
>> I think it should be a + after the second [[:blank:]], because there
>> should be space between the interpreter name and its argument.
>
> Sometimes, there is no space at all: In
> #!/usr/bin/perl
> for instance, when there is no argument. Then the "+" version would fail. 
> With "*", it works as expected.

Oh, right.

>> > +                                    (if has-env
>> > +                                      (begin
>> Please align with the ‘h’ here...
>> > +                                        (patch p bin
>> > +                                          (string-append " " second
>> > third)))))
>> ... and with the ‘p’ here.
>
> Ok. The annoying thing with this way of indenting is that lines become very 
> long very quickly; I like keeping them at less than 80 characters.

When it becomes too long, it’s sometimes a sign that an auxiliary
function may be needed.

Thanks!

Ludo’.

Re: #!/usr/bin/env perl

[Andreas Enge]

[-- Attachment #1: Type: Text/Plain, Size: 1198 bytes --]

Am Freitag, 22. Februar 2013 schrieb Andreas Enge:
> Please find attached a patch for review. I tested it on a few example
> files, but not yet with a real packet - I am waiting for the full
> rebuild ;-)

I had test failures in coreutils, and to check whether it was related to my 
patch, I tried from scratch with the current core-updates branch. Any call 
to guix aborts with

Backtrace:
In ice-9/boot-9.scm:
 149: 9 [catch #t #<catch-closure 286aa00> ...]
 157: 8 [#<procedure 28060f0 ()>]
In unknown file:
   ?: 7 [catch-closure]
In ice-9/boot-9.scm:
  63: 6 [call-with-prompt prompt0 ...]
In ice-9/eval.scm:
 407: 5 [eval # #]
In ice-9/boot-9.scm:
2111: 4 [save-module-excursion #<procedure 280b100 at 
ice-9/boot-9.scm:3646:3 ()>]
3651: 3 [#<procedure 280b100 at ice-9/boot-9.scm:3646:3 ()>]
In unknown file:
   ?: 2 [load-compiled/vm "/home/privat/.cache/guile/ccache/2.0-
LE-8-2.0/usr/local/guix-git/bin/guix.go"]
In /usr/local/bin/guix:
  59: 1 [#<procedure 28381c0 ()>]
In unknown file:
   ?: 0 [scm-error misc-error #f ...]

ERROR: In procedure scm-error:
ERROR: No variable named guix-main in #<interface (guix ui) 28a0ea0>

I am also attaching the final patch-shebang patch.

Andreas

[-- Attachment #2: 0001-Patch-shebang-Handle-usr-bin-env-command.patch --]
[-- Type: text/x-patch, Size: 3719 bytes --]

From 63ac645147871eb773032da23d9f52bbd4f823eb Mon Sep 17 00:00:00 2001
From: Andreas Enge <andreas@enge.fr>
Date: Fri, 22 Feb 2013 23:00:41 +0100
Subject: [PATCH] Patch-shebang: Handle "#!/usr/bin/env command"

* guix/build/utils.scm (patch-shebang): Handle replacement of
   "#!.*/env CMD ARGS" by "#!/nix/store/path/.../to/CMD ARGS".
---
 guix/build/utils.scm |   30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 6921e31..82042e9 100644
--- a/guix/build/utils.scm
+++ b/guix/build/utils.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2012, 2013 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2013 Andreas Enge <andreas@enge.fr>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -426,7 +427,7 @@ bytes transferred and the continuation of the transfer as a thunk."
          (stat:mtimensec stat)))
 
 (define patch-shebang
-  (let ((shebang-rx (make-regexp "^[[:blank:]]*([[:graph:]]+)(.*)$")))
+  (let ((shebang-rx (make-regexp "^[[:blank:]]*([[:graph:]]+)[[:blank:]]*([[:graph:]]*)(.*)$")))
     (lambda* (file
               #:optional
               (path (search-path-as-string->list (getenv "PATH")))
@@ -465,16 +466,27 @@ FILE are kept unchanged."
                (let ((line (false-if-exception (read-line p))))
                  (and=> (and line (regexp-exec shebang-rx line))
                         (lambda (m)
-                          (let* ((cmd (match:substring m 1))
-                                 (bin (search-path path (basename cmd))))
+                          (let* ((interp (match:substring m 1))
+                                 (arg1 (match:substring m 2))
+                                 (rest (match:substring m 3))
+                                 (has-env (string-suffix? "/env" interp))
+                                 (cmd (if has-env arg1 (basename interp)))
+                                 (bin (search-path path cmd)))
                             (if bin
-                                (if (string=? bin cmd)
+                                (if (string=? bin interp)
                                     #f            ; nothing to do
-                                    (begin
-                                      (format (current-error-port)
-                                              "patch-shebang: ~a: changing `~a' to `~a'~%"
-                                              file cmd bin)
-                                      (patch p bin (match:substring m 2))))
+                                    (if has-env
+                                        (begin
+                                          (format (current-error-port)
+                                                  "patch-shebang: ~a: changing `~a' to `~a'~%"
+                                                  file (string-append interp " " arg1) bin)
+                                          (patch p bin rest))
+                                      (begin 
+                                        (format (current-error-port)
+                                                "patch-shebang: ~a: changing `~a' to `~a'~%"
+                                                file interp bin)
+                                        (patch p bin
+                                               (string-append " " arg1 rest)))))
                                 (begin
                                   (format (current-error-port)
                                           "patch-shebang: ~a: warning: no binary for interpreter `~a' found in $PATH~%"
-- 
1.7.10.4

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> I had test failures in coreutils,

Do you still have the details around?  ‘env’ is part of Coreutils, so it
may be that it’s testing its behavior, and that patching prevents that.

Perhaps ‘patch-shebang’ could have a ‘patch-env?’ keyword parameter,
defaulting to #t, and if the above is correct, we’d instead arrange to
have it called with #:patch-env? #f.

> and to check whether it was related to my patch, I tried from scratch
> with the current core-updates branch. Any call to guix aborts with

[...]

> In /usr/local/bin/guix:
>   59: 1 [#<procedure 28381c0 ()>]
> In unknown file:
>    ?: 0 [scm-error misc-error #f ...]
>
> ERROR: In procedure scm-error:
> ERROR: No variable named guix-main in #<interface (guix ui) 28a0ea0>

You should be using ‘./pre-inst-env guix build xxx’, which you don’t
seem to be doing, no?

For info about why this happens, you could try:

  guile -c '(pk (@ (guix ui) guix-main))'

> From 63ac645147871eb773032da23d9f52bbd4f823eb Mon Sep 17 00:00:00 2001
> From: Andreas Enge <andreas@enge.fr>
> Date: Fri, 22 Feb 2013 23:00:41 +0100
> Subject: [PATCH] Patch-shebang: Handle "#!/usr/bin/env command"
>
> * guix/build/utils.scm (patch-shebang): Handle replacement of
>    "#!.*/env CMD ARGS" by "#!/nix/store/path/.../to/CMD ARGS".

Looks good to me.  Perhaps the only thing left will be the addition of
#:patch-env?, unless that turns out to be unnecessary.

Thanks!

Ludo’.

Re: #!/usr/bin/env perl

[Andreas Enge]

Am Samstag, 23. Februar 2013 schrieb Ludovic Courtès:
> Andreas Enge <andreas@enge.fr> skribis:
> > I had test failures in coreutils,
> 
> Do you still have the details around?  ‘env’ is part of Coreutils, so it
> may be that it’s testing its behavior, and that patching prevents that.

This was also my suspicion, but it did not seem to be the case - I looked 
at what was patch-shebanged, and none of it contained "env". I will check 
again once core-updates works.

> > In /usr/local/bin/guix:
> >   59: 1 [#<procedure 28381c0 ()>]
> > 
> > In unknown file:
> >    ?: 0 [scm-error misc-error #f ...]
> > 
> > ERROR: In procedure scm-error:
> > ERROR: No variable named guix-main in #<interface (guix ui) 28a0ea0>
> 
> You should be using ‘./pre-inst-env guix build xxx’, which you don’t
> seem to be doing, no?

No, but I always do "make install" first. Prepending "./pre-inst-env" does 
not change anything.

> For info about why this happens, you could try:
>   guile -c '(pk (@ (guix ui) guix-main))'

Backtrace:
In ice-9/boot-9.scm:
 149: 10 [catch #t #<catch-closure 1e31a00> ...]
 157: 9 [#<procedure 1dcd0f0 ()>]
In unknown file:
   ?: 8 [catch-closure]
In ice-9/boot-9.scm:
  63: 7 [call-with-prompt prompt0 ...]
In ice-9/eval.scm:
 407: 6 [eval # #]
In unknown file:
   ?: 5 [call-with-input-string "(pk (@ (guix ui) guix-main))" ...]
In ice-9/command-line.scm:
 174: 4 [#<procedure 1d4c460 at ice-9/command-line.scm:169:3 (port)> 
#<input: string 1bf34e0>]
In unknown file:
   ?: 3 [eval (pk (@ (guix ui) guix-main)) #<directory (guile-user) 
1e33d80>]
In ice-9/eval.scm:
 368: 2 [eval # ()]
 425: 1 [eval #<memoized (@ (guix ui) guix-main)> ()]
In unknown file:
   ?: 0 [memoize-variable-access! #<memoized (@ (guix ui) guix-main)> #f]

ERROR: In procedure memoize-variable-access!:
ERROR: In procedure module-lookup: Wrong type argument in position 1 
(expecting module): #f

$ guile --version
guile (GNU Guile) 2.0.5-deb+1-3

Andreas

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

>> For info about why this happens, you could try:
>>   guile -c '(pk (@ (guix ui) guix-main))'

[...]

> In unknown file:
>    ?: 0 [memoize-variable-access! #<memoized (@ (guix ui) guix-main)> #f]
>
> ERROR: In procedure memoize-variable-access!:
> ERROR: In procedure module-lookup: Wrong type argument in position 1 
> (expecting module): #f

Hmm, what about guile -c '(use-modules (guix ui))'?

Ludo’.

Re: #!/usr/bin/env perl

[Andreas Enge]

Am Samstag, 23. Februar 2013 schrieb Ludovic Courtès:
> Hmm, what about guile -c '(use-modules (guix ui))'?

In ice-9/boot-9.scm:
 149: 16 [catch #t #<catch-closure 2584a00> ...]
 157: 15 [#<procedure 25200f0 ()>]
In unknown file:
   ?: 14 [catch-closure]
In ice-9/boot-9.scm:
  63: 13 [call-with-prompt prompt0 ...]
In ice-9/eval.scm:
 407: 12 [eval # #]
In unknown file:
   ?: 11 [call-with-input-string "(use-modules (guix ui))" ...]
In ice-9/command-line.scm:
 174: 10 [#<procedure 2498340 at ice-9/command-line.scm:169:3 (port)> 
#<input: string 23474e0>]
In unknown file:
   ?: 9 [eval (use-modules (guix ui)) #<directory (guile-user) 2586d80>]
In ice-9/eval.scm:
 480: 8 [#<procedure 2453c80 at ice-9/eval.scm:474:4 (exp)> (use-modules 
(guix ui))]
In ice-9/psyntax.scm:
1101: 7 [expand-top-sequence ((use-modules (guix ui))) () ...]
 986: 6 [scan ((use-modules (guix ui))) () ...]
 270: 5 [scan ((# #) #(syntax-object *unspecified* # #)) () (()) ...]
In ice-9/boot-9.scm:
3244: 4 [process-use-modules (((guix ui)))]
 545: 3 [map #<procedure 25227a0 at ice-9/boot-9.scm:3244:25 (mif-args)> 
((#))]
3245: 2 [#<procedure 25227a0 at ice-9/boot-9.scm:3244:25 (mif-args)> ((guix 
ui))]
2583: 1 [resolve-interface (guix ui) #:select ...]
In unknown file:
   ?: 0 [scm-error misc-error #f "~A ~S" ("no code for module" (guix ui)) 
#f]

ERROR: In procedure scm-error:
ERROR: no code for module (guix ui)

Very strange. I have ui.{scm,go} in the installation location in
/usr/local/guix-git/share/guile/site/2.0/guix/, and also in the guix 
subdirectory of the source directory. With master, there are no problems.

Andreas

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> Very strange. I have ui.{scm,go} in the installation location in
> /usr/local/guix-git/share/guile/site/2.0/guix/, and also in the guix 
> subdirectory of the source directory. With master, there are no problems.

Aaah yes, that’s because ‘master’ (which adds the ‘guix’ script) hasn’t
been merged yet into ‘core-updates’.  I’ve done it locally, and will
probably push real soon, but in the meantime you must still use
‘./pre-inst-env guix-build’ on ‘core-updates’.

Ludo’.

Re: #!/usr/bin/env perl

[Andreas Enge]

Am Samstag, 23. Februar 2013 schrieb Ludovic Courtès:
> Aaah yes, that’s because ‘master’ (which adds the ‘guix’ script) hasn’t
> been merged yet into ‘core-updates’.  I’ve done it locally, and will
> probably push real soon, but in the meantime you must still use
> ‘./pre-inst-env guix-build’ on ‘core-updates’.

Okay, I see. I used "./pre-inst-env guix build".

Andreas

Re: #!/usr/bin/env perl

[Mark H Weaver]

Hi Andreas,

Andreas Enge <andreas@enge.fr> writes:
> (has-env (string-suffix? "/env" interp))

I think we might want to make this test more restrictive.  I'm looking
at a package (guile-figl) that includes an 'env' script in its build
directory, analogous to 'pre-inst-env' in Guix.  This makes me worry
that the test above will have false positives.

In practice, the path used is (almost?) always "/usr/bin/env".
I suspect it would be safer to check for that exact string.

What do you think?

    Mark

Re: #!/usr/bin/env perl

[Andreas Enge]

Am Montag, 25. Februar 2013 schrieb Mark H Weaver:
> Andreas Enge <andreas@enge.fr> writes:
> > (has-env (string-suffix? "/env" interp))
> 
> I think we might want to make this test more restrictive.  I'm looking
> at a package (guile-figl) that includes an 'env' script in its build
> directory, analogous to 'pre-inst-env' in Guix.  This makes me worry
> that the test above will have false positives.
> 
> In practice, the path used is (almost?) always "/usr/bin/env".
> I suspect it would be safer to check for that exact string.

I also wondered whether we should do this. Do you have a concrete example 
where the current test fails? Notice that the suffix is only part of the 
test; currently, we patch files that start with
   [[:blank:]]*#![[:graph:]]*/env
Unless a problem manifests itself, I would not change that behaviour.

Andreas

Re: #!/usr/bin/env perl

[Ludovic Courtès]

Andreas Enge <andreas@enge.fr> skribis:

> Am Montag, 25. Februar 2013 schrieb Mark H Weaver:
>> Andreas Enge <andreas@enge.fr> writes:
>> > (has-env (string-suffix? "/env" interp))
>> 
>> I think we might want to make this test more restrictive.  I'm looking
>> at a package (guile-figl) that includes an 'env' script in its build
>> directory, analogous to 'pre-inst-env' in Guix.

I suspect it’s never used in a shebang though, no?

>> In practice, the path used is (almost?) always "/usr/bin/env".
>> I suspect it would be safer to check for that exact string.

That’s probably true.

> I also wondered whether we should do this. Do you have a concrete example 
> where the current test fails? Notice that the suffix is only part of the 
> test; currently, we patch files that start with
>    [[:blank:]]*#![[:graph:]]*/env
> Unless a problem manifests itself, I would not change that behaviour.

Yes, makes sense to wait for evidence.

Ludo’.