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

[Alex Deymo]

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 or to compile the src/$prog
and use that for test? "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.


>> --- a/bootstrap
>> +++ b/bootstrap
>> @@ -807,6 +807,9 @@ version_controlled_file() {
>>    fi
>>  }
>>
>> +# Regenerate src/single-binary.sh
>> +./build-aux/gen-single-binary.sh src/local.mk
>
> Where we generate this file is important and the crux of the patch.
> Now bootstrap is shared between various gnulib using projects so
> editing that script directly is not the best place I think.
> Without checking, it seems like updating the coreutils specific
> bootstrap_post_import_hook() in bootstrap.conf would be more appropriate.
>
> Things that should be checked when generating single-binary.mk early like this
> is that it handles and avoids builds for --enable-no-install-program=PROG_LIST
> and that the generated .mk file is included in the `make dist` tarball.
> If the .mk does need to be conditionalized at configure time then the
> generation could move to configure.ac, but note the much greater portability
> requirements in that case (including within gen-single-binary.sh).

The goal of gen-single-binary.sh is to not maintain two sets of rules
in .mk files for the single-binary and normal case. This should be
included in "make dist" tarball and has the rules to generate every
program, as src/local.mk does. The --enable-no-install-program is
handled by ./configure, which when used in conjunction with
--enable-single-binary puts the list of desired tools in the right
place. I managed to get the dependencies fixed so it will only compile
only the ones you need.


>> +LOCAL_MK=$1
>> +SINGLE_BINARY_MK="`dirname $1`/single-binary.mk"
>> +GEN_LISTS_OF_PROGRAMS="`dirname $0`/gen-lists-of-programs.sh"
>
> "$0"
> It's worth checking with a space in your build path
Done.


>> +ALL_PROGRAMS=$($GEN_LISTS_OF_PROGRAMS --list-progs \
>> +    | grep -v -F -e coreutils -e libstdbuf.so \
>> +    | tr '.[' '_')
>
> safer to match up the number of chars in tr's FROM and TO.
> What's the '.' for anyway?
Done. The '.' wasn't needed =)


>> +  single_binary_libs=`
>> +    for p in $single_binary_progs; do
>> +      echo -n "src/libsinglebin_$p.a " | tr "\133" _
>
> `echo -n`, and to a lesser extent `tr` wouldn't
> be portable enough for configure. How about:
>
> -      echo -n "src/libsinglebin_$p.a " | tr "\133" _
> +      # Convert '[' to '_'
> +      test x"$p" = x'@<:@' && p='_'
> +      printf 'src/libsinglebin_%s.a ' "$p"
Done.
I changed this part a little bit to handle better the case when
$single_binary_progs doesn't include all the programs.


>> diff --git a/gnulib b/gnulib
>> index a10acfb..a008d62 160000
>> --- a/gnulib
>> +++ b/gnulib
>> @@ -1 +1 @@
>> -Subproject commit a10acfb1d2118f9a180181d3fed5399dbbe1df3c
>> +Subproject commit a008d625b7854d2c08c6606d90f6f2f48263f973
>
> I see you're running the latest gnulib.
> I'll disregard this bit
I missed it. Done.


>> diff --git a/src/coreutils.c b/src/coreutils.c
>> new file mode 100644
>
>> +int
>> +main (int argc, char **argv, char** envp)
>> +{
>> +  char *name = argv[0];
>> +  char *p;
>> +
>> +  /* Replace argv[0] by the tool name, removing the path from it. */
>> +  for (p = name; *p; ++p) {
>> +    if (*p == '/')
>> +      name = p + 1;
>> +  }
>
> Probably more general to use last_component() above.
Done.


>> +  argv[0] = name;
>
> Do we need to change argv[0] away from the full path?
> It would be best to not do that because for example if
> running `/opt/gnu/ls --help` we want that full path in the output.
Done.


>> +  fprintf(stderr, "%s: program not found.\n", argv[0]);
>
> error (EXIT_ENOENT, 0, "%s: program not found.\n", argv[0]);
Actually changed this to handle --version and --help in this case.
This makes "make check" happier.


>> --- a/src/getlimits.c
>> +++ b/src/getlimits.c
>> @@ -167,4 +167,6 @@ main (int argc, char **argv)
>>    print_float (FLT);
>>    print_float (DBL);
>>    print_float (LDBL);
>> +
>> +  exit (EXIT_SUCCESS);
>>  }
>
> A vestigial attempt at suppressing the no return warning?
> Anyway getlimits is not distributed so we can leave that out?

This is not the same as the __attributte__((noreturn)). This function
relies on the implicit "return 0" that C99 says the compiler adds for
main(). So I made that explicit, but it could be a "return 0;"
instead. I'll remove this because we are not using getlimits.


>> @@ -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?


>> +# 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. The
-Wno-suggest-attribute=noreturn was a simple solution. We have that
flag implicitly for all the main() functions, but not the others
(thanks to gcc or C standard for the consistency!).

I could move this flag from here to the src_libsinglebin_*_a_CPPFLAGS,
but it won't change much (just coreutils.c won't have that flags,
which is not very important).

> If that's no possible then I'd suggest adjusting configure.ac to
> checkout the new option a little earlier, and then:
>   if test "$gl_single_binary" = yes; then
>      gl_WARN_ADD([-Wno-suggest-attribute=noreturn])
>   fi
Added this to configure.ac (merged with the other «if test
"$gl_single_binary" = yes» block).



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


> 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/cp
bin/date
bin/dd
bin/dir
bin/install
bin/ls
bin/mv
bin/pr
bin/sort
bin/timeout
bin/touch
bin/vdir

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. 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 a lot!
deymo.