[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Patch proposal: Add --clean options to unbootstrap a project.
|
From: |
Benoit Sigoure |
|
Subject: |
Re: Patch proposal: Add --clean options to unbootstrap a project. |
|
Date: |
Mon, 25 Jun 2007 00:41:48 +0200 |
|
User-agent: |
Internet Messaging Program (IMP) H3 (4.1.3) |
Quoting Ralf Wildenhues <address@hidden>:
Hello Benoit,
Thanks for your continuing work on this!
Thank *you* for your thorough and relevant reviews. It's thanks to
people like you that Free software is and remains high-quality
software. So thank you for investing your precious time in this
change (which is not critical, not release-blocker, not fixing any
bug...).
* 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)?
Nope, you already pointed out that the name should be uniform in
autoconf/automake/libtool and now it's named 'unbootstrap' everywhere
but we can change this to 'bootclean' if you prefere.
--- 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.
Fixed by the last patch for autoconf which provides autom4te with a
--clean option.
+ }
+ }
--- 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.
Uh, OK, sorry but it was making ViM go wild :)
I should try Emacs' perl-mode some day (I am ambidextrous, I can code
with both editors :D).
@@ -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'.
Sorry I have no idea, maybe I need to take more time to think about
this. Meanwhile we can leave the FIXME here and come back to it
later. We better do not enough cleaning rather than too much.
--- /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?
Neither do I, so I removed it :)
+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.
ACK
+$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
ACK
as -u is not portable, and there is no need to clean up after ourselves.
ACK
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.
All of these issues were obvious, I wonder why I missed them. Fixed, thanks.
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).
What internal error?
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.)
Alright so here is a revised patch. Changes since the previous one:
I Removed the hard-coded reference to `autom4te.cache'. Instead
trace_used_macros ends up invoking $trace --clean ($trace being
autom4te) when in clean mode and not-dry-run.
Now --clean and --add-missing are not accepted together. Also the
messages raised by not using `foreign' when cleaning are now ignored.
I didn't investigate further to see if it was relevant to ignore other
messages. Maybe it would be easier/safer to set_strictness
('foreign') if $clean; WDYT?
The test has been enhanced to test aclocal --clean --dry-run, deal
with the find-sorting-issue and diff -u being non-portable. The files
of the test are left over for easier debugging but the test starts by
making sure that the folder is fresh. It would be annoying if a
previous failed-run of the test could interfere with a new run.
I have one last concern though, but for a later patch if possible.
Now Autoconf (autoreconf) relies on the fact that Automake provide a
--clean option and also the other way around (since now aclocal uses
autom4te --clean). The new releases of Autoconf and Automake won't
make it at the same time in the repositories of the various
distributions. For instance when both autoconf 2.61 and automake 1.10
were released, the former was available several weeks before the later
(especially in MacPorts). Should we do something about this (such as
autoreconf testing that aclocal and automake have '--clean' in their
--help message)? Or should we just say that because the feature is
new, you need the good version of the various tools to use it and let
autoreconf --clean fail miserably?
Cheers,
--
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
automake-bootclean.patch
Description: Text document