Re: [PATCH 3 of 5] scripts/eglibc: add support for locales

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

BenoÃt, All,

On Thursday 28 July 2011 21:10:11 BenoÃt THÃBAUDEAU wrote:
> # HG changeset patch
> # User "BenoÃt THÃBAUDEAU" <benoit.thebaudeau@advansee.com>
> # Date 1311851059 -7200
> # Node ID 3b3715d0f1d412df91e472ed563d0cde0afedc6b
> # Parent  4fed02058cc3276180d6014899062f83e620eb37
> scripts/eglibc: add support for locales

Patch is globally good, but for a few issues, see inlined below.

> Signed-off-by: "BenoÃt THÃBAUDEAU" <benoit.thebaudeau@advansee.com>
> 
> diff --git a/scripts/build/libc/eglibc.sh b/scripts/build/libc/eglibc.sh
> --- a/scripts/build/libc/eglibc.sh
> +++ b/scripts/build/libc/eglibc.sh
> @@ -14,6 +14,7 @@
>  # snapshots available.
>  do_libc_get() {
>      local addon
> +    local -a extra_addons
>      local svn_base
>  
>      if [ "${CT_EGLIBC_HTTP}" = "y" ]; then
> @@ -31,7 +32,11 @@
>                "${svn_base}/libc"            \
>                "${CT_EGLIBC_REVISION:-HEAD}"
>  
> -    for addon in $(do_libc_add_ons_list " "); do
> +    if [ "${CT_LIBC_LOCALES}" = "y" ]; then
> +        extra_addons+=("localedef")
> +    fi
> +
> +    for addon in $(do_libc_add_ons_list " ") ${extra_addons[@]}; do

Always quote variable expansion, unless it is strictly required not to. In
the array-variables, expansion of "${foo[@]}" expands to as many entries
there are in the array, not to a single entry, exactly like "$@" does for
args. Similarly, "${foo[*]}" behaves like "$*".

Here, we want a word for each entry in the array 'extra_addons', even if
those entries have embedded spaces (I doubt any sane add-on would ever
have a space in its name, but never ever trust developpers' insanity after
10+ coffees and a 20+ hours stuck in front of a screen! :-])

>          # Never ever try to download these add-ons,
>          # they've always been internal
>          case "${addon}" in
> @@ -97,13 +102,69 @@
>  }
>  
>  # Extract the files required for the libc locales
> -# Nothing to do
>  do_libc_locales_extract() {
> -    :
> +    CT_Extract "eglibc-localedef-${CT_LIBC_VERSION}"
> +    CT_Patch "eglibc" "localedef-${CT_LIBC_VERSION}"
>  }
>  
>  # Build and install the libc locales
> -# Not yet supported
>  do_libc_locales() {
> -    :
> +    local libc_src_dir="${CT_SRC_DIR}/eglibc-${CT_LIBC_VERSION}"
> +    local src_dir="${CT_SRC_DIR}/eglibc-localedef-${CT_LIBC_VERSION}"
> +    local -a extra_config
> +    local -a localedef_opts
> +
> +    mkdir -p "${CT_BUILD_DIR}/build-localedef"
> +    cd "${CT_BUILD_DIR}/build-localedef"
> +
> +    CT_DoLog EXTRA "Configuring C library localedef"
> +
> +    if [ "${CT_LIBC_EGLIBC_HAS_PKGVERSION_BUGURL}" = "y" ]; then
> +        extra_config+=("--with-pkgversion=${CT_PKGVERSION}")
> +        [ -n "${CT_TOOLCHAIN_BUGURL}" ] && extra_config+=("--with-bugurl=${CT_TOOLCHAIN_BUGURL}")
> +    fi
> +
> +    CT_DoLog DEBUG "Extra config args passed: '${extra_config[*]}'"
> +
> +    # ./configure is misled by our tools override wrapper for bash
> +    # so just tell it where the real bash is _on_the_target_!
> +    # Notes:
> +    # - ${ac_cv_path_BASH_SHELL} is only used to set BASH_SHELL
> +    # - ${BASH_SHELL}            is only used to set BASH
> +    # - ${BASH}                  is only used to set the shebang
> +    #                            in two scripts to run on the target
> +    # So we can safely bypass bash detection at compile time.
> +    # Should this change in a future eglibc release, we'd better
> +    # directly mangle the generated scripts _after_ they get built,
> +    # or even after they get installed... eglibc is such a sucker...
> +    echo "ac_cv_path_BASH_SHELL=/bin/bash" >>config.cache
> +
> +    # Configure with --prefix the way we want it on the target...
> +
> +    CT_DoExecLog CFG                                                \
> +    "${src_dir}/configure"                                          \
> +        --prefix=/usr                                               \
> +        --cache-file="$(pwd)/config.cache"                          \
> +        --with-glibc="${libc_src_dir}"                              \
> +        "${extra_config[@]}"
> +
> +    CT_DoLog EXTRA "Building C library localedef"
> +    CT_DoExecLog ALL make ${JOBSFLAGS}
> +
> +    # Set the localedef endianness option
> +    case "${CT_ARCH_BE},${CT_ARCH_LE}" in
> +        y,) localedef_opts+=(--big-endian);;
> +        ,y) localedef_opts+=(--little-endian);;
> +    esac
> +
> +    # Set the localedef option for the target's uint32_t alignment in bytes.
> +    # This is target-specific, but for now, 32-bit alignment should work for all
> +    # supported targets, even 64-bit ones.
> +    localedef_opts+=(--uint32-align=4)
> +
> +    CT_DoLog EXTRA "Installing C library locales"
> +    CT_DoExecLog ALL make ${JOBSFLAGS}                              \
> +                          "LOCALEDEF_OPTS=${localedef_opts[*]}"     \
> +                          install_root="${CT_SYSROOT_DIR}"          \
> +                          install-locales
>  }

Otherwise looks good. 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.  |
'------------------------------^-------^------------------^--------------------'

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