Thanks for review. On Thu, Jun 19, 2014 at 3:58 PM, Mike Frysinger <[email protected]> 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 <[email protected]> */ > > 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.
