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

[Pádraig Brady]

On 06/12/2014 04:51 AM, Alex Deymo wrote:
> Awesome review! Thanks.
> Some comments below.
> 
> On Tue, Jun 10, 2014 at 7:36 AM, Pádraig Brady <address@hidden> wrote:
>> We usally send patches around in git am format to ease processing.
>>   I.E. format-patch --stdout -1 > to-attach.patch
> 
> Ok, I'll attach v2.patch and v3.patch next time.
> 
> 
>>> +# If we are building a single-binary, create symlinks for them.
>>> +install-exec-hook:
>>> +     @for i in $(single_binary_progs); do \
>>> +             $(LN_S) -s coreutils $(DESTDIR)$(bindir)/$$i$(EXEEXT); \
>>> +     done
>>
>> Only doing this at install time is problematic.
>> Ideally we would generate either combined + symlinks _or_ separate binaries
>> in the build dir. That avoids redundant building in the combined case,
>> but more importantly allows running the test suite with `make check`.
>>
>> Note one might be able to leverage the following in configure.ac:
>>
>>   AM_CONDITIONAL([SINGLE_BINARY], [test "$gl_single_binary" = yes])
>>
>> Then the local.mk could be conditionalized with "if SINGLE_BINARY" etc.
> 
> I'll fix the "make check" use case.
> When configured with --enable-single-binary, do you want "make check"
> to run test over the symlinks to coreutils

+1

> or to compile the src/$prog and use that for test?

-1

> "make check" will now compile all the programs
> (src/$prog) and run tests using that.
> 
> There's something I don't understand about how would that conditional
> on configure.ac be useful at all. The autoconf/automake runs first and
> then the user will run ./configure with the flag enabled or not. So,
> automake has to generate all the duplicated rules to build either the
> single binary or the multiple binaries; and configure will emit
> variables that will select the right set of rules as Makefile
> dependencies, so the extra rules won't be used unless needed.

Fair enough.


>>> @@ -150,6 +151,7 @@ src_mv_LDADD = $(LDADD)
>>>  src_nice_LDADD = $(LDADD)
>>>  src_nl_LDADD = $(LDADD)
>>>  src_nohup_LDADD = $(LDADD)
>>> +src_numfmt_LDADD = $(LDADD)
>>
>> Ah we were relying on automake auto deps for that :)
> 
> It sounds like it was missing. Should I keep this change?

No need to separate out.

>>> +# Single binary dependencies
>>> +src_coreutils_SOURCES = src/coreutils.c src/coreutils.h
>>> +src_coreutils_CFLAGS = $(AM_CFLAGS) -Wno-suggest-attribute=noreturn
>>
>> One can't add -Wno-suggest-attribute=noreturn unconditionally.
>> Perhaps we should be marking all these as __attribute__ ((noreturn))
>> and explicitly calling exit() in all main() functions rather than
>> doing an implicit or explicit return. The noreturn attribute could
>> be added easily to the declaration at the top of coreutils.c I expect.
> 
> The noreturn attribute can be added to the gen-single-binary with
> something ugly like:
> "-Dmain=_single_binary_main_${cmd}(int, char**) ATTRIBUTTE_NORETURN;
> int _single_binary_main_${cmd}"
> on the CPPFLAGS of the given ${cmd}. I tried that before, it sort of
> works, except that I need to go an fix all the main() functions. C99
> adds a "return 0" at the end of main() so you can omit the return.
> Some main() functions have return, some other have only exit() on
> every execution path, but some others (like src/getlimits.c) rely on
> this extra return 0. I'd have to fix all those main()s to pick a
> single behavior (like, always exit()) and enforce that adding
> ATTRIBUTTE_NORETURN to every main() to enforce that even if you don't
> compile with --enable-single-binary.

Right. That's what I was suggesting, which is better for consistency anyway.

>> Some more general points...
>>
>> It seems that vdir/dir/ls are not handled appropriately.
>> You probably have to set the ls_mode based on the name being compiled.
> I'll look what's going on there. The default behavior should be that
> they are compiled, right?

y

>> I'd really like the configure and build logic to not
>> change unless the --enable-single-binary option is specified
>> coreutils has to be portable to all sorts of shells/compilers/linkers/...
>>
>> Also it would be useful to be able to exclude certain programs from this.
>> I don't mean to give user the choice of particular programs to build 
>> seperately,
>> rather hardcoding `sort` as being a separate program for example due to
>> threading constraints for example.
> 
> There are a couple of programs using pthreads (according to ldd on my system).
> $ for f in bin/*; do if ldd $f | grep pthread >/dev/null; then echo $f; fi; 
> done
> bin/date
> bin/dd
> bin/dir
> bin/ls
> bin/vdir
> bin/cp
> bin/mv
> bin/install
> bin/pr
> bin/touch

These only link -lrt (due to LIB_GETHRXTIME or LIB_CLOCK_GETTIME)
On GLIBC that pulls in libpthread implicitly:

$ ldd /lib64/librt.so.1
...
  libpthread.so.0 => /lib64/libpthread.so.0 (0x000000375d200000)

> bin/timeout

Links pthreads for timer support on Linux < 2.6 or kFreeBSD

> bin/sort

This is the only multithreaded one

> I'd leave this feature (exclude programs from the coreutils binary and
> build them separatedly) outside this patch since I don't really have a
> use case for that.

The reason I ask is so we could support this now while
we're thinking about it, rather than when we need to
specify program specific CFLAGS or need to otherwise
separate a particular program.

> If none of the selected programs require a lib,
> then the coreutils binary will not link against that library (this is
> now properly handled).
> 
> I'll send v3 patch later today.

thanks,
Pádraig.