[PATCH 1 of 1] cc/gcc: Split gcc configurations and functions from cc ones
Yann E. MORIN
yann.morin.1998@free.fr
Thu Dec 12 22:07:00 GMT 2013
Yann, All,
On 2013-12-11 21:41 +0100, Yann Diorcet spake thusly:
> # HG changeset patch
> # User Yann Diorcet <diorcet.yann@gmail.com>
> # Date 1386793717 -3600
> # Wed Dec 11 21:28:37 2013 +0100
> # Node ID cc545a5c18967b99bb58e57bef03471254eb9745
> # Parent 7e569a9cb5fd3ab591bb307328b947a5b7312cba
> cc/gcc: Split gcc configurations and functions from cc ones
>
> Signed-off-by: Yann Diorcet <diorcet.yann@gmail.com>
>
> diff -r 7e569a9cb5fd -r cc545a5c1896 config/cc.in
> --- a/config/cc.in Sat Nov 16 18:14:45 2013 +0100
> +++ b/config/cc.in Wed Dec 11 21:28:37 2013 +0100
> @@ -4,9 +4,7 @@
>
> config CC
> string
> -
> -config CC_VERSION
> - string
> + default "gcc"
This should have gone in config/cc/gcc.in itself, as:
config CC
default "gcc"
I'll do the change locally, no need to resend.
[--SNIP--]
> diff -r 7e569a9cb5fd -r cc545a5c1896 config/config.mk
> --- a/config/config.mk Sat Nov 16 18:14:45 2013 +0100
> +++ b/config/config.mk Wed Dec 11 21:28:37 2013 +0100
> @@ -83,7 +83,7 @@
>
> config.gen/cc.in: $(CC_CONFIG_FILES) $(CC_CONFIG_FILES_2)
> @$(ECHO) ' IN $(@)'
> - $(SILENT)$(CT_LIB_DIR)/scripts/gen_in_frags.sh choice "$@" "C compiler" "CC" "config/cc" "N" $(CCS)
> + $(SILENT)$(CT_LIB_DIR)/scripts/gen_in_frags.sh menu "$@" "C compiler" "CC" "config/cc" $(CCS)
With this change, it is now possible to generate invalid configurations,
where no C compiler is selected. This is Not Good (TM).
Why do you need that?
[--SNIP--]
> diff -r 7e569a9cb5fd -r cc545a5c1896 scripts/build/cc.sh
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/scripts/build/cc.sh Wed Dec 11 21:28:37 2013 +0100
> @@ -0,0 +1,59 @@
> +# Wrapper to build the companion tools facilities
s/companion tools facilities/C compiler components/
Yet, this be a separate patch. Please do only one semantic change per
patch, that is:
- one patch to rename the config options
- a second patch to change the config opitons (can be folded in the
previous patch, but separate helps do the review)
- a third patch to introduce this wrapper
- and so on...
I must say I do not like this wrapper: why can you handle the llvm and
clang stuff as another compiler, on-par with gcc?
The wrapper for the companion tools is needed because we do not have
proper dependency tracking about the order the components are to be
built.
[--SNIP--]
> diff -r 7e569a9cb5fd -r cc545a5c1896 scripts/build/debug/300-gdb.sh
> --- a/scripts/build/debug/300-gdb.sh Sat Nov 16 18:14:45 2013 +0100
> +++ b/scripts/build/debug/300-gdb.sh Wed Dec 11 21:28:37 2013 +0100
> @@ -181,11 +181,11 @@
> if [ "${CT_GDB_INSTALL_GDBINIT}" = "y" ]; then
> CT_DoLog EXTRA "Installing '.gdbinit' template"
> # See in scripts/build/internals.sh for why we do this
> - if [ -f "${CT_SRC_DIR}/gcc-${CT_CC_VERSION}/gcc/BASE-VER" ]; then
> - gcc_version=$( cat "${CT_SRC_DIR}/gcc-${CT_CC_VERSION}/gcc/BASE-VER" )
> + if [ -f "${CT_SRC_DIR}/gcc-${CT_CC_GCC_VERSION}/gcc/BASE-VER" ]; then
> + gcc_version=$( cat "${CT_SRC_DIR}/gcc-${CT_CC_GCC_VERSION}/gcc/BASE-VER" )
> else
> - gcc_version=$( sed -r -e '/version_string/!d; s/^.+= "([^"]+)".*$/\1/;' \
> - "${CT_SRC_DIR}/gcc-${CT_CC_VERSION}/gcc/version.c" \
> + gcc_version=$( sed -r -e '/version_string/!d; s/^.+= "([^"]+)".*$/\1/;' \
> + "${CT_SRC_DIR}/gcc-${CT_CC_GCC_VERSION}/gcc/version.c" \
> )
> fi
This whole if-else-fi block should be conditional to gcc being selected,
in the end, since otherwise the ele-part will fail. Like you did below,
BTW...
[--SNIP--]
> diff -r 7e569a9cb5fd -r cc545a5c1896 scripts/build/internals.sh
> --- a/scripts/build/internals.sh Sat Nov 16 18:14:45 2013 +0100
> +++ b/scripts/build/internals.sh Wed Dec 11 21:28:37 2013 +0100
> @@ -28,33 +28,35 @@
> CT_DoExecLog ALL "${CT_TARGET}-strip" ${strip_args} \
> "${CT_TARGET}/debug-root/usr/bin/gdbserver"
> fi
> - # We can not use the version in CT_CC_VERSION because
> - # of the Linaro stuff. So, harvest the version string
> - # directly from the gcc sources...
> - # All gcc 4.x seem to have the version in gcc/BASE-VER
> - # while version prior to 4.x have the version in gcc/version.c
> - # Of course, here is not the better place to do that...
> - if [ -f "${CT_SRC_DIR}/gcc-${CT_CC_VERSION}/gcc/BASE-VER" ]; then
> - gcc_version=$( cat "${CT_SRC_DIR}/gcc-${CT_CC_VERSION}/gcc/BASE-VER" )
> - else
> - gcc_version=$( sed -r -e '/version_string/!d; s/^.+= "([^"]+)".*$/\1/;' \
> - "${CT_SRC_DIR}/gcc-${CT_CC_VERSION}/gcc/version.c" \
> - )
> + if [ "${CT_CC_gcc}" = "y" ]; then
... here.
[--SNIP--]
> diff -r 7e569a9cb5fd -r cc545a5c1896 scripts/crosstool-NG.sh.in
> --- a/scripts/crosstool-NG.sh.in Sat Nov 16 18:14:45 2013 +0100
> +++ b/scripts/crosstool-NG.sh.in Wed Dec 11 21:28:37 2013 +0100
[--SNIP--]
> @@ -162,8 +162,8 @@
> # Put user-supplied flags at the end, so that they take precedence.
> CT_TARGET_CFLAGS="${CT_ARCH_TARGET_CFLAGS} ${CT_TARGET_CFLAGS}"
> CT_TARGET_LDFLAGS="${CT_ARCH_TARGET_LDFLAGS} ${CT_TARGET_LDFLAGS}"
> -CT_CC_CORE_EXTRA_CONFIG_ARRAY=( ${CT_ARCH_CC_CORE_EXTRA_CONFIG} "${CT_CC_CORE_EXTRA_CONFIG_ARRAY[@]}" )
> -CT_CC_EXTRA_CONFIG_ARRAY=( ${CT_ARCH_CC_EXTRA_CONFIG} "${CT_CC_EXTRA_CONFIG_ARRAY[@]}" )
> +CT_CC_GCC_CORE_EXTRA_CONFIG_ARRAY=( ${CT_ARCH_CC_CORE_EXTRA_CONFIG} "${CT_CC_GCC_CORE_EXTRA_CONFIG_ARRAY[@]}" )
> +CT_CC_GCC_EXTRA_CONFIG_ARRAY=( ${CT_ARCH_CC_EXTRA_CONFIG} "${CT_CC_GCC_EXTRA_CONFIG_ARRAY[@]}" )
Maybe now would be the proper time to move that into gcc.sh?
> # Compute the package version string
> CT_PKGVERSION="crosstool-NG ${CT_VERSION}${CT_TOOLCHAIN_PKGVERSION:+ - ${CT_TOOLCHAIN_PKGVERSION}}"
> @@ -545,8 +545,8 @@
> CT_EndStep
> fi
>
> - if [ "${CT_CC_STATIC_LIBSTDCXX}" = "y" ]; then
> - CT_DoStep DEBUG "Checking that gcc can statically link libstdc++ (CT_CC_STATIC_LIBSTDCXX)"
> + if [ "${CT_CC_GCC_STATIC_LIBSTDCXX}" = "y" ]; then
> + CT_DoStep DEBUG "Checking that gcc can statically link libstdc++ (CT_CC_GCC_STATIC_LIBSTDCXX)"
> CT_DoLog DEBUG "You may need to ensure that libstdc++.a is installed on your system"
> CT_DoExecLog DEBUG "${CT_HOST}-gcc" ${CT_CFLAGS_FOR_HOST} ${CT_LDFLAGS_FOR_HOST} "${testc}" -static -lstdc++ -o "${gccout}"
> rm -f "${gccout}"
And move this block in gcc.sh, too.
This would require a new step before cc_extract. Like we have
libc_check_config, we could introduce cc_check_config that would do this
static link test.
> diff -r 7e569a9cb5fd -r cc545a5c1896 scripts/gen_in_frags.sh
> --- a/scripts/gen_in_frags.sh Sat Nov 16 18:14:45 2013 +0100
> +++ b/scripts/gen_in_frags.sh Wed Dec 11 21:28:37 2013 +0100
> @@ -138,6 +138,9 @@
> _entry=$(printf '%s\n' "${entry}" |"${sed}" -r -s -e 's/[-.+]/_/g;')
> printf 'menuconfig %s_%s\n' "${cfg_prefix}" "${_entry}"
> printf ' bool\n'
> + if "${grep}" -E '^## default' ${file} >/dev/null 2>&1; then
> + "${sed}" -r -e '/^## default ?/!d; s/^## default ?/ default /;' ${file} 2>/dev/null
> + fi
This should also go into a separate patch: it adds yet _another_ new
feature.
Typically, this could go as the first patch in a series: since this is
very simple, it can be reviewed very easily and applied quickly*.
[*] yes, 'quickly' can be very relative, sorry... :-/
> printf ' prompt "%s"\n' "${entry}"
> "${sed}" -r -e '/^## depends on /!d; s/^## / /;' ${file} 2>/dev/null
> "${sed}" -r -e '/^## select /!d; s/^## / /;' ${file} 2>/dev/null
> diff -r 7e569a9cb5fd -r cc545a5c1896 scripts/showSamples.sh
> --- a/scripts/showSamples.sh Sat Nov 16 18:14:45 2013 +0100
> +++ b/scripts/showSamples.sh Wed Dec 11 21:28:37 2013 +0100
> @@ -76,16 +76,24 @@
> [ -z "${CT_LIBELF}" -a -z "${CT_LIBELF_TARGET}" ] || printf " libelf-%s" "${CT_LIBELF_VERSION}"
> [ -z "${complibs}" ] || printf "\n"
> printf " %-*s : %s\n" ${width} "binutils" "binutils-${CT_BINUTILS_VERSION}"
> - printf " %-*s : %s" ${width} "C compiler" "${CT_CC}-${CT_CC_VERSION} (C"
> - [ "${CT_CC_LANG_CXX}" = "y" ] && printf ",C++"
> - [ "${CT_CC_LANG_FORTRAN}" = "y" ] && printf ",Fortran"
> - [ "${CT_CC_LANG_JAVA}" = "y" ] && printf ",Java"
> - [ "${CT_CC_LANG_ADA}" = "y" ] && printf ",ADA"
> - [ "${CT_CC_LANG_OBJC}" = "y" ] && printf ",Objective-C"
> - [ "${CT_CC_LANG_OBJCXX}" = "y" ] && printf ",Objective-C++"
> - [ "${CT_CC_LANG_GOLANG}" = "y" ] && printf ",Go"
> - [ -n "${CT_CC_LANG_OTHERS}" ] && printf ",${CT_CC_LANG_OTHERS}"
> - printf ")\n"
> + printf " %-*s :" ${width} "C compilers"
> + for cc in $(compgen -A variable | sed -n 's/^CT_CC_\([^_]\+\)_VERSION$/\1/p'); do
> + cc_variable=CT_CC_${cc}_VERSION
> + version=${!cc_variable}
> + compiler=$(echo $cc | sed -E ''| awk '{print tolower($0)}')
> + printf " $compiler-$version"
> + done
> + printf "\n"
> + printf " %-*s : %s" ${width} "Languages" "C"
> + [ "${CT_CC_LANG_CXX}" = "y" ] && printf " C++"
> + [ "${CT_CC_LANG_FORTRAN}" = "y" ] && printf " Fortran"
> + [ "${CT_CC_LANG_JAVA}" = "y" ] && printf " Java"
> + [ "${CT_CC_LANG_ADA}" = "y" ] && printf " ADA"
> + [ "${CT_CC_LANG_OBJC}" = "y" ] && printf " Objective-C"
> + [ "${CT_CC_LANG_OBJCXX}" = "y" ] && printf " Objective-C++"
> + [ "${CT_CC_LANG_GOLANG}" = "y" ] && printf " Go"
> + [ -n "${CT_CC_LANG_OTHERS}" ] && printf " ${CT_CC_LANG_OTHERS}"
Here, you're again doing two unrelated changes:
- one to replace commas with spaces (to match the tools, below: good)
- one to introduce the compilers list
Really, I can't see the whole picture of all this work.
By splitting the changes into independent, self-contained patches, it's
like if you were telling a story: a patch series is a scenario, with each
change being a chapter. Then, writing a series is like writing a book:
- first, the cover-letter: you state the overal goal of the series. A
cover-letter can be very long if need be: you need to explain the
intent, the issues you're trying to solve, why you decided to do
things the way you've done them. Eg.:
This series introduces the basis needed to add Darwin as a target.
Darwin requires llvm and/or clang as a compiler. Thus we need to be
able to build two or more different compilers in the same toolchain.
So this series does:
For that, we need to differentiate the current gcc-specific options;
they are currently not tied to gcc, but are generic (eg. CT_CC_VERSION).
--> patches 1, 2 and 3
Then, we add clang, and then llvm, as two new C compilers.
--> patches 4 to 9
Finally, we make it possible to select more than one compiler. For
this we need to change the current choice into a multi-select.
--> patches 10 and 11
- then the individual patches: you explain more in details what each
patch does. If it is not obvious why a patch exist, explain that it
is a requirement for the next patch (eg. your '## default' does not
look very useful by itself, but saying "needed to select more than one
compiler, coming in a following patch, and still be able to have a
default value" is very informative).
Again, I know Ray and you have put a lot of effort in this work. nd I
haven't been very reactive.
But the sheer amount of changes in this "simple preliminary" patch is
just overwhelming.
Please, be sure to reduce the changes as much as possible: if all the
above had been split into separate patches, it would have been easier to
review and apply.
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
More information about the crossgcc
mailing list