Re: [PATCH] Improve shell script headers and pre-inst-env handling

[Mark H Weaver]

Hi Ludovic,

address@hidden (Ludovic Courtès) writes:

> Mark H Weaver <address@hidden> skribis:
>
>> address@hidden (Ludovic Courtès) writes:
>>> Honestly, I wouldn’t worry about the propagation of $GUILE_LOAD_PATH &
>>> co. to subprocesses, because we know there’s none anyway.
>>
>> That policy will lead to future where libguile-using programs break in
>> random ways when they happen to be subprocesses of each other.
>
> I agree in general with your feeling.
>
> However, in that case, we know that these command-line tools are just
> wrappers around our Scheme APIs, and that they won’t ever launch any
> program (programs are a thing of the past; procedures are the future).
> So it just seemed safe to me to do that in this particular case.
>
> What do you think?

So I've been working on a patch to fix the ./pre-inst-env problem using
portable shell code instead of Guile code, as you suggested, and this is
the kind of code I'm coming up with:

--8<---------------cut here---------------start------------->8---
#!/bin/sh
# aside from this initial boilerplate, this is actually -*- scheme -*- code

prefix="@prefix@"
datarootdir="@datarootdir@"

main='(module-ref (resolve-interface '\''(guix-package)) '\'guix-package')'

if test "x$GUIX_UNINSTALLED" = x; then
  GUILE_LOAD_COMPILED_PATH="@guilemoduledir@:$GUILE_LOAD_COMPILED_PATH"
  export GUILE_LOAD_COMPILED_PATH
  exec address@hidden@} -L "@guilemoduledir@" -l "$0"    \
           -c "(apply $main (cdr (command-line)))" "$@"
else
  exec address@hidden@} -l "$0"    \
           -c "(apply $main (cdr (command-line)))" "$@"
fi
!#
--8<---------------cut here---------------end--------------->8---

or perhaps you'd prefer something more like this:

--8<---------------cut here---------------start------------->8---
#!/bin/sh
# aside from this initial boilerplate, this is actually -*- scheme -*- code

prefix="@prefix@"
datarootdir="@datarootdir@"

if test "x$GUIX_UNINSTALLED" = x; then
  GUILE_LOAD_COMPILED_PATH="@guilemoduledir@:$GUILE_LOAD_COMPILED_PATH"
  export GUILE_LOAD_COMPILED_PATH
  load_path_args="-L @guilemoduledir@"
else
  load_path_args=""
fi

main='(module-ref (resolve-interface '\''(guix-package)) '\'guix-package')'
exec address@hidden@} $load_path_args -l "$0"    \
         -c "(apply $main (cdr (command-line)))" "$@"
!#
--8<---------------cut here---------------end--------------->8---

but the more I look at this ugly, buggy code; and the more I fret
about the inherent bugs having to do with poor handling of shell
meta-characters and colons in file names; and the more I read of the
"Portable Shell Programming" chapter of the autoconf manual, the less
I understand why you feel so strongly about using this awful language
instead of the Guile code I wrote.  To save a few lines?

Please take a look at my proposed code one more time with fresh eyes:

--8<---------------cut here---------------start------------->8---
#!/bin/sh
# aside from this initial boilerplate, this is actually -*- scheme -*- code

script=guix-build

prefix="@prefix@"
datarootdir="@datarootdir@"

startup="
(let ()
  (define-syntax-rule (push! elt v) (set! v (cons elt v)))
  (define (main interpreter module-dir script-file . args)
    (unless (getenv \"GUIX_UNINSTALLED\")
      (push! module-dir %load-path)
      (push! module-dir %load-compiled-path))
    (load script-file)
    (let ((proc (module-ref (resolve-interface '($script))
                            '$script)))
      (apply proc args)))
  (apply main (command-line)))
"
exec "address@hidden@}" -c "$startup" "@guilemoduledir@" "$0" "$@"
--8<---------------cut here---------------end--------------->8---

Allow me to list the advantages:

* Schemers will have a much easier time understanding precisely what
  this code does, without having to grok the finer details of shell
  programming and Guile's command-line handling.

* It sets a good example for how to write a Guile program that will be
  robust in the case where subprocesses might also use Guile.

* The boilerplate code is identical in all scripts except on line 4
  (script=guix-build).

* It is completely robust in its handling of unusual characters, with
  the sole exception of "address@hidden@}" which could fail if @GUILE@
  contains meta-characters.  I could fix that too with a few more lines
  of code.  (And yes, I know that autoconf is already unable to handle
  filenames with meta-characters, but that's no excuse to create similar
  bugs in our own code if we can easily avoid it.  Besides, maybe some
  day autoconf can be made more robust).

and the only disadvantage I'm aware of:

* It's four lines longer (two of which could be trivially eliminated by
  removing the "script=guix-build" line and instead replacing the two
  occurrences of "$script" with "guix-build").

I would urge you to please reconsider your position.

If you still prefer the shell-based approach, then could you please take
care of fixing the ./pre-inst-env bug as you think is best?  I don't
want my name associated with it.

> (BTW, rather than $GUIX_UNINSTALLED, it just occurred to me that
> $GUIX_LOAD_PATH would do just as well while being more generic and
> easier to implement/use.)

I thought about this too, but it seems to me that it wouldn't work
properly for "./pre-inst-env guile".  Or am I missing something?

    Regards,
      Mark