Re: a saner bootstrap script

[Stefano Lattarini]

On Monday 26 September 2011, Gary V wrote:
> Hi Stefano!
>
Hi Gary, thanks for your quick and detailed answers.

> [[Items I agree with elided]]
>
Ditto.

> On 26 Sep 2011, at 17:27, Stefano Lattarini wrote:
> > Do you have a diff file to show what you've changed exactly?  I'd find that
> > really helpful.
> 
> http://git.savannah.gnu.org/cgit/zile.git/diff/?id2=42498d861f5f5fbeb0a0b61d11c57a7553c855f1
>
Thanks for the reference.  At this point, couln't you extract the existing
history of your boostrap rewrite from there, and start applying it to a new
topic branch in the gnulib repository?  This way, future contributors could
have a more realistic view of the developement process of the new bootstrap
script.  And even in the very worst scenario, in which your rewrite is
ultimately rejected, there will be no problem: the topic branch could just
be deleted, or simply declared a "dead end", without *any* impact for the
gnulib master branch.

> > I agree that keeping such snippets as unchanged as possible is a good idea.
> > The real fix for the style inconsistencies they introduce is to add proper
> > comments referencing the place from where any snipped had been copied.
> 
> Yes, I'm still kicking myself for not having kept track of that better.  
> Sorry.
>
No big deal, this can still be fixed in later commits, whenever someone points
out an inconsistency (we did something similar for some code in the automake
testsuite that had been copied & pasted from libtool).

> >>>> func_update_translations ()
> >>>> {
> >>>>   $debug_cmd
> >>>> 
> >>>>   $opt_skip_po || {
> >>>>     test -d po && {
> >>>>       $require_package
> >>>> 
> >>>>       func_update_po_files po $package || exit $?
> >>>>     }
> >>>> 
> >>>>     func_run_hooks func_update_translations
> >>>>   }
> >>>> }
> >>>> 
> >>> I personally find the code flow here quite unclear.  Something like
> >>> this would be clearer IMHO:
> >>> 
> >>> $opt_skip_po && return
> >>> if test -d po; then
> >>>   $require_package
> >>>   func_update_po_files po $package || exit $?
> >>> fi
> >>> func_run_hooks func_update_translations
> >>> 
> >>> The same goes for similar usages throughout the script.
> >> 
> >> I suppose it's just a matter of which idioms your eye have become 
> >> accustomed
> >> to.  Personally, I find the additional noise of 'if', '; then' and 'fi'
> >> distracting, and have been happily using the braces idiom (when there's no
> >> 'else' branch to consider) for many many years as a result (e.g. the shell
> >> code in libtool).
> >> 
> >> If all that code churn and retesting is a prerequisite for acceptance into
> >> gnulib,
> >> 
> > This is only for the gnulib maintainers to decide (I'm an "occasional
> > small contributor" at the very most, so my opinion carries no weight in
> > this regard).  What I can say is that the current style is very, *very*
> > confusing to me, and the need to respect it would shy me away from the
> > idea of contributing to the bootstrap script.
> 
> I'm not entirely opposed to changing it, but only if it is a show-stopper.
> I can honestly say that I've never misspelled a closing '}', where I would
> be quite rich if I had a penny for the number of times I'd typed endif or
> some variant instead of 'fi', or forgotten the semicolon before 'then', or
> forgotten to type 'then' entirely.
>
It's quite the contrary for me.

> Once you start writing scripts in this style it's hard to go back to the
> clumsy noisy 'if' '; then' 'fi' constructs
>
I've never found that style "clumsy"; I've come to find it rather pleasant
instead.

> Unless there are serious issues with a peculiarity of style, when accepting
> contributions to my projects I tend to be sympathetic to the preferences of
> the author and maintainer of a contributed piece of code... except of course
> where it flies in the face of conventions set by the surrounding code.
>
In the case of gnulib and the autotools, it seems to me that the accepted
style for shell code is to almost always use "if ... then ... fi" for
conditionals; the only regular expections being setting the default for
possibly uninitialized variables, as in e.g. (from gnulib-tool):

  test -n "$destdir" || destdir=testdir$$

or *sometimes* (seldom indeed) when the conditional action is a *very simple*
one-liner, as in e.g. (again from gnulib-tool):

  test -z "$inc_cxx_tests" || echo "gl_WITH_CXX_TESTS"

You might find this command instructive in this regard:

 $ grep -E'(&&|\|\|)' gnulib-tool

It will find almost no use of your coding style.

> Really, many stylistic conventions come down to programmer preferences,
>
Yes, but in case a stylistic convention makes it more diffucult for a
portion of the potential contributors to understand and hack the code,
then it might make sense to change that convention.

For example, while I find your style of "no space between pipe `|' symbol
and subsequent commands" visually annoying, I also find it in *no* way
more difficult to understand or follow than the usual style of "space
before and after `|'".  The same goes for various conventions of the GNU
coding standards: I don't like the requirement to put a space between a
function name and the following `(', and I find the GNU-mandated
indentation style really wasteful w.r.t. vertical spacing -- *but* these
conventions doesn't make the code harder to understand for me, so I don't
complain about them.

OTOH, the way in which you structure non-trivial code flow with the (ab)use 
of `&&' and `||' makes it really harder for the to understand what your
code is supposed to do; it hampers my comprehesion of it, and my will to
contribute to it.

> although in common with many of the people who prefer conflicting conventions,
> I can back up most of my choices with anti-examples of what can go wrong by
> not following the conventions I've built up over the last quarter century of
> writing shell scripts for the FSF :)
>
I'd like to read some of these examples.

> >> then I'll reluctantly go through and change the style to match...
> >> but that will also prevent cross-patching with donor code in libtool and
> >> various other scripts I have that share the current idioms.
> >> 
> > As I've said, occasional style inconsistencies are OK if they are meant to
> > facilitate syncing with third-party projects (but I'd like to see big noisy
> > comments about this fact, for every snippet of inconsistent code).
> 
> The main points of sharing are functions used between many of the scripts in
> Libtool and to a lesser extent in M4, all of which I had the luxury of writing
> in my preferred style since I was the lead maintainer and/or contributor to
> those projects at the time.  I wouldn't dream of being arrogant enough to say
> that the conventions I like and follow are any better than another set of
> conflicting conventions such as those used in gnulib-tool... but I will say
> that I'm very happy with mine, and that they have served me well in helping
> to keep my shell-scripting productivity pretty high considering how fiddly and
> annoying it can be to write clean elegant shell code, let alone portable shell
> code! :-)
> 

> >> Actually, yuck for the mixed styles in that function.  Refactored the last
> >> block to match the first:
> >> 
> >>    ${opt_dry_run-false} || {
> >>      eval "$my_cmd"
> >>      my_status=$?
> >>      test 0 -eq $my_status || eval "(exit $my_status); $my_fail_exp"
> >>    }
> >> 
> > Oh no!  Not these nested `||'!  They obfuscate a perfectly clear logic IMHO;
> > couldn't you use this instead, please?
> > 
> >    ${opt_dry_run-false} || {
> >      eval "$my_cmd"
> >      my_status=$?
> >      if test $my_status -ne 0; then
> >         eval "(exit $my_status); $my_fail_exp"
> >      fi
> >    }
> 
> As much as you hate them that way, I find the opportunity to misspell a 'fi'
> or forget the ';' before the 'then' equally painful.  Again though, it's just
> a stylistic issue, and the apparent ugliness is just because of the difference
> to what our eyes are used to seeing.
> 
> What could be clearer than 'try this' || 'if that failed, do this instead'?
>
Nothing; but this is not what you are doing!  You are not "trying this", but
rather looking whether "this is true".  The use of `||' to catch and handle
expected errors is great and idiomatic, but you should not start to abuse it
to also "catch" and "handle" false conditions.  This just add confusion and
makes your intentions more difficult to understand; and it gets much worse
when you start using complex nested `||' and `&&'.

> I acknowledge that spending equal time programming C, Python, Lua, elisp and
> shell has probably warped my sensibilities at least a little though.  Be
> thankful that I never started to enjoy Perl, or the horrors would be so much
> worse for us all!! :-)
>
It might surprise you, but I do enjoy perl :-)

> >>>>   { $app --version || $app --version </dev/null; } >/dev/null 2>&1 \
> >>>>     || return 1
> >>>> 
> >>> What's the point of the second `$app --version'?
> >> 
> >> To accomodate tools like git2cl, which error out with no stdin.
> >> 
> > Then why not a simpler:
> >  $app --version </dev/null >/dev/null 2>&1
> > instead?
> 
> Other applications (I think help2man was a culprit here) refuse to spit
> out a version number when started with stdin pointing at /dev/null.
>
Ouch, I'd have never guessed that, really.  Time for a comment? ;-)

> >>> Finally, a couple of more general observations:
> >>> 
> >>> 1. The bootstrap script is now complex enough to warrant the
> >>>    introduction of a testsuite.
> >> 
> >> That's an excellent notion.  But after a year or more of prodding and
> >> cajoling, I haven't even gotten the script itself accepted into gnulib.
> >> I'm not ready to burn another month or two of my GNU hacking time yet,
> >> at least until all the work I put into bootstrap itself is legitimized
> >> by it's acceptance.
> >> 
> > Makes sense.  As long as you agree that adding a testsuite once your
> > bootstrap rewrite has been accepted is something that should be done,
> > I'm really fine.
> 
> Well, I won't promise to actually write the testsuite,
>
I wasn't pretending that much from you; you are kindly *offering* your
time and work here, so it's not like me or anybody else could pretend
anything from you!

> but I do promise
> that I would like to see it just as much as you, and will try to at
> least put a framework in place for contributors to hang test cases on
> not too long after bootstrap makes its way upstream.
>
Thanks!

> >>> 2. IMHO, since bootstrap is a maintainer tool anyway, we should
> >>>    assume that a POSIX shell is used to run it, thus allowing
> >>>    various simplifications and optimizations.  If we want to be
> >>>    helpful towards developer having an inferior default shell,
> >>>    we could run a sanity check on the shell itself early in the
> >>>    script, warning the user if his shell isn't able to handle
> >>>    the required constructs and usages.
> >> 
> >> Because of the amount of boilerplate in this script (from my own code,
> >> much of which I also share with libtool and m4) I'd rather not optimize
> >> any of it for bootstraps particular use, since that reduces future
> >> sharing opportunities.
> >> 
> > OK.  Still, I'm somewhat saddened by the fact that, when it comes to the
> > shell, we are still coding as if we were in the late eighties :-(
> 
> You mean the late 90's, surely?
>
No, I mean we're still constraining ourselves to only use features and
idioms that have been in place since *the late eighties*.  I mean, if
coreutils has started to require a C99 compiler, the GNU shell-based
projects could at least start to require a POSIX shell where this is
convenient, no?

> Before that, we still weren't comfortable even using shell functions in
> portable shell scripts!!
>
> Cheers,
> -- 
> Gary V. Vaughan (gary AT gnu DOT org)

Regards,
  Stefano