Re: [RFT PATCH v3 3/9] add --with-sysroot

[Ralf Wildenhues]

Next one: 3/9.   This is a bit of a preliminary review, because I
haven't read all patches and all of the discussion yet.

* Paolo Bonzini wrote on Thu, Jul 29, 2010 at 01:23:16AM CEST:
> * libltdl/m4/libtool.m4 (_LT_HOST_NONCANONICAL, _LT_WITH_SYSROOT): New.
> (LT_SETUP): Require _LT_WITH_SYSROOT.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>         Right now the default is to use a sysroot.  Probably it is
>         a bit too aggressive, especially because most existing
>         cross-compilation setups are using the wrong prefix that
>         includes the sysroot.  They would thus stop working until
>         they switched to --prefix and installing with DESTDIR.

Can you explain this comment in a bit more detail?  What do you mean
with "use the wrong prefix"?  Is that, they use
  ./configure --prefix=/win-mount/usr
  make
  make install

but should be using
  ./configure --prefix=/usr
  make
  make install DESTDIR=/win-mount

instead?

Thanks.  Ideally, the answer is put in the form of a diff against the
manual.  ;-)

> --- a/libltdl/m4/libtool.m4
> +++ b/libltdl/m4/libtool.m4
> @@ -173,6 +173,7 @@ m4_require([_LT_CHECK_MAGIC_METHOD])dnl
>  m4_require([_LT_CHECK_SHAREDLIB_FROM_LINKLIB])dnl
>  m4_require([_LT_CMD_OLD_ARCHIVE])dnl
>  m4_require([_LT_CMD_GLOBAL_SYMBOLS])dnl
> +m4_require([_LT_WITH_SYSROOT])dnl
>  
>  _LT_CONFIG_LIBTOOL_INIT([
>  # See if we are running on zsh, and set the options which allow our
> @@ -1163,6 +1164,55 @@ _LT_DECL([], [ECHO], [1], [An echo program that 
> protects backslashes])
>  ])# _LT_PROG_ECHO_BACKSLASH
>  
>  
> +# _LT_HOST_NONCANONICAL
> +# ----------------
> +AC_DEFUN([_LT_HOST_NONCANONICAL],
> +[build_noncanonical=${build_alias:-$ac_build_alias}
> +host_noncanonical=${host_alias:-$build_noncanonical}

This needs to AC_REQUIRE AC_CANONICAL_{BUILD,HOST}.

> +target_noncanonical=${target_alias:-$host_noncanonical}

This either needs to AC_REQUIRE AC_CANONICAL_TARGET or act
m4-conditionally upon whether that has been provided before.
Before the patch series, $target is not automatically used
in packages using AC_PROG_LIBTOOL, it would be nice if it
could be kept that way, because it tends to cause lot of
confusion still.

If you care about ordering, an AC_BEFORE to warn about
AC_CANONICAL_TARGET after AC_PROG_LIBTOOL can be added
(the warning can be appended to the former in the same
way Libtool sticks code to AC_PROG_CC et al).

> +_LT_DECL([], [build_noncanonical], [0],
> +      [The user-supplied name of the build machine.])dnl
> +_LT_DECL([], [host_noncanonical], [0],
> +      [The user-supplied name of the host machine.])dnl

> +_LT_DECL([], [target_noncanonical], [0],
> +      [The user-supplied name of the target machine.])

Likewise for this _LT_DECL.

Actually, I don't see any other uses of target in the whole patch
series, so why not just omit that here?

> +])# _LT_HOST_NONCANONICAL
> +
> +# _LT_WITH_SYSROOT
> +# ----------------
> +AC_DEFUN([_LT_WITH_SYSROOT],
> +[AC_REQUIRE([_LT_HOST_NONCANONICAL])
> +AC_MSG_CHECKING([for sysroot])
> +AC_ARG_WITH([sysroot],
> +[  --with-sysroot[=DIR] Search for dependent libraries within DIR

Please use quadrigraphs for the optional argument.

> +                        (or the compiler's sysroot if not specified).],
> +[], [with_sysroot=no])

The GCC package specifies --with-sysroot in at least its
gcc/configure.ac as well, with slightly incompatible semantics (setting
to empty not no); how to address this?  codesearch finds other instances
as well (e.g., LLVM), I'm not sure how to address these (not even sure
they use Libtool) except for a big sign in NEWS, and documentation in
the manual.

> +dnl lt_sysroot will always be passed unquoted.  We quote it here
> +dnl in case the user passed a directory name.
> +lt_sysroot=
> +case ${with_sysroot} in #(
> + yes)
> +   if test "$GCC" = yes; then
> +     lt_sysroot=`$GCC --print-sysroot 2>/dev/null`

OK, GCC between 2.7.2.3 and 4.2 print an error message on stderr and
exit 1.  Can we blissfully assume the user knows what she's doing when
passing --with-sysroot?  I'd guess so.

> +   fi
> +   ;; #(
> + /*)

Can it be a windows C:/sys-root style path?

> +   AC_MSG_RESULT([$with_sysroot])
> +   lt_sysroot=`echo "$with_sysroot" | sed -e "$sed_quote_subst"`
> +   ;; #(
> + no)

Normalization of "no" to empty?  The rest of the series doesn't
special-case "no", and I think empty would please GCC configury, too
(untested).

> +   ;; #(
> + *)
> +   AC_MSG_RESULT([])

Why not print the borked result?

> +   AC_MSG_ERROR([The sysroot must be an absolute path.])
> +   ;;
> +esac
> +
> + AC_MSG_RESULT([${with_sysroot:-no}])

Duplicate results in the code path going through the /* case above.

> +_LT_DECL([], [lt_sysroot], [0], [The root where to search for ]dnl
> +[dependent libraries, and in which our libs should be installed.])])

s/libs/libraries/

>  # _LT_ENABLE_LOCK
>  # ---------------
>  m4_defun([_LT_ENABLE_LOCK],

Thanks,
Ralf