| Message ID | CAKduhSuhU8V+5PxD04D4cRbk0v+1iDz_LijrpEoQEzmgno+OtQ@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
diff -r 5df2d60ca847 -r 0e65ef94066c scripts/crosstool-NG.sh.in --- a/scripts/crosstool-NG.sh.in Mon Nov 19 15:23:05 2012 -0800 +++ b/scripts/crosstool-NG.sh.in Mon Nov 19 16:13:49 2012 -0800 @@ -421,8 +421,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. 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}" @@ -474,17 +473,75 @@ *) ;; esac + # 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 + 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})" + ;; + *) + CT_Abort "Ran '${GCC} -v', but command failed with exit ${ret}" + ;; + esac + rm -f "${gccout}" + + 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}" + # 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 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:" + CT_DoLog DEBUG "$(cat ${gccout})" + CT_Abort "Static linking impossible on the host system '${CT_HOST}'; is libc.a installed?" 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}" fi # Help gcc
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. | > '------------------------------^-------^------------------^--------------------'