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

[Bernhard Voelker]

First of all, this should not be taken as a complete review.
It just reflects my first impression of --enable-single-binary.

On 06/19/2014 10:24 PM, Alex Deymo wrote:
> diff --git a/src/coreutils.c b/src/coreutils.c
> new file mode 100644
> index 0000000..70c2d21
> --- /dev/null
> +++ b/src/coreutils.c
> @@ -0,0 +1,138 @@
...
> +
> +  /* 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);

This should go to stderr. Why not error (...)?

> diff --git a/tests/misc/env.sh b/tests/misc/env.sh
> index c4b9737..f9400c7 100755
> --- a/tests/misc/env.sh
> +++ b/tests/misc/env.sh
> @@ -107,7 +107,8 @@ export PATH
>  # '/bin/sh -i', rather than '/bin/sh -- -i', which doesn't do what we want.
>  # Avoid the issue by using an executable rather than a script.
>  # Test -u, rather than -i, to minimize PATH problems.
> -ln -s "$abs_top_builddir/src/echo" ./-u || framework_failure_
> +echo "#!$abs_top_builddir/src/echo" > ./-u || framework_failure_
> +chmod +x ./-u || framework_failure_

This looks problematic - as the change contradicts the explicit comment
about avoiding a script.  Why should a symlink to src/echo (which in turn
is a symlink to coreutils) not work?


Further comments:

a) The syntax-check, check-very-expensive and the root tests pass ...
but some of them are skipped because the check on $built_programs fails:

  + require_built_ chroot
  + skip_=no
  + for i in '"$@"'
  + case " $built_programs " in
  + echo 'chroot: not built'
  chroot: not built
  + skip_=yes
  + test yes = yes
  + skip_ 'required program(s) not built'

The variable $build_programs now only contains "coreutils".

b) As space is the argument for using --enable-single-binary,
shouldn't a stripped install be the default there?

  $ ls -ldog coreutils
  -rwxr-xr-x 1 4816636 Jun 20 00:19 coreutils

c) Finally, it'd be good to have a test case for the new
'coreutils' binary. ;-)

Have a nice day,
Berny