Hi Jim, * Jim Meyering wrote on Mon, May 26, 2008 at 05:05:01PM CEST: > > Since part of coreutils' install program (currently ifdef'd out) incurs > a performance hit due to an SELinux implementation problem (I hear that > a solution is in the works), I'm motivated to continue Ralf's work of > minimizing automake's practice of exec'ing install once per installed > file.
You just barely beat me! :-) > This first installment addresses man pages. > I've tested it with coreutils, and as expected, it results in a single > invocation of install for all man/*.1 files, rather than 100 separate > invocations, where they were being done one at a time. This assumes that there is no command line length limit, this could be an issue here. The command line must anyway cope with one instance of `%..._LIST%'. However, for the install command, your code may enlarge that by `$(srcdir)/' for each file. As an example package, OpenMPI has roughly 300 man3 manpages, which with a relative source dir of ../$PACKAGE enlarges the install command line from roughly 9K to 17K. Libtool's LT_CMD_MAX_LEN macro lists 8/12/16K for some systems, it's not clear to me how much safety zone was included there. > This is mainly a heads-up, to see if there are any objections. > If not, I'll write a NEWS entry and a test or two. A NEWS entry and a couple of tests would be great, but fixing the above is needed, too, as well as the nits below. Note that I'm quite OK with reworking this patch myself, if you don't have enough time; but I surely would welcome if your motivation keeps you going. ;-) > --- a/lib/am/mans.am > +++ b/lib/am/mans.am > @@ -72,6 +72,25 @@ if %?TRANS_MANS% > ?HAVE_TRANS? *.%SECTION%*) list="$$list $$i" ;; \ > ?HAVE_TRANS? esac; \ > ?HAVE_TRANS? done; \ > + install_all_at_once=1; \ What about s/install_all_at_once=1/install_multiple=:/ and setting that to false if needed, adjusting everywhere? That way I don't have to nit about using `=' vs. `-eq' below. > + case '$(program_transform_name)' in \ > + s,x,x,) for i in $$list; do \ I've seen s,y,y, being used as well for the trivial case, though git log -p -Sprogram_transform_name shows that that hasn't been in Autoconf's history; also, there is code that specifically checks for "s,x,x,". On anoter note, you're suboptimal here in that you turn off installation all at once for notrans_ manpages as well, which is the new feature to avoid manpage name transformation with --program-* at all. :-) > + case $$i in *.%SECTION%) ;; *) install_all_at_once=0;; esac;done;;\ Please put a space after `esac;'. > + *) install_all_at_once=0 ;; esac; \ Please no space before `;;' (only for consistency with the line above). > + if test $$install_all_at_once = 1; then \ > +## convert names in $$list to be $(srcdir)-relative, if needed > + new_list=; \ > + for i in $$list; do \ > +## Find the file. > + if test -f $$i; then file=$$i; \ > + else file=$(srcdir)/$$i; fi; \ > + new_list="$$new_list $$file"; \ > + done; \ > + list=$$new_list; \ > + echo " $(INSTALL_DATA) $$list $(DESTDIR)$(man%SECTION%dir)"; \ Please put single quotes around $(DESTDIR)$(man%SECTION%dir) in the echo line. > + $(INSTALL_DATA) $$list "$(DESTDIR)$(man%SECTION%dir)"; \ > + exit 0; \ Hmm, `exit $?' would seem more appropriate here, although it would fix but one of the cases. Maybe better to fix them all in a separate patch (including test exposure for each code path). > + fi; \ > for i in $$list; do \ > ## Find the file. > if test -f $$i; then file=$$i; \ Cheers, and thanks for the patch! Ralf
