Re: bug#10697: subdir-objects make clean invokes one rm for each .o file
severity 10697 minor
tags 10697 + patch
thanks
Reference: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10697
Hi Jim, sorry for the awful delay.
On 02/02/2012 05:06 PM, Jim Meyering wrote:
In cppi (http://git.savannah.gnu.org/cgit/cppi.git), I am now using non-
recursive make via subdir-objects, modeled after the way bison does it.
I see that make clean is inefficient: one rm -f per .o file:
rm -fr *.o
rm -f *.o
rm -f lib/calloc.o
rm -f lib/close-stream.o
...
rm -f lib/xstrtol.o
rm -f lib/xstrtoul.o
rm -f src/cppi.o
cppi has so few .o files that it's not a problem, but with hundreds
(coreutils has over 600), it could be noticeable.
Contrast that with its removal of *.o above and of all tests/*.log files
using just one rm invocation each, I think it must simply be an oversight.
Seems like it'd be worth fixing some day.
I agree, and today could be the day :-)
See the attached patch; I will push it by tomorrow if there is no objection
(and if anyone would like to give it a try on a real project, that would be
much appreciated).
Regards,
Stefano
From 06dfdbe38e78c5eedb03f688f0264ec0097a4e21 Mon Sep 17 00:00:00 2001
Message-Id: 06dfdbe38e78c5eedb03f688f0264ec0097a4e21.1339330238.git.stefano.lattar...@gmail.com
From: Stefano Lattarini stefano.lattar...@gmail.com
Date: Sun, 10 Jun 2012 13:38:58 +0200
Subject: [PATCH] subdir-objects: improve make mostlyclean efficiency and
flexibility
Fixes automake bug#10697.
Before this change, the generated Makefile issued one 'rm' invocation
for each subdir object file. Not only was this very inefficient when
there were several such files, but it also caused stale object files
to be left behind when a source file was renamed or removed.
* automake.in (handle_single_transform): When a subdir object is seen,
update '%compile_clean_files' to clean all the compiled objects in its
same subdirectory, and all the libtool compiled objects ('.lo') there
as well is that subdir object is a libtool one.
* t/subobj-clean-pr10697.sh: New test.
* t/subobj-clean-lt-pr10697.sh: Likewise.
* t/list-of-tests.mk: Add them.
* NEWS: Update.
Signed-off-by: Stefano Lattarini stefano.lattar...@gmail.com
---
NEWS |8 ++
automake.in | 28 ---
t/list-of-tests.mk |2 +
t/subobj-clean-lt-pr10697.sh | 169 ++
t/subobj-clean-pr10697.sh| 164
5 files changed, 359 insertions(+), 12 deletions(-)
create mode 100755 t/subobj-clean-lt-pr10697.sh
create mode 100755 t/subobj-clean-pr10697.sh
diff --git a/NEWS b/NEWS
index cf45836..588e8ed 100644
--- a/NEWS
+++ b/NEWS
@@ -60,6 +60,14 @@ New in 1.12.2:
input file. Such a warning will also be present in the next
Autoconf version (2.70).
+* Cleaning rules:
+
+ - Cleaning rules for compiled objects (both plain and libtool) work
+better when subdir objects are involved, not triggering a distinct
+'rm' invocation for each such object. They do so by removing *any*
+compiled object file that is in the same directory of a subdir
+object. See automake bug#10697.
+
New in 1.12.1:
diff --git a/automake.in b/automake.in
index 5cf5a2c..6f8ac0c 100644
--- a/automake.in
+++ b/automake.in
@@ -1963,18 +1963,22 @@ sub handle_single_transform ($%)
err_am '$full' should not contain a '..' component;
}
- # Make sure object is removed by 'make mostlyclean'.
- $compile_clean_files{$object} = MOSTLY_CLEAN;
- # If we have a libtool object then we also must remove
- # the ordinary .o.
- if ($object =~ /\.lo$/)
- {
- (my $xobj = $object) =~ s,lo$,\$(OBJEXT),;
- $compile_clean_files{$xobj} = MOSTLY_CLEAN;
-
- # Remove any libtool object in this directory.
- $libtool_clean_directories{$directory} = 1;
- }
+# Make sure *all* objects files in the subdirectory are
+# removed by make mostlyclean. Not only this is more
+# efficient than listing the object files to be removed
+# individually (which would cause an 'rm' invocation for
+# each of them -- very inefficient, see bug#10697), it
+# would also leave stale object files in the subdirectory
+# whenever a source file there is removed or renamed.
+$compile_clean_files{$directory/*.\$(OBJEXT)} = MOSTLY_CLEAN;
+if ($object =~ /\.lo$/)
+ {
+# If we have a libtool object, then we also must remove
+# any '.lo' objects in its same subdirectory.
+$compile_clean_files{$directory/*.lo} = MOSTLY_CLEAN;
+# Remember to cleanup .libs/ in this directory.
+$libtool_clean_directories{$directory} = 1;
+ }
push (@dep_list,
[PATCH] maint: grammar fixes: s/all these/all of these/
I stumbled across one of these, so fixed all of them and added a new
syntax-check rule in gnulib's maint.mk to discourage recurrence:
http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/30912
From fa0cd34b38729a59a40fa946fc621df3ef0924cd Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sun, 10 Jun 2012 22:03:49 +0200
Subject: [PATCH] maint: grammar fixes: s/all these/all of these/
Run this command:
git grep -li '\all.these\' \
|xargs perl -pi -e 's/\b([Aa])ll these\b/${1}ll of these/'
---
NEWS | 2 +-
doc/automake-history.texi | 6 +++---
doc/automake.texi | 24
doc/fdl.texi | 2 +-
lib/texinfo.tex | 2 +-
old/ChangeLog.01 | 2 +-
old/ChangeLog.11 | 2 +-
t/tap-whitespace-normalization.sh | 2 +-
8 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/NEWS b/NEWS
index cf45836..fd52dea 100644
--- a/NEWS
+++ b/NEWS
@@ -1632,7 +1632,7 @@ New in 1.8:
rule for this target. Running `automake -Woverride' will diagnose
all such overriding definitions.
-It should be noted that almost all these targets support a *-local
+It should be noted that almost all of these targets support a *-local
variant that is meant to supplement the automake-defined rule
(See node `Extending' in the manual). The above rule should
be rewritten as
diff --git a/doc/automake-history.texi b/doc/automake-history.texi
index 5cb3685..7bbdeb4 100644
--- a/doc/automake-history.texi
+++ b/doc/automake-history.texi
@@ -560,7 +560,7 @@ Timeline
It's only the beginning: in two months he will send 192 patches. Then
he would slow down so Tom can catch up and review all this. Initially
-Tom actually read all these patches, then he probably trustingly
+Tom actually read all of these patches, then he probably trustingly
answered OK to most of them, and finally gave up and let Akim apply
whatever he wanted. There was no way to keep up with that patch rate.
@@ -570,7 +570,7 @@ Timeline
been moved :) -- Alexandre Duret-Lutz
@end quotation
-All these patches were sent to and discussed on
+All of these patches were sent to and discussed on
@email{automake@@gnu.org}, so subscribed users were literally drowning in
technical mails. Eventually, the @email{automake-patches@@gnu.org}
mailing list was created in May.
@@ -581,7 +581,7 @@ Timeline
various places in the @command{automake} script itself; this does not
help ensuring a consistent treatment of these rules (for instance
making sure that user-defined rules override Automake's own rules).
-One of Akim's goal was moving all these hard-coded rules to separate
+One of Akim's goal was moving all of these hard-coded rules to separate
@file{Makefile} fragments, so the logic could be centralized in a
@file{Makefile} fragment processor.
diff --git a/doc/automake.texi b/doc/automake.texi
index 939fe44..120dbea 100644
--- a/doc/automake.texi
+++ b/doc/automake.texi
@@ -540,7 +540,7 @@ Use Cases
@cindex @file{amhello-1.0.tar.gz}, use cases
In this section we explore several use cases for the GNU Build System.
-You can replay all these examples on the @file{amhello-1.0.tar.gz}
+You can replay all of these examples on the @file{amhello-1.0.tar.gz}
package distributed with Automake. If Automake is installed on your
system, you should find a copy of this file in
@file{@var{prefix}/share/doc/automake/amhello-1.0.tar.gz}, where
@@ -1567,7 +1567,7 @@ Creating amhello
@command{autoreconf} is a script that calls @command{autoconf},
@command{automake}, and a bunch of other commands in the right order.
If you are beginning with these tools, it is not important to figure
-out in which order all these tools should be invoked and why. However,
+out in which order all of these tools should be invoked and why. However,
because Autoconf and Automake have separate manuals, the important
point to understand is that @command{autoconf} is in charge of
creating @file{configure} from @file{configure.ac}, while
@@ -2868,7 +2868,7 @@ Requirements
that is compatible with @command{make}'s syntax) and furthermore use
@code{AC_SUBST} to ensure that @samp{$@{file@}} is meaningful in a
@file{Makefile}, then @command{automake} will be able to use
-@samp{$@{file@}} to generate all these rules. For instance, here is
+@samp{$@{file@}} to generate all of these rules. For instance, here is
how the Automake package itself generates versioned scripts for its
test suite:
@@ -3560,7 +3560,7 @@ Extending aclocal
and many third party macros are underquoted; and we have to apologize
for this temporary inconvenience. The reason we have to be stricter
is that a future implementation of @command{aclocal} (@pxref{Future of
-aclocal}) will have to temporarily include all these third party
+aclocal}) will have to temporarily include all of these third party
@file{.m4} files, maybe several
Re: [PATCH] maint: grammar fixes: s/all these/all of these/
Hi Jim.
On 06/10/2012 10:08 PM, Jim Meyering wrote:
I stumbled across one of these, so fixed all of them and added a new
syntax-check rule in gnulib's maint.mk to discourage recurrence:
http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/30912
From fa0cd34b38729a59a40fa946fc621df3ef0924cd Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sun, 10 Jun 2012 22:03:49 +0200
Subject: [PATCH] maint: grammar fixes: s/all these/all of these/
Run this command:
git grep -li '\all.these\' \
|xargs perl -pi -e 's/\b([Aa])ll these\b/${1}ll of these/'
---
NEWS | 2 +-
doc/automake-history.texi | 6 +++---
doc/automake.texi | 24
doc/fdl.texi | 2 +-
lib/texinfo.tex | 2 +-
old/ChangeLog.01 | 2 +-
old/ChangeLog.11 | 2 +-
t/tap-whitespace-normalization.sh | 2 +-
8 files changed, 21 insertions(+), 21 deletions(-)
You have my ACK, of course.
Thanks,
Stefano
Re: [PATCH] maint: grammar fixes: s/all these/all of these/
Stefano Lattarini wrote:
Hi Jim.
On 06/10/2012 10:08 PM, Jim Meyering wrote:
I stumbled across one of these, so fixed all of them and added a new
syntax-check rule in gnulib's maint.mk to discourage recurrence:
http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/30912
From fa0cd34b38729a59a40fa946fc621df3ef0924cd Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sun, 10 Jun 2012 22:03:49 +0200
Subject: [PATCH] maint: grammar fixes: s/all these/all of these/
Run this command:
git grep -li '\all.these\' \
|xargs perl -pi -e 's/\b([Aa])ll these\b/${1}ll of these/'
---
NEWS | 2 +-
doc/automake-history.texi | 6 +++---
doc/automake.texi | 24
doc/fdl.texi | 2 +-
lib/texinfo.tex | 2 +-
old/ChangeLog.01 | 2 +-
old/ChangeLog.11 | 2 +-
t/tap-whitespace-normalization.sh | 2 +-
8 files changed, 21 insertions(+), 21 deletions(-)
You have my ACK, of course.
Thanks. pushed.
[PATCH] subdirs: unify rules for cleaning and normal recursive targets
Before this change, the recursive invocation of cleaning targets in the $(SUBDIRS) where done in inverse order, i.e., starting from the last $(SUBDIRS) entry and proceeding towards the first. According to the code comments, this was done ... ... in an attempt to alleviate a problem that can happen when dependencies are enabled. In this case, the .P file in one directory can depend on some automatically generated header in an earlier directory. Since the dependencies are required before any target is examined, make bombs. But this comment does not apply anymore to the current implementation of automatic dependency tracking: the '.Po' and '.Plo' files does not depend on any C header or source file, ever! So it seems that the distinction between normal and cleaning recursive targets is a stale leftover of an older implementation of the automatic dependency tracking. In fact, the Automake History manual seems to confirm this suspect, reading in section First Take on Dependency Tracking: Because each .P file was a dependency of Makefile, this meant that dependency tracking was done eagerly by make. For instance, make clean would cause all the dependency files to be updated, and then immediately removed. This eagerness also caused problems with some configurations; if a certain source file could not be compiled on a given architecture for some reason, dependency tracking would fail, aborting the entire build. and then, in the following section Dependencies As Side Effects: In this approach, the .P files were included using the -include command, which let us create these files lazily. This avoided the make clean problem. So the distinction between normal and cleaning recursive targets has likely been obsolete since by then already. We can thus remove such distinction, thus reducing some complications and duplication in our rules. Not however that we still keep '$(RECURSIVE_TARGETS)' and '$(RECURSIVE_CLEAN_TARGETS)' as two distinct variables, to ensure a better backward-compatibility for any user-defined rules that happen to use those variables. * lib/am/subdirs.am ($(RECURSIVE_CLEAN_TARGETS), $(CLEAN_TARGETS)): Merge their recipes. * t/subdir-distclean.sh: New test, check that ./configure make make distclean is actually a no-op, even when conditional SUBDIRS are involved. * t/list-of-tests.mk: Add it. Signed-off-by: Stefano Lattarini stefano.lattar...@gmail.com --- lib/am/subdirs.am | 54 +- t/list-of-tests.mk|1 + t/subdir-distclean.sh | 103 + 3 files changed, 113 insertions(+), 45 deletions(-) create mode 100755 t/subdir-distclean.sh diff --git a/lib/am/subdirs.am b/lib/am/subdirs.am index 1d1295e..3fc2888 100644 --- a/lib/am/subdirs.am +++ b/lib/am/subdirs.am @@ -32,7 +32,7 @@ AM_RECURSIVE_TARGETS += $(RECURSIVE_TARGETS:-recursive=) \ # (which will cause the Makefiles to be regenerated when you run 'make'); # (2) otherwise, pass the desired values on the 'make' command line. -$(RECURSIVE_TARGETS): +$(RECURSIVE_TARGETS) $(RECURSIVE_CLEAN_TARGETS): ## Using $failcom allows -k to keep its natural meaning when running a ## recursive rule. @fail= failcom='exit 1'; \ @@ -44,7 +44,14 @@ $(RECURSIVE_TARGETS): done; \ dot_seen=no; \ target=`echo $@ | sed s/-recursive//`; \ - list='$(SUBDIRS)'; for subdir in $$list; do \ +## For distclean and maintainer-clean we make sure to use the full +## list of subdirectories. We do this so that 'configure; make +## distclean' really is a no-op, even if SUBDIRS is conditional. + case $@ in \ + distclean-* | maintainer-clean-*) list='$(DIST_SUBDIRS)' ;; \ + *) list='$(SUBDIRS)' ;; \ + esac; \ + for subdir in $$list; do \ echo Making $$target in $$subdir; \ if test $$subdir = .; then \ dot_seen=yes; \ @@ -65,46 +72,3 @@ clean: clean-recursive distclean: distclean-recursive maintainer-clean: maintainer-clean-recursive -## We run all 'clean' targets in reverse order. Why? It's an attempt -## to alleviate a problem that can happen when dependencies are -## enabled. In this case, the .P file in one directory can depend on -## some automatically generated header in an earlier directory. Since -## the dependencies are required before any target is examined, make -## bombs. -$(RECURSIVE_CLEAN_TARGETS): -## Using $failcom allows -k to keep its natural meaning when running a -## recursive rule. - @fail= failcom='exit 1'; \ - for f in x $$MAKEFLAGS; do \ - case $$f in \ - *=* | --[!k]*);; \ - *k*) failcom='fail=yes';; \ - esac; \ - done; \ - dot_seen=no; \ -## For distclean and maintainer-clean we make sure to use the full -## list of subdirectories. We do this so that 'configure; make -## distclean' really is a no-op, even if SUBDIRS is conditional.
[PATCH 0/2] subdirs: unify rules for cleaning and normal recursive targets
I've reworked and improved the commit message a bit, and added a new test case for extra-safety (in a separate, preparatory patch). This is what I intend to push soon enough (in a day or two). Regards, Stefano -*-*-*- Stefano Lattarini (2): tests: add a demo test on C support subdirs: unify rules for cleaning and normal recursive targets lib/am/subdirs.am | 54 +++ t/c-demo.sh | 179 + t/list-of-tests.mk|2 + t/subdir-distclean.sh | 103 4 files changed, 293 insertions(+), 45 deletions(-) create mode 100755 t/c-demo.sh create mode 100755 t/subdir-distclean.sh -- 1.7.9.5
[PATCH 1/2] tests: add a demo test on C support
Showing and testing non-trivial use of C support, and its interaction
with other features.
* t/c-demo.sh: New test.
* t/list-of-tests.mk: Add it.
Signed-off-by: Stefano Lattarini stefano.lattar...@gmail.com
---
t/c-demo.sh| 179
t/list-of-tests.mk |1 +
2 files changed, 180 insertions(+)
create mode 100755 t/c-demo.sh
diff --git a/t/c-demo.sh b/t/c-demo.sh
new file mode 100755
index 000..ed2a109
--- /dev/null
+++ b/t/c-demo.sh
@@ -0,0 +1,179 @@
+#! /bin/sh
+# Copyright (C) 2012 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see http://www.gnu.org/licenses/.
+
+# Demo on C support, also testing automatic dependency tracking,
+# conditional SUBDIRS and convenience libraries.
+
+required=cc
+am_create_testdir=empty
+. ./defs || Exit 1
+
+cat configure.ac 'END'
+AC_INIT([GNU C Demo], [22.3.2], [bug-autom...@gnu.org])
+AC_CONFIG_SRCDIR([tests/test.test])
+AC_CONFIG_AUX_DIR([build-aux])
+AM_INIT_AUTOMAKE
+AC_PROG_CC
+AM_PROG_CC_C_O
+AM_PROG_AR
+AC_PROG_RANLIB
+AM_CONDITIONAL([RUN_TESTS], [test x$run_tests != xno])
+AC_CONFIG_FILES([Makefile src/Makefile lib/Makefile tests/Makefile])
+AC_OUTPUT
+END
+
+if cross_compiling; then
+ run_tests=no
+else
+ run_tests=yes
+fi
+export run_tests
+
+mkdir build-aux lib src tests
+
+cat Makefile.am 'END'
+SUBDIRS = lib src
+
+if RUN_TESTS
+SUBDIRS += tests
+endif
+
+.PHONY: test-objs
+check-local: test-objs
+test-objs:
+ test -f src/zardoz-main.$(OBJEXT)
+ test -f lib/foo.$(OBJEXT)
+ test -f lib/bar.$(OBJEXT)
+END
+
+cat src/Makefile.am 'END'
+bin_PROGRAMS = zardoz
+zardoz_SOURCES = main.c
+zardoz_LDADD = $(top_builddir)/lib/lib-convenience.a
+zardoz_CPPFLAGS = -I$(top_builddir)/lib -I$(top_srcdir)/lib
+END
+
+cat lib/Makefile.am 'END'
+noinst_LIBRARIES = lib-convenience.a
+lib_convenience_a_SOURCES = foo.c
+lib_convenience_a_SOURCES += bar.c
+dist_lib_convenience_a_SOURCES = bar.h
+nodist_lib_convenience_a_SOURCES = foo.h
+
+# We want this to be auto-generated an removed by make clean, to
+# ensure that cleaning rules work correctly; an older implementation
+# of automatic dependency tracking support suffered of weaknesses in
+# this situation, see the historical comments reported in:
+# http://lists.gnu.org/archive/html/automake-patches/2012-06/msg00033.html
+foo.h: $(srcdir)/foo.c
+ sed -n 's/.*foo *(.*/;/p' $(srcdir)/foo.c $@-t
+ test 1 -eq `wc -l $@-t`
+ chmod a-w $@-t mv -f $@-t $@
+BUILT_SOURCES = foo.h
+CLEANFILES = $(BUILT_SOURCES)
+
+check-local:
+ test -f ${top_srcdir}/tests/test.test
+END
+
+cat tests/Makefile.am 'END'
+AUTOMAKE_OPTIONS = parallel-tests
+TEST_LOG_COMPILER = $(SHELL)
+TESTS = test.test
+EXTRA_DIST = $(TESTS)
+END
+
+cat tests/test.test 'END'
+#!/bin/sh
+set -x; set -e;
+../src/zardoz
+test `../src/zardoz` = 'Foo, Bar!'
+END
+
+$ACLOCAL
+$AUTOCONF
+$AUTOMAKE --add-missing
+
+test -f build-aux/depcomp
+test -f build-aux/compile # We have per-target flags on C sources.
+
+# Don't reject slow dependency extractors.
+./configure --enable-dependency-tracking
+
+cat src/main.c 'END'
+#include foo.h
+#include bar.h
+int main (void)
+{
+ printf (%s, %s!\n, foo (), bar ());
+ return 0;
+}
+END
+
+cat lib/foo.c 'END'
+#include foo.h
+static char s[4];
+volatile char *foo (void)
+{
+ s[0] = 'F';
+ s[1] = s[2] = 'o';
+ s[3] = '\0';
+ return s;
+}
+END
+
+cat lib/bar.c 'END'
+#include bar.h
+const char *bar (void)
+{
+ return BARBAR;
+}
+END
+
+cat lib/bar.h 'END'
+#define BARBAR Bar
+const char *bar (void);
+END
+
+$MAKE
+ls -l . src lib # For debugging.
+$MAKE test-objs
+
+VERBOSE=x $MAKE check
+if cross_compiling; then
+ test ! -f tests/test-suite.log
+ test ! -f tests/test.log
+else
+ test -f tests/test-suite.log
+ grep 'Foo, Bar!' tests/test.log
+fi
+
+$MAKE distcheck
+
+if ! cross_compiling ! grep [ $tab]depmode=none Makefile; then
+ # Let's check automatic dependency tracking.
+ sed 's/^\(#define BARBAR \).*/\1 Zap/' lib/bar.h t
+ mv -f t lib/bar.h
+ $MAKE
+ ./src/zardoz
+ test `./src/zardoz` = 'Foo, Zap!'
+fi
+
+$MAKE clean
+test ! -f lib/foo.h
+test -f lib/bar.h
+
+:
diff --git a/t/list-of-tests.mk b/t/list-of-tests.mk
index a79f8cb..31b6871 100644
--- a/t/list-of-tests.mk
+++ b/t/list-of-tests.mk
@@ -322,6 +322,7 @@ t/copy.sh \
t/cscope.tap \
t/cscope2.sh \
t/cscope3.sh \
+t/c-demo.sh \
t/cxx.sh \
t/cxx2.sh \
