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 <bonz...@gnu.org>
> ---
> 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
