Re: Patch proposal: Add --clean options to unbootstrap a project.

[Ralf Wildenhues]

Hello Benoit,

* Benoit Sigoure wrote on Sun, Jun 17, 2007 at 01:16:35AM CEST:
> 
> I've reviewed the patches I proposed back in March, completed them with
> tests, ChangeLog and NEWS entries.  If they are accepted, I'll write the
> documentation.

Please write the documentation.  ;-)

> PS: autopoint (formerly gettextize)

Erm, both tools still exist (and have different semantics).

> PPS: Although this would belong to
> {autoconf,automake,address@hidden, I only sent it to the
> Autoconf ML because Autoconf has the biggest patch and I expect Automake
> and Libtool maintainers to see this message.  I hope my assumption
> wasn't wrong :)

Well.  One purpose of the different lists is that, a couple of years
down the road, one can still dig out useful information from the
archives.  And I personally tend to look for patches to some package
only in my $PACKACKE-patches list archive.  So please send each patch to
its list.  Thank you.

Some nits inline to the Autoconf patch:

> Index: ChangeLog

Please send ChangeLog entries plain, not as diffs.  They are likely to
generate spurious conflicts otherwise.

> +     * bin/autoconf.as, bin/autoheader.in, bin/autoreconf.in: New option
> +     `--clean'.
> +     * NEWS: Mention it.
> +     * bin/autoreconf.in (&run_make): New.

Surely run_make would be used from somewhere, no?  Please mention that
function as well.  Also you are making several more changes in this
file.

> +     * lib/Autom4te/General.pm ($clean): New boolean value.
> +     (getopt): Handle `--clean'.
> +     * tests/torture.at: Test the new feature.
> +

> --- bin/autoconf.as   17 May 2007 02:43:12 -0000      1.25
> +++ bin/autoconf.as   16 Jun 2007 22:47:02 -0000
> @@ -33,6 +33,7 @@
>    -v, --verbose             verbosely report processing
>    -d, --debug               don't remove temporary files
>    -f, --force               consider all files obsolete
> +      --clean               remove files installed by autoconf

Please put this line before --debug.  I think the order is
alphabetically, with -h, -v, -V sorted first.  Likewise for
the other tools.

>    -o, --output=FILE         save output in FILE (stdout is the default)
>    -W, --warnings=CATEGORY   report the warnings falling in CATEGORY [syntax]
>  

> --- bin/autoheader.in 4 Jan 2007 16:43:06 -0000       1.147
> +++ bin/autoheader.in 16 Jun 2007 22:47:02 -0000

> @@ -195,6 +196,17 @@
>  ($config_h, $config_h_in) = split (':', $config_h, 2);
>  $config_h_in ||= "$config_h.in";
>  
> +if ($clean)
> +  {
> +    my $SIMPLE_BACKUP_SUFFIX = $ENV{'SIMPLE_BACKUP_SUFFIX'} || '~';

Shouldn't SIMPLE_BACKUP_SUFFIX be made an export from FileUtils, for
cleanliness?

> +    foreach my $f ($config_h, $config_h_in . $SIMPLE_BACKUP_SUFFIX)
> +    {
> +      unlink $f or error "error: Cannot remove `$f': $!"
> +        unless not -f $f;
> +    }
> +    exit 0;
> +  }
> +

> --- bin/autoreconf.in 4 Jan 2007 16:43:06 -0000       1.137
> +++ bin/autoreconf.in 16 Jun 2007 22:47:02 -0000

> +  error "Cannot install and clean at the same time."

No punctuation mark at the end of an error message.

> +    if ($install && $clean);
> +
>    # Split the warnings as a list of elements instead of a list of
>    # lists.
>    @warning = map { split /,/ } @warning;


> @@ -304,12 +319,50 @@
>      }
>  }
>  
> +# &run_make ([TARGET])
> +# --------------------
> +# Run make in the current directory.  config.status is run first
> +# in order to recreate the Makefile unless we're in --clean mode.
> +# Return true on success, false if something went wrong.
> +sub run_make (;$)
> +{
> +  my ($target) = @_;
> +  $target = '' if not defined $target;
> +
> +  # Regenerate the Makefile first (unless we're in --clean mode).
> +  if (!$clean)
> +    {
> +      if (!-f 'config.status')
> +     {
> +       verb 'no config.status: cannot re-make';
> +       return 0;
> +     }
> +      else
> +     {
> +       xsystem ('./config.status --recheck');
> +       xsystem ('./config.status');
> +     }
> +    }
> +  if (!-f 'Makefile')
> +    {
> +      verb 'no Makefile: cannot re-make' unless $clean;
> +      return 0;
> +    }
> +  else
> +    {
> +      xsystem ("make $target");

Ouch.  This may go berserk, for example because the package needs GNU
make (and that may happen to be 'gmake', or /opt/fsw/bin/gmake, or
whatnot).  I don't see easily how to get at $MAKE, especially since
I assume
  ./configure && gmake

is often used rather than, say,
  ./configure MAKE=gmake && gmake

Hmm.  This bug isn't new, 'autoreconf -m' already exists and has it.
I think maybe it should honor the $MAKE environment variable (in another
patch, not this one; and it should be documented as well).

> +    }
> +  return 1;
> +}
> +

> --- tests/torture.at  12 Jun 2007 11:36:57 -0000      1.82
> +++ tests/torture.at  16 Jun 2007 22:47:07 -0000
> @@ -1179,3 +1179,39 @@
>  AT_CHECK([test -f HeeHee.in])
>  
>  AT_CLEANUP
> +
> +## --------------------- ##
> +## Unbootstrap (--clean) ##
> +## --------------------- ##
> +
> +AT_SETUP([Unbootstrap with --clean])
> +AT_KEYWORDS([autoreconf])
> +
> +# We use aclocal (via autoreconf).
> +AT_CHECK([aclocal --version || exit 77], [], [ignore], [ignore])
> +# We need a version of aclocal that is recent enough to support --clean
> +AT_CHECK([aclocal --help | grep -e --clean || exit 77], [], [ignore], 
> [ignore])

According to the Autoconf manual, -e is not portable.  I guess you could
grep for ' --clean'.

> +
> +AT_DATA([configure.ac],
> +[[AC_INIT
> +AC_CONFIG_HEADERS(config.h:config.hin)

Let's also have a AC_CONFIG_FILES.  Ideally also another test that uses
automake, but I see you have that in the Automake patch, so ok.

> +AC_OUTPUT
> +]])
> +
> +
> +AT_DATA([config.hin], [])
> +
> +# Save the result of `find' to see whether we properly cleaned everything.
> +AT_DATA([list.after], [])
> +AT_DATA([list.before], [])
> +
> +# Save the list of files in the current directory.
> +find . >list.before

I don't think find output is a stable way for comparing directory
listings, judging from the hoops AC_STATE_SAVE in tests/local.at goes
through.

> +
> +AT_CHECK([autoreconf], [0], [ignore], [ignore])
> +
> +AT_CHECK([autoreconf --clean], [0], [ignore], [ignore])
> +find . >list.after
> +AT_CHECK([diff -u list.before list.after])

'diff -u' is not portable, but I think you can use $at_diff here.

> +
> +AT_CLEANUP

Another nit: it would be helpful if all three tools used tests of the
same name, in the file name and/or the AT_SETUP name.  That way running
them is easier.

FWIW, the Automake patch is malformed (missing lines at the end?) so you
may want to look into that before resending it to automake-patches.

Cheers, and thanks for all your work,
Ralf