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

[Alex Deymo]

Thanks for review.

On Thu, Jun 19, 2014 at 3:58 PM, Mike Frysinger <address@hidden> wrote:
>> +install-exec-hook:
>> +     $(AM_V_at)for i in $(single_binary_progs); do                   \
>> +             rm -f $(DESTDIR)$(bindir)/$$i$(EXEEXT);                 \
>> +             $(LN_S) -s coreutils$(EXEEXT) 
>> $(DESTDIR)$(bindir)/$$i$(EXEEXT);\
>> +     done
>
> i think you need `|| exit $$?` after the rm & ln statements since this doesn't
> run in set -e mode
Done.


>> --- /dev/null
>> +++ b/src/coreutils-arch.c
>> @@ -0,0 +1,14 @@
>> +#include <config.h>
>> +#include "system.h"
>
> each of these stub files need a one line blurb/copyright/license comment
> block.  see kill.c as an example.
Done.


>> +/* Ensure that the main for uname is declared even if the tool is not being
>> + * build in this single-binary. */
>
> GNU style says to omit the * at the start of line, and two spaces after the
> period.

Have you tried running:
find src/ -name '*.c' | xargs grep -H '^ \* '  --color
;-)
I'll do my best to fix this on my patch.


>> --- /dev/null
>> +++ b/src/coreutils.c
>>
>> +/* coreutils.c aggregates the functionality of every other tool into a
> single
>> +   binary multiplexed by the value of argv[0]. This is enabled by passing
>
> two spaces after the period
Done.


>> +   By Alex Deymo <address@hidden> */
>
> two spaces at the end, and a period.
Done.


>> +/* Declare the main() function on each one of the selected tools. This name
>
> GNU style says to just use main -- omit the ().


>> +  if (STREQ (prog_name, "coreutils"))
>> +  {
>> +    prog_argv++;
>
> GNU style says to indent the braces with two spaces
>   if (...)
>     {
>       ...
>     }
Done.


>> +    prog_name = prog_argv[0]; /* Don't use last_component() in this case.
> */
>
> i think the preference is to not do inline comments, so move it to above
Done.


>> +  /* Only print the "program not found" message when no options have been
>> +   * passed to coreutils. */
>> +  if (optind == 1 && prog_name && !STREQ (prog_name, "coreutils"))
>> +    printf ("%s: program not found.\n", prog_name);
>
> shouldn't this write to stderr w/fprintf ?
>
> how about the message also taking the form:
>         coreutils: program not found: foo
How about using error() to print:
./coreutils: program foo: No such file or directory



>> --- a/src/local.mk
>> +++ b/src/local.mk
>>
>> +src/coreutils_symlinks:
>> +     $(AM_V_GEN)touch $@
>> +     $(AM_V_at)for i in $(single_binary_progs); do \
>> +             rm -f $(top_srcdir)/src/$$i$(EXEEXT); \
>> +             $(LN_S) -s coreutils$(EXEEXT) $(top_srcdir)/src/$$i$(EXEEXT); \
>> +     done
>> +
>> +clean-local:
>> +     rm -f src/coreutils_symlinks
>> +     $(AM_V_at)for i in $(single_binary_progs); do \
>> +             rm -f $(top_srcdir)/src/$$i$(EXEEXT); \
>> +     done
>
> same comment wrt `|| exit $$?`
Done.


I'll send an update tomorrow.