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

[Pádraig Brady]

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.

multicall-adjustments2.patch
Description: Text Data