Re: [PATCH] libc/prebuilt-glibc: add support for a pre-built GLIBC

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

Michael, All,

On Monday 14 November 2011 03:43:31 Michael Hope wrote:
> # HG changeset patch
> # User Michael Hope <michael.hope@linaro.org>
> # Date 1321238468 -46800
> # Branch prebuilt-sysroot
> # Node ID ebfc38601b7351231f742d5dbc4db71c878a3a5d
> # Parent  20bc56e72ca1559279b3c9c9b191d41061757653
> libc/prebuilt-glibc: add support for a pre-built GLIBC

I don't like using pre-build components. crosstool-NG is from the
beginning a way to build stand-alone and reproducible toolchains
using the sources, and only the sources.

I understand there could be a need for using a pre-built sysroot,
though. But then, it has to be much more generic than this.

Next time someone needs another base as sysroot, I do not want to add
yet another config+script, and yet another every time...

What if:
 - we add an option in the toolchain sub-menu, like:
      config SYSROOT_PREBUILT
          bool
          prompt "Use an existing sysroot"
      config SYSROOT_PREBUILT_PATH
          string
          depends on SYSROOT_PREBUILT
          prompt "Path to that sysroot"
 - make the kernel and C library selections depend on ! SYSROOT_PREBUILT
   but then, we'd still need to know the C library kind (glibc/uClibc) so
   we can contruct the tuple...
 - copy that custom sysroot to its final place in the toolchain

Of course, that would need preparing the prebuilt sysroot beforehand,
and not from inside crosstool-NG.

But on the whole, that does not sound very much like something I would
like to see in crosstool-NG anyway... You can try to trick^Wconvince me
otherwise, though! ;-)

> Used to build a cross compiler against an existing sysroot such as
> Debian or Ubuntu.  Adds Ubuntu Maverick (the last before Ubuntu added
> multiarch) as an example.  Can be extended to support Debian 6 and
> Fedora but left as an exercise to the reader :)
> 
> Warts:
>  * Still builds the intermediate stage compilers even though they're
>    not needed

Could be pretty easy not to build them if not required:
 - in config/cc.in:
    config CC_CORE_NEEDED
        bool
        depends on ! SYSROOT_PREBUILT
        default y

 - then in scripts/build/cc/gcc.sh:
    if [ "${CT_CC_CORE_NEEDED}" != "y" ]; then
        return 0
    fi

>  * Still pulls in the linux headers even though they're incompletely
>    overwritten

That indeed is bad.


Some review below, just for the sake of reviewing. ;-)

> Signed-off-by: Michael Hope <michael.hope@linaro.org>
> 
> diff -r 20bc56e72ca1 -r ebfc38601b73 config/libc/prebuilt-glibc.in
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/config/libc/prebuilt-glibc.in	Mon Nov 14 15:41:08 2011 +1300
> @@ -0,0 +1,29 @@
> +# Prebuilt GLIBC options
> +
> +## depends on ! MINGW32 && ! BARE_METAL && ARCH_USE_MMU

As this is valid only for ARM, maybe: depends on ARCH_arm

[--SNIP--]
> diff -r 20bc56e72ca1 -r ebfc38601b73 scripts/build/libc/prebuilt-glibc.sh
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/scripts/build/libc/prebuilt-glibc.sh	Mon Nov 14 15:41:08 2011 +1300
> @@ -0,0 +1,110 @@
> +# This file adds functions to fetch and use a prebuilt GLIBC such as
> +# Debian or Ubuntu.
> +# Copyright 2011  Linaro Limited
> +# Licensed under the GPL v2. See COPYING in the root of this package
> +
> +# Convert a crosstool-NG architecture into the Debian form
> +do_libc_to_debian_arch() {
> +    local arch
> +    local endian="${CT_ARCH_BE},${CT_ARCH_LE}"

Hmm. looks like an endian string is needed, same as the float string.

> +    local abi="${CT_ARCH_FLOAT_SW},${CT_ARCH_FLOAT_HW}"

There is this brand new ${CT_ARCH_FLOAT} string that makes it so much
easier to test floating point! ;-)

> +    case "${CT_ARCH},${endian},${abi}" in
> +	arm,,y,y,)  arch=armel;;

Spaces, no tabs.

> +	arm,y,,y,)  arch=armeb;;
> +	arm,,y,,y)  arch=armhf;;

<off-topic>

 - IIRC, this was introduced by Debian to differentiate their softfloat
   armv5 (or v4?) port from their hardfloat armv7(?) port. Right?
 - So, is 'armhf' the definitive canonical way to describe a little
   endian, hard-FP ARM target?
 - How does it plays with config.sub and config.guess?
 - Maybe we need to add this support in the arm.sh script, too, no?

</off-topic>

> +	*)          CT_Abort "Unhandled architecture \"${CT_ARCH}\"";;
> +    esac
> +
> +    echo "${arch}"
> +}
> +
> +# Generate the list of files to fetch for the particular distro and
> +# arch
> +do_libc_file_list() {
> +    local arch
> +    local libc
> +    local linux
> +    local files

Use an array variable:
    local -a files
    files+=( value1 )
    files+=( value2 )
    echo "${files[@]}"

> +    case "${CT_LIBC_VERSION}" in
> +	ubuntu_10_10)
> +	    arch=$( do_libc_to_debian_arch )
> +	    libc="2.12.1-0ubuntu6"
> +	    linux="2.6.35-1022.33"
> +	    files="libc6_${libc}_${arch}"
> +	    files="${files} libc6-dev_${libc}_${arch}"
> +	    files="${files} linux-libc-dev_${linux}_${arch}"

That's another reason I do not like that: hidding a component (here: the
Linux kernel headers) inside a second component (here: the C library).

Let's do it just right: we need an separate way to specify a place where
to get a pre-populated sysroot.

> +	    ;;
> +	*)  CT_Abort "Unhandled libc version ${CT_LIBC_VERSION}"
> +    esac
> +
> +    echo "${files}"
> +}
> +
> +do_libc_get() {
> +    local pool
> +    local ext
> +
> +    # Where to pull packages from
> +    case "${CT_LIBC_VERSION}" in
> +	ubuntu_*)
> +	    pool="http://ports.ubuntu.com/pool";
> +	    ext=".deb"
> +	    ;;
> +    esac
> +
> +    files=$( do_libc_file_list )
> +
> +    for file in ${files}; do
> +	CT_DoLog DEBUG "Fetching ${file}"
> +	CT_GetFile "${file}" "${ext}" \
> +	    "${pool}/main/{e/eglibc,l/linux}"

This does not expands properly for me:
    $ echo "/main/{e/eglibc,l/linux}"
    /main/{e/eglibc,l/linux}

> +    done
> +
> +    return 0
> +}
> +
> +do_libc_extract() {
> +    for file in $( do_libc_file_list ); do
> +	CT_Extract "${file}"
> +    done
> +}
> +
> +do_libc_check_config() {
> +    :

If the test for the supported architectures were to be done at runtime,
they should be done here, as this is the very first (build-)step that
is executed, so it bails early in the build process.

> +}
> +
> +do_libc_start_files() {
> +    # do_kernel_headers has already run
> +    CT_DoLog EXTRA "Installing the pre-built libc"
> +
> +    for file in $( do_libc_file_list ); do
> +	CT_DoExecLog ALL cp -av "${CT_SRC_DIR}/${file}"/* "${CT_SYSROOT_DIR}"
> +    done

And this is where all goes amiss: the kernel headers are already installed
by the previous step, and the new ones will overwrite them, possibly
leaving unwanted headers. And you get a probably-broken sysroot.

> +    # Some packages include absolute links in sysroot/usr/lib.
> +    # Convert to relative links instead
> +    for lib in "${CT_SYSROOT_DIR}/usr/lib"/*; do \
> +	if [ -L "${lib}" ] && (readlink "${lib}" | grep -q ^/); then

grep -q is not portable, use: ${grep} blabla >/dev/null 2>&1

Also, (single- or double-) quote strings, even constants: '^/'
This is un-needed for scripts, I know, but this makes for some
homogeneity: "a string is a string, so it is quoted". :-)

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