[1,of,1] refine static linking check to better guide user

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.

         -dp

On Mon, Nov 19, 2012 at 2:55 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Daniel, All,
>
> On Monday 19 November 2012 Daniel Price wrote:
>> Here it is again, as an attachment.  I cannot (apparently) persuade
>> gmail not to wrap lines.  Apologies.
>
> Do not use gmail's web client. It is uterly broken.
> Instead, use 'hg email' to send your patches. It can use your gmail
> account as smtp server, but ensurss the patches are not mangled.
>
> I'll look at your patch later. 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.  |
> '------------------------------^-------^------------------^--------------------'

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.

On Tue, Nov 20, 2012 at 3:15 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>> 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.

There was a spelling error (bally) in the original comment; I'll drop
it from my patch since it isn't crucial.

> 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.

I didn't love it either, but I thought that up-front sanity checking
was an improvement.

> 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"

Will fix.

> 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.

Sorry, I missed that logic, I didn't know it was there.  Will fix.

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

As for all the rest: You are right.  I'll rework to use the common
infrastructure.  I wasn't familiar enough with the codebase, clearly.
Thanks for the insightful review!

      -dp

--
Daniel.Price@gmail.com; Twitter: @danielbprice

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

Daniel, All,

On Wednesday 21 November 2012 Daniel Price wrote:
> On Tue, Nov 20, 2012 at 3:15 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >> -            # 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.
> 
> There was a spelling error (bally) in the original comment; I'll drop
> it from my patch since it isn't crucial.

Ah yes, 'badly'. OK for your fix, then.

> > 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.
> 
> I didn't love it either, but I thought that up-front sanity checking
> was an improvement.

Yes, but your next patch will be much cleaner! ;-)

> > Why can't we just simply run:
> >     CT_DoLog DEBUG "Testing blabla"
> >     CT_DoExecLog DEBUG "${CT_HOST}-gcc" -v
> 
> As for all the rest: You are right.  I'll rework to use the common
> infrastructure.  I wasn't familiar enough with the codebase, clearly.
> Thanks for the insightful review!

No problem. I really spent a lot of time on this, even testing some code
snippets in shell scripts, to come up with the alternate code construct,
to eventually realise that we already had all we needed with the error-
catching infra. Sigh..

/me goes to bed. ;-)

Thanks for working on this! :-)

Regards,
Yann E. MORIN.