Re: [PATCH] debug/cross-gdb: check host dependencies

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

Benoit, All,

See comments inlined below...

On Monday 06 June 2011 16:50:25 BenoÃt THÃBAUDEAU wrote:
> # HG changeset patch
> # User "BenoÃt THÃBAUDEAU" <benoit.thebaudeau@advansee.com>
> # Date 1307370928 -7200
> # Node ID ca6222a138d007bcc7403276c924db0259a0f178
> # Parent  2ab553e37517082017b10e992f5b0f5390fc053c
> debug/cross-gdb: check host dependencies
> 
> Cross-gdb depends on expat and python. If either is missing, cross-gdb will
> build successfully, but lacking some features.
> 
> Especially, if expat is missing, cross-gdb will be unable to parse the target
> description, which may lead to runtime malfunctions and the following GDB
> warning:
> "Can not parse XML target description; XML support was disabled at compile time"
> 
> Hence, expat should be considered mandatory.
> 
> On the other hand, the features missing without python are not critical, so
> python should not be considered mandatory.
> 
> This patch does the following:
>  - At configure time, warn the user if either expat or python is missing.
>  - In menuconfig, disable the static build options regarding cross-gdb if no
>    static version of expat is available, and disable cross-gdb if expat is
>    missing.
> 
> Signed-off-by: "BenoÃt THÃBAUDEAU" <benoit.thebaudeau@advansee.com>
> 
> diff --git a/config/debug/gdb.in.cross b/config/debug/gdb.in.cross
> --- a/config/debug/gdb.in.cross
> +++ b/config/debug/gdb.in.cross
> @@ -3,10 +3,14 @@
>  config STATIC_TOOLCHAIN
>      select GDB_CROSS_STATIC if GDB_CROSS
>  
> +comment "Cross-gdb - disabled (requires expat; re-run ./configure to enable)"
> +    depends on !CONFIGURE_has_expat_header || !CONFIGURE_has_expat_library
> +
>  config GDB_CROSS
>      bool
>      prompt "Cross-gdb"
>      default y
> +    depends on CONFIGURE_has_expat_header && CONFIGURE_has_expat_library

Why do we need to have two options: headers and libs? Can't we live with
a single option that is the 'aggregate' of the two?

Indeed, the ./configure infrastructure currently does not allow checking
both a header *and* a lib atomically; we currently need to call has_or_*
twice, once for the lib, once for the header. Maybe it would make sense
to be able to check both in a single run?

>      select GDB_GDBSERVER if ! BARE_METAL
>      help
>        Build and install a cross-gdb for the target, to run on host.
> @@ -16,6 +20,7 @@
>  config GDB_CROSS_STATIC
>      bool
>      prompt "Build a static cross gdb"
> +    depends on CONFIGURE_has_static_expat
>      help
>        A static cross gdb can be usefull if you debug on a machine that is
>        not the one that is used to compile the toolchain.
> diff --git a/config/toolchain.in b/config/toolchain.in
> --- a/config/toolchain.in
> +++ b/config/toolchain.in
> @@ -52,6 +52,7 @@
>      bool
>      default y
>      depends on CONFIGURE_has_static_libstdcxx
> +    depends on !GDB_CROSS || CONFIGURE_has_static_expat

I would like to avoid as much as possible that early options in the menu
depends on later options. I mean, I want to avoid the user having to go
back and forth in the menuconfig to see if new options appeared or
disapeared.

CT_STATIC_TOOLCHAIN is early is the menu, while CT_GDB_CROSS is much
later. Which means that, given this initial state (static expat missing):
  CT_GDB_CROSS=y

then firing menuconfig, going to disable CT_GDB_CROSS. This means that the
CT_STATIC_TOOLCHIAN option suddenly appears, and the user would have to go
back into an earlier menu to see that a new option is available.

I would suggest that we instead have CT_GDB_CROSS depends on
CT_STATIC_TOOLCHAIN, a bit like:

  config GDB_CROSS
      bool
      depends on ! STATIC_TOOLCHAIN || has_static_expat
      depends on has_expat_lib && has_expat_hdr

This means that, CT_STATIC_TOOLCHAIN will not depend on CT_GDB_CROSS, while
CT_GDB_CROSS will indeed not be selectable is static toolchain is required,
but static expat is missing.

Yes, this is the reverse as what is done for the static libstdc++. But that's
not really the same context. libstdc++ *is* absolutely required to build a
toolchain; static libstdc++ *is* absolutely requried to build a static
toolchain.

OTOH, static libexpat is *not* absolutely required. It just means that a sub,
non-mandatory component is not buildable.

Briefly looking at the code, it seems rather easy. We migh just need to
change:
  case "${prog}:${inc}:${lib}" in
    ...
  esac

with a succession of independent if-then-fi:
  if [ -n "${prog}" ]; then
    ...
  fi
  if [ -n "${inc}" ]; then
    ...
  fi
  if [ -n "${inc}" ]; then
    ...
  fi

This way, we can check at the same time a prog, an inc and a lib. Not sure
it makes sense to check for a prog, except maybe if we want to check a
version at the same time, such as (purely fictitious):
  has_or_abort prog="expat-config" ver="1.2.3" \
               inc="expat.h"                   \
               lib="libexpat.so"               \
               err="'libexpat 1.2.3' is required to foobar the buz"

Dunno...

>      # Add new deps here! :-)
>  
>  config STATIC_TOOLCHAIN
> diff --git a/configure b/configure
> --- a/configure
> +++ b/configure
> @@ -444,6 +444,42 @@
>               err="static 'libstdc++' is needed to statically link the toolchain's executables" \
>               kconfig=has_static_libstdcxx
>  
> +has_or_warn  inc="expat.h" \
> +             err="The 'expat' header file is needed to build cross-gdb" \
> +             kconfig=has_expat_header
> +
> +expat_libs="$( for x in so dylib a; do \
> +                   printf "libexpat.$x "; \
> +               done \
> +             )"
> +has_or_warn  lib="${expat_libs}" \
> +             err="The 'expat' library is needed to link cross-gdb's executables" \
> +             kconfig=has_expat_library
> +
> +# Yes, we may be checking twice for libexpat.a
> +# The first is because we need one instance of libexpat (shared or static)
> +# because it is needed for cross-gdb; the second is because the static version
> +# is required for static-linking, and if missing, the option is removed.
> +has_or_warn  lib="libexpat.a" \
> +             err="static 'expat' is needed to statically link cross-gdb's executables" \
> +             kconfig=has_static_expat
> +
> +python_incs="$( for v in 7 6 5 4; do \
> +                    printf "python2.$v/Python.h "; \
> +                done \
> +              )"
> +has_or_warn  inc="${python_incs}" \
> +             err="The 'python' header file is needed for some features of cross-gdb"
> +
> +python_libs="$( for v in 7 6 5 4; do \
> +                    for x in so dylib a; do \
> +                        printf "libpython2.$v.$x "; \
> +                    done; \
> +                done \
> +              )"
> +has_or_warn  lib="${python_libs}" \
> +             err="The 'python' library is needed for some features of cross-gdb"

Your checking for both the header and the libs (twice, once for expat, and
once for Python) really seems to imply we need a way to check both at the
same time.

> +
>  #---------------------------------------------------------------------
>  # Compute the version string
>  
> diff --git a/scripts/build/debug/300-gdb.sh b/scripts/build/debug/300-gdb.sh
> --- a/scripts/build/debug/300-gdb.sh
> +++ b/scripts/build/debug/300-gdb.sh
> @@ -139,6 +139,7 @@
>              --prefix="${CT_PREFIX_DIR}"                 \
>              --with-build-sysroot="${CT_SYSROOT_DIR}"    \
>              --with-sysroot="${CT_SYSROOT_DIR}"          \
> +            --with-expat=yes                            \
>              --disable-werror                            \
>              "${cross_extra_config[@]}"

Don't we have to disable something if Python is missing, or is it automagic?

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