[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Improve shell script headers and pre-inst-env handling
From: |
Mark H Weaver |
Subject: |
Re: [PATCH] Improve shell script headers and pre-inst-env handling |
Date: |
Wed, 13 Feb 2013 04:55:05 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
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
- [PATCH] Improve shell script headers and pre-inst-env handling, Mark H Weaver, 2013/02/11
- Re: [PATCH] Improve shell script headers and pre-inst-env handling, Mark H Weaver, 2013/02/11
- Re: [PATCH] Improve shell script headers and pre-inst-env handling, Ludovic Courtès, 2013/02/12
- Re: [PATCH] Improve shell script headers and pre-inst-env handling, Mark H Weaver, 2013/02/12
- Re: [PATCH] Improve shell script headers and pre-inst-env handling,
Mark H Weaver <=
- Re: [PATCH] Improve shell script headers and pre-inst-env handling, Ludovic Courtès, 2013/02/13
- Re: [PATCH] Improve shell script headers and pre-inst-env handling, Mark H Weaver, 2013/02/14
- [PATCH] Replace individual scripts with master 'guix' script, Mark H Weaver, 2013/02/14
- Re: [PATCH] Replace individual scripts with master 'guix' script, Ludovic Courtès, 2013/02/14
- Re: [PATCH] Replace individual scripts with master 'guix' script, Mark H Weaver, 2013/02/14
- Re: [PATCH] Replace individual scripts with master 'guix' script, Ludovic Courtès, 2013/02/16
- Re: [PATCH] Replace individual scripts with master 'guix' script, Ludovic Courtès, 2013/02/17