Re: [PATCH v2] build: Option for building all tools in a single binary

[Bernhard Voelker]

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.


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.


2. Typo in commit msg:

> When installing, the makefile will create etiher symlinks or

s/etiher/either/

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/

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

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)

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


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

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

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

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

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.

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

> 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.

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


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.

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?

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

Thanks & have a nice day,
Berny