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

[Ralf Wildenhues]

Hello Benoit,

Thanks for your continuing work on this!

* Benoit Sigoure wrote on Tue, Jun 19, 2007 at 12:22:05AM CEST:
>
> 2007-06-17  Benoit Sigoure  <address@hidden>
>
>       * automake.in, aclocal.in: New option `--clean'.
>       ($clean, @files_installed): New.
>       * automake.in (&require_conf_file): Save the list of files to be
>         removed in @files_installed.
>       * aclocal.in (&install_file, &write_aclocal): Likewise.
>       * NEWS: Mention the new option.
>       * tests/Makefile.am, tests/Makefile.in: Add the new test.

Tiny nit: In the Automake package, it's been convention for a few years
not to mention changes of generated files, so let's not mention
Makefile.in here.

>       * tests/unbootstrap.test: New.

doc changes are missing:

        * doc/automake.texi (Invoking Automake, aclocal options):
        Document new options.

Also, can we rename unbootstrap.test to bootclean.test (I think that is
what you used in Autoconf as well)?

> --- aclocal.in        14 Oct 2006 17:40:25 -0000      1.140
> +++ aclocal.in        18 Jun 2007 22:20:06 -0000

> @@ -1050,7 +1076,29 @@
>      last if write_aclocal ($output_file, keys %macro_traced);
>      last if $dry_run;
>    }
> -check_acinclude;
> +check_acinclude unless $clean;
> +
> +if ($clean)
> +  {
> +    if ($dry_run)
> +      {
> +        foreach my $cleanfile (@files_installed)
> +       {
> +         print "rm -f $cleanfile\n"
> +           unless not -e $cleanfile;
> +       }
> +        print "rm -rf autom4te.cache\n";
> +      }
> +    else
> +      {
> +        foreach my $cleanfile (@files_installed)
> +       {
> +         unlink $cleanfile or fatal "Failed to remove `$cleanfile': $!"
> +           unless not -e $cleanfile;
> +       }
> +        system ('rm -rf autom4te.cache');

Same problem as with autoconf: autom4te.cache should not be hard-coded.

> +      }
> +  }

> --- automake.in       3 May 2007 17:57:41 -0000       1.1645
> +++ automake.in       18 Jun 2007 22:20:08 -0000
[...]
> @@ -7068,7 +7074,7 @@
>        # The default auxiliary directory is the first
>        # of ., .., or ../.. that contains install-sh.
>        # Assume . if install-sh doesn't exist yet.
> -      for my $dir (qw (. .. ../..))
> +      for my $dir (qw(. .. ../..))
>       {
>         if (-f "$dir/install-sh")
>           {

Drop this unrelated change, please.

> @@ -7296,6 +7302,7 @@
>      $macro = rvar ($macro) unless ref $macro;
>      if ($config_libobj_dir)
>        {
> +     # FIXME: Do we need to put @files in @files_installed when --clean?
>       require_file_internal ($macro->rdef ($cond)->location, $mystrict,
>                              $config_libobj_dir, @files);
>        }

Good question.  How can we know after the fact that some AC_LIBSOURCEd
file was installed as-is by automake, and not later changed by the user,
or even hand-written by the user in the first place?  She may get mighty
angry if we remove her precious work.

AFAICS the same issue exists with files installed by 'aclocal -I m4
--install'.

> --- /dev/null 2007-06-19 00:18:28.000000000 +0200
> +++ tests/unbootstrap.test    2007-06-18 15:35:28.000000000 +0200
[...]
> +# Test that automake and aclocal --clean remove all the files installed
> +# by --add-missing.
> +
> +. ./defs || exit 1
> +
> +set -e
> +
> +# Run the test in a subdir.
> +mkdir frob
> +cd frob
> +
> +# The "./." is here so we don't have to mess with subdirs.

I don't understand this comment.  Can it be removed?

> +cat > configure.ac << 'END'
> +AC_INIT([unbootstrap], [1.0])
> +AM_INIT_AUTOMAKE
> +AC_CONFIG_FILES([Makefile])
> +END
> +: > Makefile.am
> +
> +touch list.after
> +find . >list.before

Similar sorting issue as in the Autoconf patch.

> +$ACLOCAL
> +$AUTOMAKE --add-missing
> +$AUTOMAKE --clean
> +$ACLOCAL --clean
> +
> +find . >list.after

The rest of the test: ...

> +res=false
> +
> +diff -u list.before list.after && res=:
> +
> +cd ..
> +rm -rf frob
> +$res

... should be replaced by just
  diff list.before list.after

as -u is not portable, and there is no need to clean up after ourselves.
In fact, being able to play around with the test after it has run is
nice for turning up more issues.  ;-)

For example, if I run
  ../../automake-1.10a --clean

instead of
  ../../automake-1.10a --foreign --clean

(which is what the last $AUTOMAKE expands to), then I get 
| Makefile.am: required file `./NEWS' not found
| Makefile.am: required file `./README' not found
| Makefile.am: required file `./AUTHORS' not found
| Makefile.am: required file `./ChangeLog' not found

which seems a bit odd.  If I run
  ../../automake-1.10a --foreign --clean

twice, then the second invocation outputs
| configure.ac:2: required file `./missing' not found
| configure.ac:2:   `automake --add-missing' can install `missing'
| configure.ac:2: required file `./install-sh' not found
| configure.ac:2:   `automake --add-missing' can install `install-sh'

which is a bit odd as well.  Further,
  ../../automake-1.10a --clean --foreign --add-missing

outputs
| configure.ac:2: installing `./missing'
| configure.ac:2: installing `./install-sh'

but actually those files aren't present afterwards.  It may be sensible
to forbid combining --add-missing with --clean.

Then, there are issues connected with 'aclocal --install'.  But I see
that there is an unrelated internal error to be fixed before (in another
thread).

Anyway the 'aclocal --clean --dry-run' execution path is not tested in
the test yet.  (You can also write more than one test if you prefer to.)

Cheers, and thanks,
Ralf