Re: [PATCH 1 of 1] refine static linking check to better guide user

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

Daniel, All,

On Tuesday 20 November 2012 Daniel Price wrote:
> Here's a revised version of the patch which also diagnoses missing
> libstdc++.a, which, at least on my Fedora system, was not installed by
> default.  This fails after several minutes of grinding away, so I
> think it's better to catch it early and help the user out.  Sorry for
> the noise.

Please, use 'hg email' to send your patches. See section titled "Using
Mercurial to hack crosstool-NG" in:
    docs/C - Misc. tutorials.txt

Adding patches as attachment makes the review harder, especially when
there are comments.

> diff --git a/scripts/crosstool-NG.sh.in b/scripts/crosstool-NG.sh.in
> --- a/scripts/crosstool-NG.sh.in
> +++ b/scripts/crosstool-NG.sh.in
> @@ -420,8 +420,7 @@
>                  where=$(CT_Which "${tool}")
>              fi
>  
> -            # Not all tools are available for all platforms, but some are really,
> -            # bally needed
> +            # Not all tools are available for all platforms, but some are required.

Gratuitous change with no real change in meaning.

>              if [ -n "${where}" ]; then
>                  CT_DoLog DEBUG "  '${!v}-${tool}' -> '${where}'"
>                  printf "#${BANG}${CT_CONFIG_SHELL}\nexec '${where}' \"\${@}\"\n" >"${CT_BUILDTOOLS_PREFIX_DIR}/bin/${!v}-${tool}"
> @@ -473,17 +472,75 @@
>          *)  ;;
>      esac

I really do not like the following. However, I don't see how we could make
it cleaner, given the overall design of this script. From the beginning,
I've been too lax when I wrote it.

> +    # Now that we've set up $PATH, sanity test that GCC is runnable so that
> +    # the user can troubleshoot problems if not.
> +    CT_DoLog DEBUG "Sanity testing gcc"
> +    gccout="${CT_BUILD_DIR}/.gcc-output"
> +    GCC=${CT_HOST}-gcc

Don't introduce that variable. It can be confusing to read "${GCC}", and
not be sure what gcc we're speakign about (host, build or target?). Just
use "{$CT_HOST}-gcc"

> +    ret=0
> +    ${GCC} -v > $gccout 2>&1 || ret=$?
> +    if [ $ret != 0 ]; then
> +        CT_DoLog DEBUG "Failed to invoke '${GCC} -v' (exited ${ret}): Output Follows:"
> +        CT_DoLog DEBUG "$(cat ${gccout})"
> +    fi
> +    case $ret in
> +     0)
> +             ;;
> +     126)
> +             CT_Abort "${GCC}: cannot execute; check permissions."
> +                ;;
> +     127)
> +             CT_Abort "${GCC}: not found in PATH; check for metacharacters or other problems in PATH (PATH=${PATH})"

Don't refer to metacharacter issues. Treat them in the existing test on
lines 57..70. Add a test for ':', and add other variables if needed.

> +                ;;
> +     *)
> +             CT_Abort "Ran '${GCC} -v', but command failed with exit ${ret}"
> +                ;;
> +    esac
> +    rm -f "${gccout}"

Variables must be quoted. Also, the following construct is more compact,
and also prints a debug message in the working case (it's can be usefull
to have debug messages in working cases, too):

    CT_DoLog DEBUG "Checking if we can run host gcc '${GCC}'"
    gccout="${CT_BUILD_DIR}/.gcc-output"
    if "${CT_HOST}-gcc" -v >"${gccout}" 2>&1; then
        CT_DoLog DEBUG "Yes, we can run host gcc '${GCC}'"
        rm -f "${gccout}"
    else
        case "${?}" in
            126) CT_Abort "${CT_HOST}-gcc: can not execute";;
            127) CT_Abort "${CT_HOST}-gcc: command not found";;
            *)   CT_DoLog ERROR "${CT_HOST}-gcc: exited with unknow errorcode '${?}', output was:"
                 CT_DoLog ERROR <"${gccout}"
                 rm -f "${gccout}"
                 CT_Abort "Bailing out, now."
                 ;;
        esac
    fi

And why do we even to run this at all, since we already have proper error
detection?

Why can't we just simply run:
    CT_DoLog DEBUG "Testing blabla"
    CT_DoExecLog DEBUG "${CT_HOST}-gcc" -v

Then, our error-catching infrastrucutre will catch any failure, print the
exit code, and the complete stdout+stderr will be in the build.log...
Sigh, I need more sleep, if I did not have seen that. :-(

> +    CT_DoLog DEBUG "Testing that gcc can compile a trivial program"
> +    tmp="${CT_BUILD_DIR}/.gcc-test"
> +    # Try a trivial program to ensure the compiler works.
> +    if ! "${CT_HOST}-gcc" -xc - -o "${tmp}"  > ${gccout} 2>&1 <<-_EOF_
> +                             int main() {return 0; }
> +                     _EOF_
> +    then
> +        CT_DoLog DEBUG "'${GCC}' failed (exited ${ret}): Output Follows:"
> +        CT_DoLog DEBUG "$(cat ${gccout})"
> +        CT_Abort "Couldn't compile a trivial program using ${CT_HOST}-gcc"
> +    fi
> +    rm -f "${tmp}" "${gccout}"
> +

Ditto, prepare the test-case file, and simply run through CT_DoExecLog:
    CT_DoLog DEBUG "Testing trivial blabla"
    printf "int main() { return 0; }\n" >"${gcctest}"
    CT_DoExecLog DEBUG "${CT_HOST}-gcc" -xc - -o "${tmp}"
    rm -f blablabla

>      # Now we know our host and where to find the host tools, we can check
>      # if static link was requested, but only if it was requested

Gah, non-sensical comment of mine. Blur de bla de blo...

>      if [ "${CT_WANTS_STATIC_LINK}" = "y" ]; then
>          tmp="${CT_BUILD_DIR}/.static-test"
> -        if ! "${CT_HOST}-gcc" -xc - -static -o "${tmp}" >/dev/null 2>&1 <<-_EOF_
> +
> +        CT_DoLog DEBUG "Testing that gcc can compile a trivial statically linked program"
> +        if ! "${CT_HOST}-gcc" -xc - -static -o "${tmp}" > ${gccout} 2>&1 <<-_EOF_
>                               int main() { return 0; }
>                       _EOF_
>          then
> -            CT_Abort "Static linking impossible on the host system '${CT_HOST}'"
> +            CT_DoLog DEBUG "'${GCC}' failed (exited ${ret}): Output Follows:"

${ret} will always be '0', because you do not set it.

> +            CT_DoLog DEBUG "$(cat ${gccout})"
> +            CT_Abort "Static linking impossible on the host system '${CT_HOST}'; is libc.a installed?"

Ditto, prepare test-case file, and run through CT_DoExecLog.

>          fi
> -        rm -f "${tmp}"
> +        rm -f "${tmp}" "${gccout}"
> +    fi
> +
> +    if [ "${CT_CC_STATIC_LIBSTDCXX}" = "y" ]; then
> +        tmp="${CT_BUILD_DIR}/.static-test"
> +
> +        CT_DoLog DEBUG "Testing that gcc can statically link libstdc++"
> +        if ! "${CT_HOST}-gcc" -xc - -static -lstdc++ -o "${tmp}" > ${gccout} 2>&1 <<-_EOF_
> +                             int main() { return 0; }
> +                     _EOF_
> +        then
> +            CT_DoLog DEBUG "'${GCC}' failed (exited ${ret}): Output Follows:"
> +            CT_DoLog DEBUG "$(cat ${gccout})"
> +            CT_Abort "Failed to statically link to libstdc++ on the host system '${CT_HOST}'; is libstdc++.a installed?"
> +        fi
> +        rm -f "${tmp}" "${gccout}"

Ditto again.

So, your patch should be as simple as:
  - add a test for ':' in paths, lines 57..70
  - run a few incatantions of host gcc with various flags and a few
    test-case files (possibly also printing a few user-friendly messages
    at DEBUG level).

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