From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12335 invoked by alias); 12 Dec 2013 22:07:37 -0000 Mailing-List: contact crossgcc-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: crossgcc-owner@sourceware.org Received: (qmail 12322 invoked by uid 89); 12 Dec 2013 22:07:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f175.google.com Received: from mail-wi0-f175.google.com (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 12 Dec 2013 22:07:34 +0000 Received: by mail-wi0-f175.google.com with SMTP id hi5so256829wib.14 for ; Thu, 12 Dec 2013 14:07:31 -0800 (PST) X-Received: by 10.194.85.75 with SMTP id f11mr8821787wjz.14.1386886051405; Thu, 12 Dec 2013 14:07:31 -0800 (PST) Received: from ymorin.is-a-geek.org (ks3095497.kimsufi.com. [94.23.60.27]) by mx.google.com with ESMTPSA id q19sm1436055wiw.4.2013.12.12.14.07.24 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Thu, 12 Dec 2013 14:07:25 -0800 (PST) Received: by ymorin.is-a-geek.org (sSMTP sendmail emulation); Thu, 12 Dec 2013 23:07:22 +0100 Date: Thu, 12 Dec 2013 22:07:00 -0000 From: "Yann E. MORIN" To: Yann Diorcet Cc: crossgcc@sourceware.org Subject: Re: [PATCH 1 of 1] cc/gcc: Split gcc configurations and functions from cc ones Message-ID: <20131212220722.GB1639@free.fr> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2012-12-30) X-IsSubscribed: yes X-SW-Source: 2013-12/txt/msg00018.txt.bz2 Yann, All, On 2013-12-11 21:41 +0100, Yann Diorcet spake thusly: > # HG changeset patch > # User Yann Diorcet > # 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 > > 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