On 07/07/2014 12:41 AM, Bernhard Voelker wrote:
> On 07/05/2014 03:40 PM, Pádraig Brady wrote:
>> On 07/04/2014 05:06 PM, Pádraig Brady wrote:
>> Rolled up for easy application on master:
>> http://www.pixelbeat.org/cu/single-binary_v8.patch
> 
> Thanks for squashing into one patch.
> Here some comments - the cases are marked with either
> "[{normal,shebang,symlinks} case]" depending on whether
> configure was invoked without --enable-single-binary,
> with --s...=shebang or with ...=symlinks.

Excellent review again.
Details below...

> 
> 
> 1. "make syntax-check" is boken [normal case]
> 
>   $ git am single-binary_v8.patch
>   $ git clean -xdfq
>   $ ./bootstrap
>   $ ./configure --quiet
>   $ make
>   $ make syntax-check
>   make: *** No rule to make target `src/coreutils_', needed by 
> `src/coreutils'.  Stop.

fixed

> 
> 
> 2. Typo in commit msg:
> 
>> When installing, the makefile will create etiher symlinks or
> 
> s/etiher/either/

fixed

> 
> 3. build-aux/gen-single-binary.sh:
> 
>>> # We need to duplicate the specific rules to build each program into a new
>> # static library target. We can't reuse the existing target since we need to
>> # create a .a file instead of linking the program. We can't do this at
>> # ./configure since the file names need to available when automake runs to 
>> let
>> # it generate all the required rules in Makefile.in.
> 
> s/to available/to be available/

fixed

> 
> 4. Dependencies don't seem to be tracked 100% yet [shebang case]
> 
>   $ git clean -xdfq
>   $ ./bootstrap
>   $ ./configure --quiet --enable-single-binary=shebangs
>   $ make -j
>   ...
>   src/coreutils.c:37:23: fatal error: coreutils.h: No such file or directory
>    #include "coreutils.h"
>                        ^
>   compilation terminated.
>   make[2]: *** [src/src_coreutils-coreutils.o] Error 1

fixed

> 
> After a second 'make -j', the build is okay.
> 
> 
> 5. Some remainders are not yet in .gitignore [shebang case]
> 
>   $ git status
>   # On branch allinone
>   # Untracked files:
>   #   (use "git add <file>..." to include in what will be committed)
>   #
>   #       build-aux/git-version-gen
>   #       intl/
>   #       man/dynamic-deps.mk
>   #       src/coreutils_shebangs
>   nothing added to commit but untracked files present (use "git add" to track)

fixed

> 
> 6. "make" dist complains about duplicate name [normal case]
> This one is very likely independent of the single-binary patch.
> 
>     GEN      THANKS
>   ./thanks-gen: THANKS.in: duplicate name: P�draig Brady

separate issue

> 
> 
> 7. Several tests skipped [shebang case]
> E.g.
> 
>   chroot-fail.sh: skipped test: required program(s) not built
>   SKIP: tests/misc/chroot-fail.sh
> 
>   coreutils.sh: skipped test: multicall binary is not built
>   SKIP: tests/misc/coreutils.sh

fixed

> 
> Reason: the variable $built_programs now only contains "coreutils",
> thus leading to a wrong result for e.g. require_built_ function.
> 
> 8. Test case tests/misc/coreutils.sh skipped [shebang case]
> Typo:
> 
>> test -x "$abs_top_builddir}/src/coreutils" \
>>  || skip_ "multicall binary is not built"
> 
> s/}//
> 
> 9. Test case tests/misc/coreutils.sh: non-standard compare_ call
> 
>> compare_ out exp || fail=1
> 
> s/compare_/compare/
> s/out exp/exp out/
> 
> It should read
> compare_ out exp || fail=1

various issues with that test fixed up

> 
> 10. man/coreutils.1 doesn't get built [symlinks case]
> ... at least not with a plain 'make'.

fixed

> 11. "make install" doesn't complain when man/coreutils.1 is missing [shebang 
> case]

fixed

> 12. man/coreutils.1: should avoid having the full path here:
> 
>> Try: '/tmp/coreutils-8.22.142-d6383/src/coreutils PROGRAM_NAME --help' for 
>> help on the particular program.
> 
> I noticed the same in several man/*.1 files now.

Separate issue:

$ grep -l $PWD man/*.1
man/basename.1
man/cat.1
man/chgrp.1
man/chown.1
man/comm.1
man/coreutils.1
man/dirname.1
man/nohup.1
man/numfmt.1
man/rm.1

> 
> 13. man/coreutils.1: hint about info page wrong:
> That node does not exist:

fixed

>> info coreutils 'coreutils invocation'
> 
> 14. man/local.mk: writing non-atomically into target:
> 
>> +man/dynamic-deps.mk: Makefile
>> +    $(AM_V_GEN)rm -f $@
>> +    $(AM_V_at)for man in $(ALL_MANS); do                            \
>> ...
>> +    done > $@
> 
> It should write to temporary file and do a final mv(1) instead.

fixed

> 
> 15. src/coreutils-{arch,dir,vdir}.c wrapper:
> Why don't we do this also in non-single-binary case? ;-)

leaving as is for now

> 16. src/coreutils.c: search for right main program should have 'else'
> 
>>   /* Lookup the right main program.  */
>> #define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) \
>>   if (STREQ (prog_name_str, prog_name)) \
>>     prog_main = _single_binary_main_##main_name;
>> #include "coreutils.h"
>> #undef SINGLE_BINARY_PROGRAM
> 
> This will STREQ thru all ~100 program names strings, even if the first
> one already matched.

fixed

> 17. Re calling exit() instead of return in main function:
>>  * src/kill.c: Changes to call exit() in main.
> 
> Shouldn't we have a syntax-check rule to ensure this?

leaving as is for now

> Although this is quite a big list, I think you're on a good way!

Changes are attached to this email.

Rolled up patch is at:
http://www.pixelbeat.org/cu/single-binary_v9.patch

One other change to consider is that we might
change `coreutils --coreutils-prog=` to `coreutils --program`
as the former is a bit long/awkward/redundant?

Another thing I just thought of is that we should change the ENOENT
warning in coreutils.c to something explicit as the error
pertains to internal functionality, rather than optional
external links/scripts etc.

thanks again,
Pádraig.
>From cd99ef63483989fd89b85e68c1b6fac21cc902d6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 7 Jul 2014 09:46:12 +0100
Subject: [PATCH 1/2] build: fix multicall issues identified by Bernhard

* src/local.mk (EXTRA_src_coreutils_DEPS): Only set for multicall.
(built_SOURCES): Add coreutils.h so generated always.
(src_coreutils_SOURCES): Leave at default so that set appropriately
when --enable-single-binary is not set (significant with syntax-check).
(src_coreutils_CFLAGS): Add -DSINGLE_BINARY to allow building
coreutils.c without warnings.
* coreutils.c: Avoid using the SINGLE_BINARY_PROGRAM without the
SINGLE_BINARY define to avoid an unused macro warning.
* build-aux/gen-single-binary.sh: s/to available/to be available/.
* tests/misc/coreutils.sh: Fix typos. Now not always skipped.
Also skip correctly even after make syntax-check when all
binaries are built.
* tests/local.mk: Add $single_binary_progs to support
require_built_() from init.cfg
* man/local.mk: Update dynamic-deps.mk atomically.
* build-aux/gen-lists-of-programs.sh: Move "coreutils" from
disabled_by_default_progs to build_if_possible_progs,
so that "coreutils" is not listed in the list that
can be enabled by the configure user with --enable-install-program
---
 .gitignore                     |    2 ++
 build-aux/gen-single-binary.sh |    8 ++++----
 configure.ac                   |    1 +
 doc/coreutils.texi             |   19 +++++++++++++++++++
 man/local.mk                   |    6 ++++--
 src/coreutils.c                |   29 ++++++++++++++++++-----------
 src/local.mk                   |   23 ++++++++++++-----------
 tests/local.mk                 |    2 +-
 tests/misc/coreutils.sh        |    6 +++---
 9 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/.gitignore b/.gitignore
index 5869791..02a61e8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -134,6 +134,7 @@
 /m4/xsize.m4
 /maint.mk
 /man/*.1
+/man/dynamic-deps.mk
 /po/*.gmo
 /po/*.po
 /po/.gitignore
@@ -157,6 +158,7 @@
 /po/stamp-po
 /src/coreutils.h
 /src/coreutils_symlinks
+/src/coreutils_shebangs
 /src/cu-progs.mk
 /src/fs-latest-magic.h
 /src/libsinglebin_*.a
diff --git a/build-aux/gen-single-binary.sh b/build-aux/gen-single-binary.sh
index f3958bb..4635706 100755
--- a/build-aux/gen-single-binary.sh
+++ b/build-aux/gen-single-binary.sh
@@ -6,10 +6,10 @@
 # We need to duplicate the specific rules to build each program into a new
 # static library target. We can't reuse the existing target since we need to
 # create a .a file instead of linking the program. We can't do this at
-# ./configure since the file names need to available when automake runs to let
-# it generate all the required rules in Makefile.in. The configure step will
-# select which ones will be used to build, but they need to be generated
-# beforehand.
+# ./configure since the file names need to be available when automake runs
+# to let it generate all the required rules in Makefile.in.  The configure
+# step will select which ones will be used to build, but they need to be
+# generated beforehand.
 #
 # Instead of maintaining a duplicated list of rules, we generate the
 # single-binary required rules based on the normal configuration found on
diff --git a/configure.ac b/configure.ac
index 27caffd..7a098a9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -509,6 +509,7 @@ single_binary_libs=
 single_binary_deps=
 single_binary_install_type=
 if test "$gl_single_binary" != no; then
+  man1_MANS="$man1_MANS man/coreutils.1"
   # Convert the list to a space separated list
   gl_single_binary_exceptions=`echo $gl_single_binary_exceptions | tr ',' ' '`
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 96220c3..19a523d 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -47,6 +47,7 @@
 * chroot: (coreutils)chroot invocation.         Specify the root directory.
 * cksum: (coreutils)cksum invocation.           Print POSIX CRC checksum.
 * comm: (coreutils)comm invocation.             Compare sorted files by line.
+* coreutils: (coreutils)coreutils invocation.   Multi-call program.
 * cp: (coreutils)cp invocation.                 Copy files.
 * csplit: (coreutils)csplit invocation.         Split by context.
 * cut: (coreutils)cut invocation.               Print selected parts of lines.
@@ -229,6 +230,7 @@ Common Options
 * Traversing symlinks::          Traversing symlinks to directories
 * Treating / specially::         Treating / specially
 * Standards conformance::        Standards conformance
+* coreutils invocation::         Multi-call binary invocation
 
 Output of entire files
 
@@ -771,6 +773,7 @@ name.
 * Treating / specially::        --preserve-root and --no-preserve-root.
 * Special built-in utilities::  @command{break}, @command{:}, @dots{}
 * Standards conformance::       Conformance to the POSIX standard.
+* coreutils invocation::        Multi-call binary invocation.
 @end menu
 
 
@@ -1494,6 +1497,22 @@ that assumes an older version of POSIX and uses @samp{sort +1}
 or @samp{tail +10}, you can work around any compatibility problems by setting
 @samp{_POSIX2_VERSION=199209} in your environment.
 
+@node coreutils invocation
+@section @command{coreutils}: Multi-call binary
+
+@pindex multicall
+@cindex combined
+@cindex calling combined multi-call binary
+
+@command{coreutils} invokes an individual utility, either
+implicitly selected by the last component of @samp{argv[0]},
+or by explicitly calling @command{coreutils} with the
+@option{--coreutils-prog} option.  Synopsis:
+
+@example
+coreutils @option{--coreutils-prog=PROGRAM} @dots{}
+@end example
+
 @node Output of entire files
 @chapter Output of entire files
 
diff --git a/man/local.mk b/man/local.mk
index 3b8fd6d..bd18f90 100644
--- a/man/local.mk
+++ b/man/local.mk
@@ -59,7 +59,7 @@ $(ALL_MANS): $(mandeps)
 # being installed will have the right dependency for the manpages.
 CLEANFILES += man/dynamic-deps.mk
 man/dynamic-deps.mk: Makefile
-	$(AM_V_GEN)rm -f $@
+	$(AM_V_GEN)rm -f $@ $@-t
 	$(AM_V_at)for man in $(ALL_MANS); do				\
 		name=$${man:4: -2} ; # Space is important		\
 		case $$name in						\
@@ -74,7 +74,9 @@ man/dynamic-deps.mk: Makefile
 			*)						\
 				echo $$man: src/$$prog$(EXEEXT);;	\
 		esac							\
-	done > $@
+	done > $@-t							\
+	&& chmod a-w $@-t						\
+	&& mv $@-t $@
 
 # Include the generated man dependencies.
 -include man/dynamic-deps.mk
diff --git a/src/coreutils.c b/src/coreutils.c
index 9bb3ab4..40342bf 100644
--- a/src/coreutils.c
+++ b/src/coreutils.c
@@ -29,13 +29,15 @@
 #include "system.h"
 #include "error.h"
 
+#ifdef SINGLE_BINARY
 /* Declare the main function on each one of the selected tools.  This name
    needs to match the one passed as CFLAGS on single-binary.mk (generated
    by gen-single-binary.sh). */
-#define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) \
+# define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) \
   int _single_binary_main_##main_name (int, char**) ATTRIBUTE_NORETURN;
-#include "coreutils.h"
-#undef SINGLE_BINARY_PROGRAM
+# include "coreutils.h"
+# undef SINGLE_BINARY_PROGRAM
+#endif
 
 /* The official name of this program (e.g., no 'g' prefix).  */
 #define PROGRAM_NAME "coreutils"
@@ -72,14 +74,16 @@ Execute the PROGRAM_NAME built-in program with the given PARAMETERS.\n\
 
       printf ("\n\
 Built-in programs:\n"
-#define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) " " prog_name_str
-#include "coreutils.h"
-#undef SINGLE_BINARY_PROGRAM
+#ifdef SINGLE_BINARY
+# define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) " " prog_name_str
+# include "coreutils.h"
+# undef SINGLE_BINARY_PROGRAM
+#endif
   "\n");
 
       printf (_("\
 \n\
-Try: '%s PROGRAM_NAME --help' for help on the particular program.\n"),
+Use: '%s --coreutils-prog=PROGRAM_NAME --help' for individual program help.\n"),
               program_name);
       emit_ancillary_info ();
     }
@@ -95,12 +99,15 @@ launch_program (const char *prog_name, int prog_argc, char **prog_argv)
   if (!prog_argc || !prog_argv || !prog_argv[0] || !prog_name)
     return;
 
+#ifdef SINGLE_BINARY
+  if (false);
   /* Lookup the right main program.  */
-#define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) \
-  if (STREQ (prog_name_str, prog_name)) \
+# define SINGLE_BINARY_PROGRAM(prog_name_str, main_name) \
+  else if (STREQ (prog_name_str, prog_name)) \
     prog_main = _single_binary_main_##main_name;
-#include "coreutils.h"
-#undef SINGLE_BINARY_PROGRAM
+# include "coreutils.h"
+# undef SINGLE_BINARY_PROGRAM
+#endif
 
   if (! prog_main)
     return;
diff --git a/src/local.mk b/src/local.mk
index e1a5632..e35d286 100644
--- a/src/local.mk
+++ b/src/local.mk
@@ -106,6 +106,9 @@ src_cksum_LDADD = $(LDADD)
 src_comm_LDADD = $(LDADD)
 src_nproc_LDADD = $(LDADD)
 src_cp_LDADD = $(LDADD)
+if !SINGLE_BINARY
+src_coreutils_LDADD = $(LDADD)
+endif
 src_csplit_LDADD = $(LDADD)
 src_cut_LDADD = $(LDADD)
 src_date_LDADD = $(LDADD)
@@ -397,22 +400,20 @@ src_libstdbuf_so_LDADD = $(LIBINTL)
 src_libstdbuf_so_LDFLAGS = -shared
 src_libstdbuf_so_CFLAGS = -fPIC $(AM_CFLAGS)
 
+BUILT_SOURCES += src/coreutils.h
 if SINGLE_BINARY
 # Single binary dependencies
-src_coreutils_SOURCES = src/coreutils.c src/coreutils.h
-src_coreutils_CFLAGS = $(AM_CFLAGS)
-src_coreutils_LDFLAGS = $(AM_LDFLAGS)
+src_coreutils_CFLAGS = -DSINGLE_BINARY $(AM_CFLAGS)
+#src_coreutils_LDFLAGS = $(AM_LDFLAGS)
 src_coreutils_LDADD = $(single_binary_deps) $(LDADD) $(single_binary_libs)
 src_coreutils_DEPENDENCIES = $(LDADD) $(single_binary_deps)
 
 include $(top_srcdir)/src/single-binary.mk
 
-endif SINGLE_BINARY
-
 # Creates symlinks or shebangs to the installed programs when building
-# coreutils single binary.  This doesn't do anything when $(single_binary_progs)
-# is empty.
+# coreutils single binary.
 EXTRA_src_coreutils_DEPENDENCIES = src/coreutils_$(single_binary_install_type)
+endif SINGLE_BINARY
 
 CLEANFILES += src/coreutils_symlinks
 src/coreutils_symlinks: Makefile
@@ -567,10 +568,10 @@ src/version.h: Makefile
 CLEANFILES += src/coreutils.h
 src/coreutils.h: Makefile
 	$(AM_V_GEN)rm -f $@
-	$(AM_V_at)for prog in $(single_binary_progs); do		       \
-		prog=`basename $$prog`					     ; \
-		main=`echo $$prog | tr '[' '_'`				     ; \
-		echo "SINGLE_BINARY_PROGRAM(\"$$prog\", $$main)"	     ; \
+	$(AM_V_at)for prog in $(single_binary_progs); do	\
+	  prog=`basename $$prog`;				\
+	  main=`echo $$prog | tr '[' '_'`;			\
+	  echo "SINGLE_BINARY_PROGRAM(\"$$prog\", $$main)";	\
 	done | sort > $@t
 	$(AM_V_at)chmod a-w $@t
 	$(AM_V_at)mv $@t $@
diff --git a/tests/local.mk b/tests/local.mk
index d1e6f59..e0f1f84 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -65,7 +65,7 @@ TESTS_ENVIRONMENT =				\
   abs_top_builddir='$(abs_top_builddir)'	\
   abs_top_srcdir='$(abs_top_srcdir)'		\
   abs_srcdir='$(abs_srcdir)'			\
-  built_programs='$(built_programs)'		\
+  built_programs='$(built_programs) $(single_binary_progs)' \
   host_os=$(host_os)				\
   host_triplet='$(host_triplet)'		\
   srcdir='$(srcdir)'				\
diff --git a/tests/misc/coreutils.sh b/tests/misc/coreutils.sh
index ad39c8b..2935bbe 100755
--- a/tests/misc/coreutils.sh
+++ b/tests/misc/coreutils.sh
@@ -20,12 +20,12 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ coreutils
 
-test -x "$abs_top_builddir}/src/coreutils" \
- || skip_ "multicall binary is not built"
+test -s "$abs_top_builddir/src/coreutils.h" \
+ || skip_ "multicall binary is disabled"
 
 # Yes outputs all its params so is good to verify argv manipulations
 echo 'y' > exp
 coreutils --coreutils-prog=yes | head -n10 | uniq > out
-compare_ out exp || fail=1
+compare exp out || fail=1
 
 Exit $fail
-- 
1.7.7.6


>From 2fcc5ffa9f6bb3217297feb245e1844a2e601a4b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 7 Jul 2014 16:31:01 +0100
Subject: [PATCH 2/2] build: disallow --enable-single-binary=symlinks
 --program-prefix combo

* configure.ac: Since we currently only match on internal names,
disallow this combination for now.
---
 configure.ac |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index 7a098a9..b48beae 100644
--- a/configure.ac
+++ b/configure.ac
@@ -235,6 +235,12 @@ AC_ARG_ENABLE([single-binary-exceptions],
   [gl_single_binary_exceptions=$enableval],
   [gl_single_binary_exceptions=]
 )
+if test "$gl_single_binary" = "symlinks"; then
+  if ! test "x$program_transform_name" = "x"; then
+    AC_MSG_ERROR([program name transformations are not currently supported
+                  with --enable-single-binary=symlinks.])
+  fi
+fi
 AM_CONDITIONAL([SINGLE_BINARY], [test "$gl_single_binary" != no])
 
 AC_FUNC_FORK
-- 
1.7.7.6

Reply via email to