559854 - Compile target xpidl only if libIDL is configured when cross compiling.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jacek Caban]

Attachment: fix v1

Currently xpidl's Makefile.in has some checks when to compile xpidl for target when cross compiling. Missing target xpidl is not critical and not requiring it avoids libIDL dependence. Makefile.in also duplicates configure's job, where deciding wherever to compile it or not xpidl belongs (currently configure may success and compilation may fail, which is bad).

My proposal is to depend only on LIBIDL_LIBS in xpidl Makefile.in and leave the rest for configure script. If it's set, we assume that xpidl can be compiled and compile it, otherwise not (note that host xpidl is still compiled and that's all that's needed for successful compilation).

Could someone push it to try server, please?

Comment 1

[Jacek Caban]

Attachment: fix v1.1

Thanks mcsmurf for pusing it to try server. The only failure was mac cross compiling:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271438429.1271439216.29250.gz
The attached version explicitly disables of libIDL check in configure.in for such case, just as Makefile.in did before the patch.

Comment 2

[(not currently active) Ted Mielczarek]

I'm not even sure why we build a target xpidl. For the libxul SDK, I guess?

Comment 3

[Jacek Caban]

(In reply to comment #2)
> I'm not even sure why we build a target xpidl. For the libxul SDK, I guess?

That's what I guess as well and thus making it a hard dependency doesn't sound right to me.

Comment 4

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/510669ff9ba1

Comment 5

[(not currently active) Ted Mielczarek]

Backed out, this broke the Mac universal build:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1273069564.1273069650.20823.gz
http://hg.mozilla.org/mozilla-central/rev/1b3deed2d784

Comment 6

[Jacek Caban]

Attachment: fix v2.0

I've done more serious changes than previously intended. Host flags and libs were mixed with target ones in configure.in. Also most checks for libIDL were done for target platform, while they should be done for host version. I've changed it trying to keep existing configure option meaning. This way I could even get rid of manual HOST_LIBIDL_CONFIG setting in my mozconfig.

BTW, I've removed a hack for libIDL <= 0.6.7. The hack was back from March 2007 and comment said to remove it when we'll start requiring 0.6.8. I think it's okay to require it now and the hack caused problems with my patch.

The patch passes on try server.

Comment 7

[Jacek Caban]

Attachment: fix v2.1

Fixed a typo.

Comment 8

[Jacek Caban]

Attachment: fix v2.1 rebased

The patch rebased after changes from bug 561464 (that wouldn't be needed with this patch).

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 445310 [details] [diff] [review]
fix v2.1

I had started reviewing this, I'll finish on the new patch:

>commit a5ff1e8c7aab506046a45b6e7614a4e3bc1b0362
>Author: Jacek Caban <[e-mail]>
>Date:   Wed May 12 12:01:05 2010 +0200
>
>    bug 559854 - Compile target xpidl only if libIDL is configured when cross compiling.
>
>diff --git a/build/autoconf/libIDL.m4 b/build/autoconf/libIDL.m4
>index c558b0b..7c9c86f 100644
>--- a/build/autoconf/libIDL.m4
>+++ b/build/autoconf/libIDL.m4
>@@ -18,14 +18,14 @@ AC_ARG_ENABLE(libIDLtest, [  --disable-libIDLtest    Do not try to compile and r
> 
>   if test x$libIDL_config_exec_prefix != x ; then
>      libIDL_config_args="$libIDL_config_args --exec-prefix=$libIDL_config_exec_prefix"
>-     if test x${LIBIDL_CONFIG+set} != xset ; then
>-        LIBIDL_CONFIG=$libIDL_config_exec_prefix/bin/libIDL-config
>+     if test x${HOST_LIBIDL_CONFIG+set} != xset ; then
>+        HOST_LIBIDL_CONFIG=$libIDL_config_exec_prefix/bin/libIDL-config
>      fi
>   fi
>   if test x$libIDL_config_prefix != x ; then
>      libIDL_config_args="$libIDL_config_args --prefix=$libIDL_config_prefix"
>-     if test x${LIBIDL_CONFIG+set} != xset ; then
>-        LIBIDL_CONFIG=$libIDL_config_prefix/bin/libIDL-config
>+     if test x${HOST_LIBIDL_CONFIG+set} != xset ; then
>+        HOST_LIBIDL_CONFIG=$libIDL_config_prefix/bin/libIDL-config
>      fi
>   fi
> 
>@@ -34,30 +34,26 @@ AC_ARG_ENABLE(libIDLtest, [  --disable-libIDLtest    Do not try to compile and r
>   dnl Force a version check to keep upgraded versions from being overridden by the cached value.
>   unset ac_cv_path_LIBIDL_CONFIG
> 
>-  AC_PATH_PROG(LIBIDL_CONFIG, libIDL-config, no)
>+  AC_PATH_PROG(HOST_LIBIDL_CONFIG, libIDL-config, no)
>   min_libIDL_version=ifelse([$1], ,0.6.0,$1)
>   AC_MSG_CHECKING(for libIDL - version >= $min_libIDL_version)
>   no_libIDL=""
>-  if test "$LIBIDL_CONFIG" = "no" ; then
>+  if test HOST_"$LIBIDL_CONFIG" = "no" ; then

This test looks broken.

>diff --git a/configure.in b/configure.in
>index e54cce7..c398871 100644
>--- a/configure.in
>+++ b/configure.in
>+    case "$host" in
>+    *-mingw*|*-cygwin*|*-msvc*|*-mks*)
>+        if test -n "$GLIB_PREFIX"; then
>+            _GLIB_PREFIX_DIR=`cd $GLIB_PREFIX && pwd -W`

Man, this is a lot of crud. I was hopeful that you were removing it (from the previous hunk), but I guess you just moved it. :-( I think we could probably simplify this all and not support different glib prefixes on windows hosts. Everyone should just be using MozillaBuild at this point anyway, which has glib and libIDL in $MOZ_TOOLS_DIR.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 448754 [details] [diff] [review]
fix v2.1 rebased

r+ if you fix that broken test I pointed out.

Comment 11

[Jacek Caban]

(In reply to comment #9)
> >-  if test "$LIBIDL_CONFIG" = "no" ; then
> >+  if test HOST_"$LIBIDL_CONFIG" = "no" ; then
> 
> This test looks broken.

Right, thanks.

> >diff --git a/configure.in b/configure.in
> >index e54cce7..c398871 100644
> >--- a/configure.in
> >+++ b/configure.in
> >+    case "$host" in
> >+    *-mingw*|*-cygwin*|*-msvc*|*-mks*)
> >+        if test -n "$GLIB_PREFIX"; then
> >+            _GLIB_PREFIX_DIR=`cd $GLIB_PREFIX && pwd -W`
> 
> Man, this is a lot of crud. I was hopeful that you were removing it (from the
> previous hunk), but I guess you just moved it. :-( I think we could probably
> simplify this all and not support different glib prefixes on windows hosts.
> Everyone should just be using MozillaBuild at this point anyway, which has glib
> and libIDL in $MOZ_TOOLS_DIR.

The patch simplifies things anyway... I will look at simplifying it when the patch will be landed.

> r+ if you fix that broken test I pointed out.

Thanks for the review.

Comment 12

[Jacek Caban]

Attachment: fix v2.1 (fiixed typo)

Comment 13

[Jacek Caban]

Thanks for landing:
http://hg.mozilla.org/mozilla-central/rev/0388c837c986

Comment 14

[Dave Townsend [:mossop]]

Looks like this broke building 32-bit Firefox on Snow Leopard

Comment 15

[Jacek Caban]

I have only Leopard here to test and there is no such build on tinderbox. Could you please attach config.log, config/autoconf.mk and console output of compilation failure?

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> I have only Leopard here to test and there is no such build on tinderbox. Could
> you please attach config.log, config/autoconf.mk and console output of
> compilation failure?

filed bug 570606