299669 - Allow mozilla to build with libIDL-2 for non-gtk2 backends

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Bastiaan Jacques]

Currently, configure fails when libIDL (version 1.x) is not found on backends
other than gtk2.
Bug 201836 comment #19 states that "gtk1 builds requires libIDL1", but I believe
that is not (or not anymore) the case. I've successfully built and run a gtk1
build with the upcoming patch.

Comment 1

[Bastiaan Jacques]

Attachment: fix

Comment 2

[Bastiaan Jacques]

This patch also makes the build system prefer libIDL2 over libIDL, but I presume
that's not a problem.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

so, question: did you verify that this pkgconfig syntax still allows building if
only libIDL 1 is present?

Also: should we maybe prefer libIDL 1 for gtk1 builds?

Comment 4

[Bastiaan Jacques]

Attachment: switch things around

(In reply to comment #3)
> so, question: did you verify that this pkgconfig syntax still allows building

> if only libIDL 1 is present?
As a matter of fact, it doesn't. This patch allows that, though; I've also
tested this patch with the qt backend (for libIDL2, that is).

> Also: should we maybe prefer libIDL 1 for gtk1 builds?
The new patch prefers libIDL1 if it is available.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

> As a matter of fact, it doesn't.

(I think adding a new argument containg [] to the PKG_CHECK_MODULES would've
worked for that part)

Comment 6

[Mark Mentovai]

I've been asked about this before.  Because configure was so careful about this,
I assumed that there was a build-time requirement to use libIDL-2 when using
gtk+-2.  This patch allows a gtk+-2 and libIDL-1 combination, and in fact
prefers it over gtk+-2 with libIDL-2.  Is that OK?

Comment 7

[Bastiaan Jacques]

(In reply to comment #6)
> This patch allows a gtk+-2 and libIDL-1 combination, and in fact
> prefers it over gtk+-2 with libIDL-2.  Is that OK?

A combination of libIDL1 and gtk2 worked fine when I tested the build.

Comment 8

[cls]

Comment on attachment 188256 [details] [diff] [review]
switch things around

The combination may work fine but people will still expect to use
libIDL-2/glib2 when buiding against gtk2 (see bug 156593).  We should check for
a matching set (libIDL-1/gtk1 & libIDL-2/gtk2) first, then fallback to
whatever's available.

Comment 9

[Bastiaan Jacques]

Attachment: address comment

Comment 10

[Mark Mentovai]

You've got four checks where three would do, and if you're using gtk1 or gtk2
but don't have any libIDL, you might run three of them when you only need two. 
Why split it out like that?  How about this?

if gtk1 selected
  check for libIDL-1
if libIDL not found
  check for libIDL-2
if both libIDL not found and gtk1 not selected
  check for libIDL-1

Reverse the 1s and 2s to prefer libIDL-1 when gtk has no influence, to mimic the
present behavior exactly but provide a fallback.

Also, a nit: if you want to set an undefined _LIBIDL_FOUND to empty, which you
don't need to do, do it outside of the tests instead of in the if-false block of
a test.

Comment 11

[Bastiaan Jacques]

(In reply to comment #10)
> You've got four checks where three would do, and if you're using gtk1 or gtk2
> but don't have any libIDL, you might run three of them when you only need two. 
> Why split it out like that?  How about this? [..]

I didn't do it that way, because the AM_PATH_LIBIDL macro doesn't seem to have a
nice error/abort like PKG_CHECK_MODULES.

> Also, a nit: if you want to set an undefined _LIBIDL_FOUND to empty, which you
> don't need to do, do it outside of the tests instead of in the if-false block of
> a test.
That was needed because adding a new argument with [], as biesi suggested in
comment #5 doesn't seem to be effective.

Comment 12

[cls]

Comment on attachment 188309 [details] [diff] [review]
address comment

Trivial nit that can be fixed upon checkin: use 4 space indention.

Comment 13

[Benjamin Smedberg]

Comment on attachment 188309 [details] [diff] [review]
address comment

a=bsmedberg with spacing fixed

Comment 14

[Bastiaan Jacques]

Attachment: patch for checkin

Fixes indentation.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1488; previous revision: 1.1487
done