diff mbox

[1/2] Remove some absolute paths

Message ID 1388421102-29198-1-git-send-email-bjorn.forsman@gmail.com
State Accepted
Commit f975a1a4738ace6344108a28a92767b95d6cef89
Headers show

Commit Message

Bjørn Forsman Dec. 30, 2013, 4:31 p.m. UTC
Buildroot fails to run on NixOS because it has no /bin/echo or
/bin/grep. Instead of relying on absolute paths, rely on tools to be
available in PATH. This should work for all systems.

Signed-off-by: Bjørn Forsman <bjorn.forsman@gmail.com>
---
 support/dependencies/dependencies.sh | 96 ++++++++++++++++++------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

Comments

Luca Ceresoli Dec. 30, 2013, 11:11 p.m. UTC | #1
Il 30/12/2013 17:31, Bjørn Forsman ha scritto:
> Buildroot fails to run on NixOS because it has no /bin/echo or
> /bin/grep. Instead of relying on absolute paths, rely on tools to be
> available in PATH. This should work for all systems.
>
> Signed-off-by: Bjørn Forsman <bjorn.forsman@gmail.com>
> ---
>   support/dependencies/dependencies.sh | 96 ++++++++++++++++++------------------
>   1 file changed, 48 insertions(+), 48 deletions(-)
>
> diff --git a/support/dependencies/dependencies.sh b/support/dependencies/dependencies.sh
> index 32b8fea..d1ce918 100755
> --- a/support/dependencies/dependencies.sh
> +++ b/support/dependencies/dependencies.sh
> @@ -7,20 +7,20 @@ export LC_ALL=C
>   # Verify that grep works
>   echo "WORKS" | grep "WORKS" >/dev/null 2>&1
>   if test $? != 0 ; then
> -	/bin/echo -e "\ngrep doesn't work\n"
> +	echo -e "\ngrep doesn't work\n"

I'm ok with the change, but while we're touching these basig commands,
why not replacing them with an ECHO and a GREP constant? Such as:
   ECHO := $(shell which grep)
or, more simply:
   ECHO := echo
and wherever it's used:
  -	/bin/echo -e "\ngrep doesn't work\n"
  +	$(ECHO) -e "\ngrep doesn't work\n"

This is what we do i.e. for sed. See package/Makefile.in.

This allows successive changes to the command definition without
the need for a massive search and replace.
Bjørn Forsman Dec. 31, 2013, 12:56 p.m. UTC | #2
On 31 December 2013 00:11, Luca Ceresoli <luca@lucaceresoli.net> wrote:
> Il 30/12/2013 17:31, Bjørn Forsman ha scritto:
>
>> Buildroot fails to run on NixOS because it has no /bin/echo or
>> /bin/grep. Instead of relying on absolute paths, rely on tools to be
>> available in PATH. This should work for all systems.
>>
>> Signed-off-by: Bjørn Forsman <bjorn.forsman@gmail.com>
>> ---
>>   support/dependencies/dependencies.sh | 96
>> ++++++++++++++++++------------------
>>   1 file changed, 48 insertions(+), 48 deletions(-)
>>
>> diff --git a/support/dependencies/dependencies.sh
>> b/support/dependencies/dependencies.sh
>> index 32b8fea..d1ce918 100755
>> --- a/support/dependencies/dependencies.sh
>> +++ b/support/dependencies/dependencies.sh
>> @@ -7,20 +7,20 @@ export LC_ALL=C
>>   # Verify that grep works
>>   echo "WORKS" | grep "WORKS" >/dev/null 2>&1
>>   if test $? != 0 ; then
>> -       /bin/echo -e "\ngrep doesn't work\n"
>> +       echo -e "\ngrep doesn't work\n"
>
>
> I'm ok with the change, but while we're touching these basig commands,
> why not replacing them with an ECHO and a GREP constant? Such as:
>   ECHO := $(shell which grep)
> or, more simply:
>   ECHO := echo
> and wherever it's used:
>
>  -      /bin/echo -e "\ngrep doesn't work\n"
>  +      $(ECHO) -e "\ngrep doesn't work\n"
>
> This is what we do i.e. for sed. See package/Makefile.in.
>
> This allows successive changes to the command definition without
> the need for a massive search and replace.

I took a quick look in support/dependencies/* and there are
interactions between several makefiles and shell scripts. Should all
deps be defined in variables in support/dependencies/dependencies.mk
and then only be *checked* in support/dependencies/dependencies.sh? Or
should maybe everything, definitions and checking, be moved to
makefiles?

Hm, package/Makefile.in also defines some "dependency" variables:
INSTALL, FLEX, BISON, SED. Looks like dependency handling/checking in
Buildroot could use some refactoring.

"But why not just define local variables in
support/dependencies/dependencies.sh?" My thinking is that as long as
we're

- not using full paths
- not writing setuid tools
- not selecting between similarly named commands, but built for host or target

there is very little benefit from using e.g. ${GREP} instead of grep.
Except that it's more difficult to type :-)

I see that SED is defined as "/path/to/sed -i -e". So mixing
/path/to/tool and tool_options is a way to use such variables. But I
don't think that's a good idea. I'd rather have separate variables,
like SED and SED_OPTS, so that there are no surprises: "oh, SED
changed the source file even though I didn't tell it to". Maybe SED
should be renamed to SED_INLINE? Or SED_WITH_OPTS?

I guess one benefit from using variables for commands is that it makes
the dependencies obvious. As in "if there is no variable defined for
'tool', don't use it", or "if there is no variable defined for 'tool',
define it in the central location and then use it". But that is
difficult to enforce. Just search for 'sed' in buildroot sources and
you'll see :-)

To sum it up, I have mixed feelings about using variables for command
names (that are always in PATH) and I think it is a different issue
than this patch set is trying to solve.

Best regards,
Bjørn Forsman
Thomas De Schampheleire Jan. 1, 2014, 5:13 p.m. UTC | #3
On Tue, Dec 31, 2013 at 1:56 PM, Bjørn Forsman <bjorn.forsman@gmail.com> wrote:
[..]
>
> Hm, package/Makefile.in also defines some "dependency" variables:
> INSTALL, FLEX, BISON, SED. Looks like dependency handling/checking in
> Buildroot could use some refactoring.
>
> "But why not just define local variables in
> support/dependencies/dependencies.sh?" My thinking is that as long as
> we're
>
> - not using full paths
> - not writing setuid tools
> - not selecting between similarly named commands, but built for host or target
>
> there is very little benefit from using e.g. ${GREP} instead of grep.
> Except that it's more difficult to type :-)
>
> I see that SED is defined as "/path/to/sed -i -e". So mixing
> /path/to/tool and tool_options is a way to use such variables. But I
> don't think that's a good idea. I'd rather have separate variables,
> like SED and SED_OPTS, so that there are no surprises: "oh, SED
> changed the source file even though I didn't tell it to". Maybe SED
> should be renamed to SED_INLINE? Or SED_WITH_OPTS?
>
> I guess one benefit from using variables for commands is that it makes
> the dependencies obvious. As in "if there is no variable defined for
> 'tool', don't use it", or "if there is no variable defined for 'tool',
> define it in the central location and then use it". But that is
> difficult to enforce. Just search for 'sed' in buildroot sources and
> you'll see :-)
>
> To sum it up, I have mixed feelings about using variables for command
> names (that are always in PATH) and I think it is a different issue
> than this patch set is trying to solve.


There is at least one reason why using variables to refer to tools in
makefiles makes sense: in make, any variable can be overruled on the
command-line. So you can call 'make SED=/my/own/sed' without any
special rules in the makefile itself. This is not possible if you
directly use 'sed' (or whatever tool).

Best regards,
Thomas
Bjørn Forsman Jan. 1, 2014, 8:24 p.m. UTC | #4
Hi Thomas,

On 1 January 2014 18:13, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
[...]
> There is at least one reason why using variables to refer to tools in
> makefiles makes sense: in make, any variable can be overruled on the
> command-line. So you can call 'make SED=/my/own/sed' without any
> special rules in the makefile itself. This is not possible if you
> directly use 'sed' (or whatever tool).

Good point.

I don't know if your remark was specific to my patch or just in
general, but I'd like to point out this patch changes a shell script,
not a makefile. Although a bit more troublesome than your make
example, this injects custom 'sed' in any language:
PATH=/my/sed/bin:$PATH some_tool.

I consider this patch a minor "fix". Adding variables for commands
(everywhere) in Buildroot is a different thing that should be in a
different patch set.

Please let me know if there is anything in *this* patch that I need to
change to get it merged.

Best regards,
Bjørn Forsman
Bjørn Forsman Jan. 18, 2014, 4 p.m. UTC | #5
On 30 December 2013 17:31, Bjørn Forsman <bjorn.forsman@gmail.com> wrote:
> Buildroot fails to run on NixOS because it has no /bin/echo or
> /bin/grep. Instead of relying on absolute paths, rely on tools to be
> available in PATH. This should work for all systems.

Ping?

When this is applied, make sure to also apply "[PATCH 2/2] Prefer
'printf' over 'echo -e' (for portability)".

Best regards,
Bjørn Forsman
Thomas De Schampheleire Jan. 21, 2014, 8:04 a.m. UTC | #6
On Mon, Dec 30, 2013 at 5:31 PM, Bjørn Forsman <bjorn.forsman@gmail.com> wrote:
> Buildroot fails to run on NixOS because it has no /bin/echo or
> /bin/grep. Instead of relying on absolute paths, rely on tools to be
> available in PATH. This should work for all systems.
>
> Signed-off-by: Bjørn Forsman <bjorn.forsman@gmail.com>
> ---
>  support/dependencies/dependencies.sh | 96 ++++++++++++++++++------------------
>  1 file changed, 48 insertions(+), 48 deletions(-)
>
> diff --git a/support/dependencies/dependencies.sh b/support/dependencies/dependencies.sh
> index 32b8fea..d1ce918 100755
> --- a/support/dependencies/dependencies.sh
> +++ b/support/dependencies/dependencies.sh
> @@ -7,20 +7,20 @@ export LC_ALL=C
>  # Verify that grep works
>  echo "WORKS" | grep "WORKS" >/dev/null 2>&1
>  if test $? != 0 ; then
> -       /bin/echo -e "\ngrep doesn't work\n"
> +       echo -e "\ngrep doesn't work\n"
>         exit 1
>  fi
>
>  # sanity check for CWD in LD_LIBRARY_PATH
>  # try not to rely on egrep..
>  if test -n "$LD_LIBRARY_PATH" ; then
> -       /bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep ':\.:' >/dev/null 2>&1 ||
> -       /bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
> -       /bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
> -       /bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
> +       echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep ':\.:' >/dev/null 2>&1 ||
> +       echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
> +       echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
> +       echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
>         if test $? = 0; then
> -               /bin/echo -e "\nYou seem to have the current working directory in your"
> -               /bin/echo -e "LD_LIBRARY_PATH environment variable. This doesn't work.\n"
> +               echo -e "\nYou seem to have the current working directory in your"
> +               echo -e "LD_LIBRARY_PATH environment variable. This doesn't work.\n"
>                 exit 1;
>         fi
>  fi;
> @@ -29,51 +29,51 @@ fi;
>  # in the PATH makes the toolchain build process break.
>  # try not to rely on egrep..
>  if test -n "$PATH" ; then
> -       /bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep ':\.:' >/dev/null 2>&1 ||
> -       /bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
> -       /bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
> -       /bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
> +       echo TRiGGER_start"$PATH"TRiGGER_end | grep ':\.:' >/dev/null 2>&1 ||
> +       echo TRiGGER_start"$PATH"TRiGGER_end | grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
> +       echo TRiGGER_start"$PATH"TRiGGER_end | grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
> +       echo TRiGGER_start"$PATH"TRiGGER_end | grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
>         if test $? = 0; then
> -               /bin/echo -e "\nYou seem to have the current working directory in your"
> -               /bin/echo -e "PATH environment variable. This doesn't work.\n"
> +               echo -e "\nYou seem to have the current working directory in your"
> +               echo -e "PATH environment variable. This doesn't work.\n"
>                 exit 1;
>         fi
>  fi;
>
>  if test -n "$PERL_MM_OPT" ; then
> -    /bin/echo -e "\nYou have PERL_MM_OPT defined because Perl local::lib"
> -    /bin/echo -e "is installed on your system. Please unset this variable"
> -    /bin/echo -e "before starting Buildroot, otherwise the compilation of"
> -    /bin/echo -e "Perl related packages will fail"
> +    echo -e "\nYou have PERL_MM_OPT defined because Perl local::lib"
> +    echo -e "is installed on your system. Please unset this variable"
> +    echo -e "before starting Buildroot, otherwise the compilation of"
> +    echo -e "Perl related packages will fail"
>      exit 1
>  fi
>
>  # Verify that which is installed
>  if ! which which > /dev/null ; then
> -       /bin/echo -e "\nYou must install 'which' on your build machine\n";
> +       echo -e "\nYou must install 'which' on your build machine\n";
>         exit 1;
>  fi;
>
>  if ! which sed > /dev/null ; then
> -       /bin/echo -e "\nYou must install 'sed' on your build machine\n"
> +       echo -e "\nYou must install 'sed' on your build machine\n"
>         exit 1
>  fi
>
>  # Check make
>  MAKE=$(which make 2> /dev/null)
>  if [ -z "$MAKE" ] ; then
> -       /bin/echo -e "\nYou must install 'make' on your build machine\n";
> +       echo -e "\nYou must install 'make' on your build machine\n";
>         exit 1;
>  fi;
>  MAKE_VERSION=$($MAKE --version 2>&1 | sed -e 's/^.* \([0-9\.]\)/\1/g' -e 's/[-\ ].*//g' -e '1q')
>  if [ -z "$MAKE_VERSION" ] ; then
> -       /bin/echo -e "\nYou must install 'make' on your build machine\n";
> +       echo -e "\nYou must install 'make' on your build machine\n";
>         exit 1;
>  fi;
>  MAKE_MAJOR=$(echo $MAKE_VERSION | sed -e "s/\..*//g")
>  MAKE_MINOR=$(echo $MAKE_VERSION | sed -e "s/^$MAKE_MAJOR\.//g" -e "s/\..*//g" -e "s/[a-zA-Z].*//g")
>  if [ $MAKE_MAJOR -lt 3 ] || [ $MAKE_MAJOR -eq 3 -a $MAKE_MINOR -lt 81 ] ; then
> -       /bin/echo -e "\nYou have make '$MAKE_VERSION' installed.  GNU make >=3.81 is required\n"
> +       echo -e "\nYou have make '$MAKE_VERSION' installed.  GNU make >=3.81 is required\n"
>         exit 1;
>  fi;
>
> @@ -83,14 +83,14 @@ if [ -z "$COMPILER" ] ; then
>         COMPILER=$(which cc 2> /dev/null)
>  fi;
>  if [ -z "$COMPILER" ] ; then
> -       /bin/echo -e "\nYou must install 'gcc' on your build machine\n";
> +       echo -e "\nYou must install 'gcc' on your build machine\n";
>         exit 1;
>  fi;
>
>  COMPILER_VERSION=$($COMPILER -v 2>&1 | sed -n '/^gcc version/p' |
>         sed -e 's/^gcc version \([0-9\.]\)/\1/g' -e 's/[-\ ].*//g' -e '1q')
>  if [ -z "$COMPILER_VERSION" ] ; then
> -       /bin/echo -e "\nYou must install 'gcc' on your build machine\n";
> +       echo -e "\nYou must install 'gcc' on your build machine\n";
>         exit 1;
>  fi;
>  COMPILER_MAJOR=$(echo $COMPILER_VERSION | sed -e "s/\..*//g")
> @@ -106,27 +106,27 @@ if [ -z "$CXXCOMPILER" ] ; then
>         CXXCOMPILER=$(which c++ 2> /dev/null)
>  fi
>  if [ -z "$CXXCOMPILER" ] ; then
> -       /bin/echo -e "\nYou may have to install 'g++' on your build machine\n"
> +       echo -e "\nYou may have to install 'g++' on your build machine\n"
>         #exit 1
>  fi
>  if [ ! -z "$CXXCOMPILER" ] ; then
>         CXXCOMPILER_VERSION=$($CXXCOMPILER -v 2>&1 | sed -n '/^gcc version/p' |
>                 sed -e 's/^gcc version \([0-9\.]\)/\1/g' -e 's/[-\ ].*//g' -e '1q')
>         if [ -z "$CXXCOMPILER_VERSION" ] ; then
> -               /bin/echo -e "\nYou may have to install 'g++' on your build machine\n"
> +               echo -e "\nYou may have to install 'g++' on your build machine\n"
>         fi
>
>         CXXCOMPILER_MAJOR=$(echo $CXXCOMPILER_VERSION | sed -e "s/\..*//g")
>         CXXCOMPILER_MINOR=$(echo $CXXCOMPILER_VERSION | sed -e "s/^$CXXCOMPILER_MAJOR\.//g" -e "s/\..*//g")
>         if [ $CXXCOMPILER_MAJOR -lt 3 -o $CXXCOMPILER_MAJOR -eq 2 -a $CXXCOMPILER_MINOR -lt 95 ] ; then
> -               /bin/echo -e "\nYou have g++ '$CXXCOMPILER_VERSION' installed.  g++ >= 2.95 is required\n"
> +               echo -e "\nYou have g++ '$CXXCOMPILER_VERSION' installed.  g++ >= 2.95 is required\n"
>                 exit 1
>         fi
>  fi
>
>  # Check bash
>  if ! $SHELL --version 2>&1 | grep -q '^GNU bash'; then
> -       /bin/echo -e "\nYou must install 'bash' on your build machine\n";
> +       echo -e "\nYou must install 'bash' on your build machine\n";
>         exit 1;
>  fi;
>
> @@ -134,16 +134,16 @@ fi;
>  missing_progs="no"
>  for prog in patch perl tar wget cpio python unzip rsync bc ${DL_TOOLS} ; do
>      if ! which $prog > /dev/null ; then
> -       /bin/echo -e "You must install '$prog' on your build machine";
> +       echo -e "You must install '$prog' on your build machine";
>         missing_progs="yes"
>         if test $prog = "svn" ; then
> -           /bin/echo -e "  svn is usually part of the subversion package in your distribution"
> +           echo -e "  svn is usually part of the subversion package in your distribution"
>         elif test $prog = "hg" ; then
> -            /bin/echo -e "  hg is usually part of the mercurial package in your distribution"
> +            echo -e "  hg is usually part of the mercurial package in your distribution"
>         elif test $prog = "zcat" ; then
> -            /bin/echo -e "  zcat is usually part of the gzip package in your distribution"
> +            echo -e "  zcat is usually part of the gzip package in your distribution"
>         elif test $prog = "bzcat" ; then
> -            /bin/echo -e "  bzcat is usually part of the bzip2 package in your distribution"
> +            echo -e "  bzcat is usually part of the bzip2 package in your distribution"
>         fi
>      fi
>  done
> @@ -155,11 +155,11 @@ fi
>  if grep ^BR2_TOOLCHAIN_BUILDROOT=y $BUILDROOT_CONFIG > /dev/null && \
>     grep ^BR2_ENABLE_LOCALE=y       $BUILDROOT_CONFIG > /dev/null ; then
>     if ! which locale > /dev/null ; then
> -       /bin/echo -e "\nYou need locale support on your build machine to build a toolchain supporting locales\n"
> +       echo -e "\nYou need locale support on your build machine to build a toolchain supporting locales\n"
>         exit 1 ;
>     fi
>     if ! locale -a | grep -q -i utf8$ ; then
> -       /bin/echo -e "\nYou need at least one UTF8 locale to build a toolchain supporting locales\n"
> +       echo -e "\nYou need at least one UTF8 locale to build a toolchain supporting locales\n"
>         exit 1 ;
>     fi
>  fi
> @@ -167,7 +167,7 @@ fi
>  if grep -q ^BR2_PACKAGE_CLASSPATH=y $BUILDROOT_CONFIG ; then
>      for prog in javac jar; do
>         if ! which $prog > /dev/null ; then
> -           /bin/echo -e "\nYou must install '$prog' on your build machine\n" >&2
> +           echo -e "\nYou must install '$prog' on your build machine\n" >&2
>             exit 1
>         fi
>      done
> @@ -175,22 +175,22 @@ fi
>
>  if grep -q ^BR2_HOSTARCH_NEEDS_IA32_LIBS=y $BUILDROOT_CONFIG ; then
>      if test ! -f /lib/ld-linux.so.2 ; then
> -       /bin/echo -e "\nYour Buildroot configuration uses pre-built tools for the x86 architecture,"
> -       /bin/echo -e "but your build machine uses the x86-64 architecture without the 32 bits compatibility"
> -       /bin/echo -e "library."
> -       /bin/echo -e "If you're running a Debian/Ubuntu distribution, install the libc6:i386,"
> -       /bin/echo -e "libstdc++6:i386, and zlib1g:i386 packages."
> -       /bin/echo -e "For other distributions, refer to the documentation on how to install the 32 bits"
> -       /bin/echo -e "compatibility libraries."
> +       echo -e "\nYour Buildroot configuration uses pre-built tools for the x86 architecture,"
> +       echo -e "but your build machine uses the x86-64 architecture without the 32 bits compatibility"
> +       echo -e "library."
> +       echo -e "If you're running a Debian/Ubuntu distribution, install the libc6:i386,"
> +       echo -e "libstdc++6:i386, and zlib1g:i386 packages."
> +       echo -e "For other distributions, refer to the documentation on how to install the 32 bits"
> +       echo -e "compatibility libraries."
>         exit 1
>      fi
>  fi
>
>  if grep -q ^BR2_HOSTARCH_NEEDS_IA32_COMPILER=y $BUILDROOT_CONFIG ; then
>      if ! echo "int main(void) {}" | gcc -m32 -x c - ; then
> -       /bin/echo -e "\nYour Buildroot configuration needs a compiler capable of building 32 bits binaries."
> -       /bin/echo -e "If you're running a Debian/Ubuntu distribution, install the gcc-multilib package."
> -       /bin/echo -e "For other distributions, refer to their documentation."
> +       echo -e "\nYour Buildroot configuration needs a compiler capable of building 32 bits binaries."
> +       echo -e "If you're running a Debian/Ubuntu distribution, install the gcc-multilib package."
> +       echo -e "For other distributions, refer to their documentation."
>         exit 1
>      fi
>  fi
> @@ -198,7 +198,7 @@ fi
>  # Check that the Perl installation is complete enough to build
>  # host-autoconf.
>  if ! perl  -e "require Data::Dumper" > /dev/null 2>&1 ; then
> -    /bin/echo -e "Your Perl installation is not complete enough, at least Data::Dumper is missing."
> -    /bin/echo -e "On Debian/Ubuntu distributions, install the 'perl' package."
> +    echo -e "Your Perl installation is not complete enough, at least Data::Dumper is missing."
> +    echo -e "On Debian/Ubuntu distributions, install the 'perl' package."
>      exit 1
>  fi


Acked-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>

Although the discussion raised some interesting points, I believe this
patch is ok to accept as-is.

Best regards,
Thomas
Peter Korsgaard Jan. 21, 2014, 8:43 a.m. UTC | #7
>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

Hi,

 > Acked-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>

 > Although the discussion raised some interesting points, I believe this
 > patch is ok to accept as-is.

True. The explicit /bin/echo was afaik to make sure we end up with an
echo variant (most likely coreutils) that understands -e, but if we're
changing it to printf anyway.

Committed, thanks.
Bjørn Forsman Jan. 21, 2014, 6:14 p.m. UTC | #8
On 21 January 2014 09:43, Peter Korsgaard <jacmet@uclibc.org> wrote:
>>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:
>
> Hi,
>
>  > Acked-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>
>
>  > Although the discussion raised some interesting points, I believe this
>  > patch is ok to accept as-is.
>
> True. The explicit /bin/echo was afaik to make sure we end up with an
> echo variant (most likely coreutils) that understands -e, but if we're
> changing it to printf anyway.
>
> Committed, thanks.

Oops, this patch wasn't supposted to be commited all by itself and not
followed by the "[PATCH 2/2] Prefer
'printf' over 'echo -e' (for portability)" patch. With the current use
of "echo -e" there will be breakage if /bin/sh is dash.

Peter, could you apply the 2nd patch also and then I can submit a
separate patch afterwards to stop the echo/printf mix?

Best regards,
Bjørn Forsman
Peter Korsgaard Jan. 21, 2014, 8:53 p.m. UTC | #9
>>>>> "Bjørn" == Bjørn Forsman <bjorn.forsman@gmail.com> writes:

Hi,

 > Oops, this patch wasn't supposted to be commited all by itself and not
 > followed by the "[PATCH 2/2] Prefer
 > 'printf' over 'echo -e' (for portability)" patch. With the current use
 > of "echo -e" there will be breakage if /bin/sh is dash.

 > Peter, could you apply the 2nd patch also and then I can submit a
 > separate patch afterwards to stop the echo/printf mix?

Ok, committed - Thanks.
diff mbox

Patch

diff --git a/support/dependencies/dependencies.sh b/support/dependencies/dependencies.sh
index 32b8fea..d1ce918 100755
--- a/support/dependencies/dependencies.sh
+++ b/support/dependencies/dependencies.sh
@@ -7,20 +7,20 @@  export LC_ALL=C
 # Verify that grep works
 echo "WORKS" | grep "WORKS" >/dev/null 2>&1
 if test $? != 0 ; then
-	/bin/echo -e "\ngrep doesn't work\n"
+	echo -e "\ngrep doesn't work\n"
 	exit 1
 fi
 
 # sanity check for CWD in LD_LIBRARY_PATH
 # try not to rely on egrep..
 if test -n "$LD_LIBRARY_PATH" ; then
-	/bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep ':\.:' >/dev/null 2>&1 ||
-	/bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
-	/bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
-	/bin/echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
+	echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep ':\.:' >/dev/null 2>&1 ||
+	echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
+	echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
+	echo TRiGGER_start"$LD_LIBRARY_PATH"TRiGGER_end | grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
 	if test $? = 0; then
-		/bin/echo -e "\nYou seem to have the current working directory in your"
-		/bin/echo -e "LD_LIBRARY_PATH environment variable. This doesn't work.\n"
+		echo -e "\nYou seem to have the current working directory in your"
+		echo -e "LD_LIBRARY_PATH environment variable. This doesn't work.\n"
 		exit 1;
 	fi
 fi;
@@ -29,51 +29,51 @@  fi;
 # in the PATH makes the toolchain build process break.
 # try not to rely on egrep..
 if test -n "$PATH" ; then
-	/bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep ':\.:' >/dev/null 2>&1 ||
-	/bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
-	/bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
-	/bin/echo TRiGGER_start"$PATH"TRiGGER_end | /bin/grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
+	echo TRiGGER_start"$PATH"TRiGGER_end | grep ':\.:' >/dev/null 2>&1 ||
+	echo TRiGGER_start"$PATH"TRiGGER_end | grep 'TRiGGER_start\.:' >/dev/null 2>&1 ||
+	echo TRiGGER_start"$PATH"TRiGGER_end | grep ':\.TRiGGER_end' >/dev/null 2>&1 ||
+	echo TRiGGER_start"$PATH"TRiGGER_end | grep 'TRiGGER_start\.TRiGGER_end' >/dev/null 2>&1
 	if test $? = 0; then
-		/bin/echo -e "\nYou seem to have the current working directory in your"
-		/bin/echo -e "PATH environment variable. This doesn't work.\n"
+		echo -e "\nYou seem to have the current working directory in your"
+		echo -e "PATH environment variable. This doesn't work.\n"
 		exit 1;
 	fi
 fi;
 
 if test -n "$PERL_MM_OPT" ; then
-    /bin/echo -e "\nYou have PERL_MM_OPT defined because Perl local::lib"
-    /bin/echo -e "is installed on your system. Please unset this variable"
-    /bin/echo -e "before starting Buildroot, otherwise the compilation of"
-    /bin/echo -e "Perl related packages will fail"
+    echo -e "\nYou have PERL_MM_OPT defined because Perl local::lib"
+    echo -e "is installed on your system. Please unset this variable"
+    echo -e "before starting Buildroot, otherwise the compilation of"
+    echo -e "Perl related packages will fail"
     exit 1
 fi
 
 # Verify that which is installed
 if ! which which > /dev/null ; then
-	/bin/echo -e "\nYou must install 'which' on your build machine\n";
+	echo -e "\nYou must install 'which' on your build machine\n";
 	exit 1;
 fi;
 
 if ! which sed > /dev/null ; then
-	/bin/echo -e "\nYou must install 'sed' on your build machine\n"
+	echo -e "\nYou must install 'sed' on your build machine\n"
 	exit 1
 fi
 
 # Check make
 MAKE=$(which make 2> /dev/null)
 if [ -z "$MAKE" ] ; then
-	/bin/echo -e "\nYou must install 'make' on your build machine\n";
+	echo -e "\nYou must install 'make' on your build machine\n";
 	exit 1;
 fi;
 MAKE_VERSION=$($MAKE --version 2>&1 | sed -e 's/^.* \([0-9\.]\)/\1/g' -e 's/[-\ ].*//g' -e '1q')
 if [ -z "$MAKE_VERSION" ] ; then
-	/bin/echo -e "\nYou must install 'make' on your build machine\n";
+	echo -e "\nYou must install 'make' on your build machine\n";
 	exit 1;
 fi;
 MAKE_MAJOR=$(echo $MAKE_VERSION | sed -e "s/\..*//g")
 MAKE_MINOR=$(echo $MAKE_VERSION | sed -e "s/^$MAKE_MAJOR\.//g" -e "s/\..*//g" -e "s/[a-zA-Z].*//g")
 if [ $MAKE_MAJOR -lt 3 ] || [ $MAKE_MAJOR -eq 3 -a $MAKE_MINOR -lt 81 ] ; then
-	/bin/echo -e "\nYou have make '$MAKE_VERSION' installed.  GNU make >=3.81 is required\n"
+	echo -e "\nYou have make '$MAKE_VERSION' installed.  GNU make >=3.81 is required\n"
 	exit 1;
 fi;
 
@@ -83,14 +83,14 @@  if [ -z "$COMPILER" ] ; then
 	COMPILER=$(which cc 2> /dev/null)
 fi;
 if [ -z "$COMPILER" ] ; then
-	/bin/echo -e "\nYou must install 'gcc' on your build machine\n";
+	echo -e "\nYou must install 'gcc' on your build machine\n";
 	exit 1;
 fi;
 
 COMPILER_VERSION=$($COMPILER -v 2>&1 | sed -n '/^gcc version/p' |
 	sed -e 's/^gcc version \([0-9\.]\)/\1/g' -e 's/[-\ ].*//g' -e '1q')
 if [ -z "$COMPILER_VERSION" ] ; then
-	/bin/echo -e "\nYou must install 'gcc' on your build machine\n";
+	echo -e "\nYou must install 'gcc' on your build machine\n";
 	exit 1;
 fi;
 COMPILER_MAJOR=$(echo $COMPILER_VERSION | sed -e "s/\..*//g")
@@ -106,27 +106,27 @@  if [ -z "$CXXCOMPILER" ] ; then
 	CXXCOMPILER=$(which c++ 2> /dev/null)
 fi
 if [ -z "$CXXCOMPILER" ] ; then
-	/bin/echo -e "\nYou may have to install 'g++' on your build machine\n"
+	echo -e "\nYou may have to install 'g++' on your build machine\n"
 	#exit 1
 fi
 if [ ! -z "$CXXCOMPILER" ] ; then
 	CXXCOMPILER_VERSION=$($CXXCOMPILER -v 2>&1 | sed -n '/^gcc version/p' |
 		sed -e 's/^gcc version \([0-9\.]\)/\1/g' -e 's/[-\ ].*//g' -e '1q')
 	if [ -z "$CXXCOMPILER_VERSION" ] ; then
-		/bin/echo -e "\nYou may have to install 'g++' on your build machine\n"
+		echo -e "\nYou may have to install 'g++' on your build machine\n"
 	fi
 
 	CXXCOMPILER_MAJOR=$(echo $CXXCOMPILER_VERSION | sed -e "s/\..*//g")
 	CXXCOMPILER_MINOR=$(echo $CXXCOMPILER_VERSION | sed -e "s/^$CXXCOMPILER_MAJOR\.//g" -e "s/\..*//g")
 	if [ $CXXCOMPILER_MAJOR -lt 3 -o $CXXCOMPILER_MAJOR -eq 2 -a $CXXCOMPILER_MINOR -lt 95 ] ; then
-		/bin/echo -e "\nYou have g++ '$CXXCOMPILER_VERSION' installed.  g++ >= 2.95 is required\n"
+		echo -e "\nYou have g++ '$CXXCOMPILER_VERSION' installed.  g++ >= 2.95 is required\n"
 		exit 1
 	fi
 fi
 
 # Check bash
 if ! $SHELL --version 2>&1 | grep -q '^GNU bash'; then
-	/bin/echo -e "\nYou must install 'bash' on your build machine\n";
+	echo -e "\nYou must install 'bash' on your build machine\n";
 	exit 1;
 fi;
 
@@ -134,16 +134,16 @@  fi;
 missing_progs="no"
 for prog in patch perl tar wget cpio python unzip rsync bc ${DL_TOOLS} ; do
     if ! which $prog > /dev/null ; then
-	/bin/echo -e "You must install '$prog' on your build machine";
+	echo -e "You must install '$prog' on your build machine";
 	missing_progs="yes"
 	if test $prog = "svn" ; then
-	    /bin/echo -e "  svn is usually part of the subversion package in your distribution"
+	    echo -e "  svn is usually part of the subversion package in your distribution"
 	elif test $prog = "hg" ; then
-            /bin/echo -e "  hg is usually part of the mercurial package in your distribution"
+            echo -e "  hg is usually part of the mercurial package in your distribution"
 	elif test $prog = "zcat" ; then
-            /bin/echo -e "  zcat is usually part of the gzip package in your distribution"
+            echo -e "  zcat is usually part of the gzip package in your distribution"
 	elif test $prog = "bzcat" ; then
-            /bin/echo -e "  bzcat is usually part of the bzip2 package in your distribution"
+            echo -e "  bzcat is usually part of the bzip2 package in your distribution"
 	fi
     fi
 done
@@ -155,11 +155,11 @@  fi
 if grep ^BR2_TOOLCHAIN_BUILDROOT=y $BUILDROOT_CONFIG > /dev/null && \
    grep ^BR2_ENABLE_LOCALE=y       $BUILDROOT_CONFIG > /dev/null ; then
    if ! which locale > /dev/null ; then
-       /bin/echo -e "\nYou need locale support on your build machine to build a toolchain supporting locales\n"
+       echo -e "\nYou need locale support on your build machine to build a toolchain supporting locales\n"
        exit 1 ;
    fi
    if ! locale -a | grep -q -i utf8$ ; then
-       /bin/echo -e "\nYou need at least one UTF8 locale to build a toolchain supporting locales\n"
+       echo -e "\nYou need at least one UTF8 locale to build a toolchain supporting locales\n"
        exit 1 ;
    fi
 fi
@@ -167,7 +167,7 @@  fi
 if grep -q ^BR2_PACKAGE_CLASSPATH=y $BUILDROOT_CONFIG ; then
     for prog in javac jar; do
 	if ! which $prog > /dev/null ; then
-	    /bin/echo -e "\nYou must install '$prog' on your build machine\n" >&2
+	    echo -e "\nYou must install '$prog' on your build machine\n" >&2
 	    exit 1
 	fi
     done
@@ -175,22 +175,22 @@  fi
 
 if grep -q ^BR2_HOSTARCH_NEEDS_IA32_LIBS=y $BUILDROOT_CONFIG ; then
     if test ! -f /lib/ld-linux.so.2 ; then
-	/bin/echo -e "\nYour Buildroot configuration uses pre-built tools for the x86 architecture,"
-	/bin/echo -e "but your build machine uses the x86-64 architecture without the 32 bits compatibility"
-	/bin/echo -e "library."
-	/bin/echo -e "If you're running a Debian/Ubuntu distribution, install the libc6:i386,"
-	/bin/echo -e "libstdc++6:i386, and zlib1g:i386 packages."
-	/bin/echo -e "For other distributions, refer to the documentation on how to install the 32 bits"
-	/bin/echo -e "compatibility libraries."
+	echo -e "\nYour Buildroot configuration uses pre-built tools for the x86 architecture,"
+	echo -e "but your build machine uses the x86-64 architecture without the 32 bits compatibility"
+	echo -e "library."
+	echo -e "If you're running a Debian/Ubuntu distribution, install the libc6:i386,"
+	echo -e "libstdc++6:i386, and zlib1g:i386 packages."
+	echo -e "For other distributions, refer to the documentation on how to install the 32 bits"
+	echo -e "compatibility libraries."
 	exit 1
     fi
 fi
 
 if grep -q ^BR2_HOSTARCH_NEEDS_IA32_COMPILER=y $BUILDROOT_CONFIG ; then
     if ! echo "int main(void) {}" | gcc -m32 -x c - ; then
-	/bin/echo -e "\nYour Buildroot configuration needs a compiler capable of building 32 bits binaries."
-	/bin/echo -e "If you're running a Debian/Ubuntu distribution, install the gcc-multilib package."
-	/bin/echo -e "For other distributions, refer to their documentation."
+	echo -e "\nYour Buildroot configuration needs a compiler capable of building 32 bits binaries."
+	echo -e "If you're running a Debian/Ubuntu distribution, install the gcc-multilib package."
+	echo -e "For other distributions, refer to their documentation."
 	exit 1
     fi
 fi
@@ -198,7 +198,7 @@  fi
 # Check that the Perl installation is complete enough to build
 # host-autoconf.
 if ! perl  -e "require Data::Dumper" > /dev/null 2>&1 ; then
-    /bin/echo -e "Your Perl installation is not complete enough, at least Data::Dumper is missing."
-    /bin/echo -e "On Debian/Ubuntu distributions, install the 'perl' package."
+    echo -e "Your Perl installation is not complete enough, at least Data::Dumper is missing."
+    echo -e "On Debian/Ubuntu distributions, install the 'perl' package."
     exit 1
 fi