Re: Split patch for Mac OS/ was Re: Help with Building toolchain with crosstool-ng on Mac OS X 10.6.2 (Snow Leopard)

["Yann E. MORIN" <yann dot morin dot 1998 at anciens dot enib dot fr>]

Hello Titus, All!

(Sorry for the previous mail, I mistakenly hit Ctrl-Enter, which sent the
mail).

On Wednesday 03 February 2010 00:24:24 tvb377@gmx.de wrote:
[--SNIP--]

Here is a further review of your patchset :

--> wrapper-compile-commandline:

As previously stated, this breaks candaian-cross. Please fix.

--> dyld-library-path-wrapper.c:

Please run the following:   gcc -E -x c - -dM </dev/null
and see if there is a macros that specifies that this is Darwin (or
some such). This would be far better than relying on uname.

--> gnu-tools:

We already have some companion tools that should take care of those.
Unfortunately, inclusion of those was not complete. Indeed, we build them
as part os the standard build procedure, and that comes after ./configure
has a chance to find them (because they do not exist yet)... Sigh...

> diff --git a/configure b/configure
> --- a/configure
> +++ b/configure
[--SNIP--]
> -has_or_abort prog="awk gawk" ver='^GNU Awk' err="GNU 'awk' was not found"
> +has_or_abort prog="gawk awk" ver='^GNU Awk' err="GNU 'awk' was not found"

Huh? Why do you need to switch the check ?

[--SNIP--]
> -has_or_abort prog=stat ver='GNU coreutils'
> +has_or_abort prog=stat

One separate patch for that, please. I want to be able to revert just this
in case it breaks later, whithout reverting the whole patch.

--> readlink:

> diff --git a/configure b/configure
> --- a/configure
> +++ b/configure
> @@ -160,7 +160,7 @@
>                  printf "Checking for '${item}'... "
>                  where="$( gcc -print-file-name="${item}" )"
>                  if [ "${where}" != "${item}" ]; then
> -                    where="$( readlink -e "${where}" )"
> +                    where="$( readlink "${where}" )"
>                      status=yes
>                      break;
>                  fi

Separate patch, please.

> diff --git a/scripts/build/internals.sh b/scripts/build/internals.sh
> --- a/scripts/build/internals.sh
> +++ b/scripts/build/internals.sh
> @@ -42,6 +42,11 @@
>          CT_DoLog EXTRA "Installing toolchain wrappers"
>          CT_Pushd "${CT_PREFIX_DIR}/bin"
>
> +        if [ "$CT_SYS_OS" = "Darwin" -o "$CT_SYS_OS" = "FreeBSD" ] ; then
> +            # wrapper does not work (when using readlink -m)
> +            CT_DoLog EXTRA "Forcing usage of binary wrappers"

CT_DoLog WARN "...."

> +            CT_TOOLS_WRAPPER="exec"
> +        fi
>          # Install the wrapper
>          case "${CT_TOOLS_WRAPPER}" in
>              script)
> diff --git a/scripts/wrapper.in b/scripts/wrapper.in
> --- a/scripts/wrapper.in
> +++ b/scripts/wrapper.in
> @@ -1,5 +1,10 @@
>  #!/bin/sh
>
> +# this wrapper will not work under BSD systems or others
> +# not containig the GNU readlink.
> +# Under those, wrapper.c will forcibly be used
> +# regardless of the config file setting.
> +

Not really needed. Put that paragraph in the documentation, please.

--> samples.mk-sed: OK, will apply! :-)

--> sed-portable:

OK, extended regular expressions are not really needed in this case.
I will apply!

Note however that there are many other places where sed is called with -r.
Did you make sure those where not impacted also?

--> stat-portable:

> diff --git a/scripts/build/internals.sh b/scripts/build/internals.sh
> --- a/scripts/build/internals.sh
> +++ b/scripts/build/internals.sh
> @@ -72,7 +72,7 @@
>          # scripts, we don't know if they would in the end spawn a
> binary... # Just skip symlinks
>          for _t in "${CT_TARGET}-"*; do
> -            if [ "$( LANG=C stat -c '%F' "${_t}" )" != "symbolic link" ]; then
> +            if [ -z "`readlink ${_t}`" ]; then 

The rest of the code uses $(...) instead of `...` Although there is
nothing wrong with `...`, I'd like to keep the code homogeneous.

And separate patch for that, please, as it's not related to the rest
of the patch.

>                  CT_DoExecLog ALL mv "${_t}" ".${_t}"
>                  CT_DoExecLog ALL ln ".${CT_TARGET}-wrapper" "${_t}"
>              fi
> diff --git a/scripts/functions b/scripts/functions
> --- a/scripts/functions
> +++ b/scripts/functions
> @@ -249,7 +249,17 @@
>      local mode
>      for dir in "${@}"; do
>          [ -d "${dir}" ] || continue
> -        mode="$(stat -c '%a' "$(dirname "${dir}")")"
> +        case "$CT_SYS_OS" in
> +            GNU/Linux)
> +                mode="$(stat -c '%a' "$(dirname "${dir}")")"
> +                ;;
> +            Darwin|FreeBSD)
> +                mode="$(stat -f '%Lp' "$(dirname "${dir}")")"
> +                ;;
> +            *)
> +                CT_Abort "Unhandled host OS $CT_SYS_OS"
> +                ;;
> +        esac
>          CT_DoExecLog ALL chmod u+w "$(dirname "${dir}")"
>          CT_DoExecLog ALL chmod -R u+w "${dir}"
>          CT_DoExecLog ALL rm -rf "${dir}"

This is good. Separate patch, please.

> diff --git a/scripts/populate.in b/scripts/populate.in
> --- a/scripts/populate.in
> +++ b/scripts/populate.in
> @@ -64,6 +64,22 @@
>  _EOF_
>  }
>
> +# return the inode of a file in a portable way
> +CT_getinode() {
> +    case `uname -s` in

`...` --> $(...)

> +        *BSD|Darwin)
> +            stat -f '%i' $1

Breaks if $1 has a space; quote appropriately.

> +            ;;
> +        Linux)
> +            stat -c '%i' $1

Ditto

> +            ;;
> +        *)
> +            echo "OS unknown";

Redirect to stderr!

> +            exit 1
> +            ;;
> +    esac
> +}
> +
>  CT_ROOT_SRC_DIR=
>  CT_ROOT_DST_DIR=
>  CT_LIB_LIST=
> @@ -104,8 +120,8 @@
>      echo "$myname: '${CT_ROOT_DST_DIR}': already exists"
>      exit 1
>  fi
> -src_inode=$(stat -c '%i' "${CT_ROOT_SRC_DIR}/.")
> -dst_inode=$(stat -c '%i' "${CT_ROOT_DST_DIR}/." 2>/dev/null || true)
> +src_inode=$(CT_getinode "${CT_ROOT_SRC_DIR}/.")
> +dst_inode=$(CT_getinode "${CT_ROOT_DST_DIR}/." 2>/dev/null || true)

Oh well, stderr is consigned to oblivion here, which means you wont see
the "OS unknown" error message anyway...

>  if [ "${src_inode}" -eq "$((dst_inode+0))" ]; then
>      echo "$myname: source and destination are the same!"
>      exit 1

Separate patch, please.
Seems we can do way better here: move the '2>/dev/null || true' up into
the function.

--> use-configguess-on-bsd:

> diff --git a/scripts/crosstool-NG.sh.in b/scripts/crosstool-NG.sh.in
> -        "") CT_BUILD=$("${CT_BUILD_PREFIX}gcc${CT_BUILD_SUFFIX}" -dumpmachine);;
> +        "") if [ "$CT_SYS_OS" = "Darwin" -o "$CT_SYS_OS" = "FreeBSD" ] ; then
> +                CT_BUILD=$(CT_DoConfigGuess)
> +            else
> +                CT_BUILD=$("${CT_BUILD_PREFIX}gcc${CT_BUILD_SUFFIX}" -dumpmachine)
> +            fi
> +            ;;

case "${CT_SYS_OS}" in
    Darwin|FreeBSD)   CT_BUILD="$(....)";;
    *)                CT_BUILD="$(... -dumpmachine)";;
esac

Could you rework your patches, please? Thank you! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
`------------------------------^-------^------------------^--------------------'



--
For unsubscribe information see http://sourceware.org/lists.html#faq