Walter Meinl Assignee
	Description • 18 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1

Building with --enable-default-toolkit=gtk2 (not cairo-gtk2) fails at the moment. This seems to be a fallout from bug333640 .
nsFontMetricsPango.o: In function `nsFontMetricsPango::CacheFontMetrics()':
nsFontMetricsPango.cpp:(.text._ZN18nsFontMetricsPango16CacheFontMetricsEv+0x82): undefined reference to `pango_fc_font_get_type'
nsFontMetricsPango.cpp:(.text._ZN18nsFontMetricsPango16CacheFontMetricsEv+0xa4): undefined reference to `pango_fc_font_lock_face'
nsFontMetricsPango.cpp:(.text._ZN18nsFontMetricsPango16CacheFontMetricsEv+0x2f7): undefined reference to `pango_fc_font_has_char'
nsFontMetricsPango.cpp:(.text._ZN18nsFontMetricsPango16CacheFontMetricsEv+0x5c2): undefined reference to `pango_fc_font_unlock_face'
nsFontMetricsPango.cpp:(.text._ZN18nsFontMetricsPango16CacheFontMetricsEv+0x6ef): undefined reference to `pango_fc_font_get_glyph'
mozilla-decoder.o: In function `mozilla_decoder_get_type':
mozilla-decoder.cpp:(.text.mozilla_decoder_get_type+0x40): undefined reference to `pango_fc_decoder_get_type'
mozilla-decoder.o: In function `mozilla_decoder_class_intern_init(void*)':
mozilla-decoder.cpp:(.text._Z33mozilla_decoder_class_intern_initPv+0x3e): undefined reference to `pango_fc_decoder_get_type'
mozilla-decoder.o: In function `mozilla_decoder_get_glyph(_PangoFcDecoder*, _PangoFcFont*, unsigned int)':
mozilla-decoder.cpp:(.text._Z25mozilla_decoder_get_glyphP15_PangoFcDecoderP12_PangoFcFontj+0x99): undefined reference to `pango_fc_font_lock_face'
mozilla-decoder.cpp:(.text._Z25mozilla_decoder_get_glyphP15_PangoFcDecoderP12_PangoFcFontj+0xfa): undefined reference to `pango_fc_font_unlock_face'
mozilla-decoder.o: In function `mozilla_find_decoder(_FcPattern*, void*)':
mozilla-decoder.cpp:(.text._Z20mozilla_find_decoderP10_FcPatternPv+0x134): undefined reference to `pango_fc_decoder_get_type'
mozilla-decoder.o: In function `mozilla_decoders_init':
mozilla-decoder.cpp:(.text.mozilla_decoders_init+0x9e1): undefined reference to `pango_fc_font_map_get_type'
2mozilla-decoder.cpp:(.text.mozilla_decoders_init+0x9fc): undefined reference to `pango_xft_get_font_map'
mozilla-decoder.cpp:(.text.mozilla_decoders_init+0xa2c): undefined reference to `pango_fc_font_map_add_decoder_find_func'
collect2: ld gab 1 als Ende-Status zurück
gmake[5]: *** [libgfx_gtk.so] Fehler 1

When I add $(MOZ_PANGO_LIBS) to EXTRA_DSO_LDOPTS in gfx/src/gtk/Makefile.in the linker finds the necessary dependencies. Probably a similar situation as in bug335568 I added it to the ifdef MOZ_ENABLE_GTK2 section as I understood that pango has to be enabled anyway in gtk2 builds (suggestions?)
However, as I am on gtk-2.8 I am then still stuck in the issues of bug305185
I can resolve this issue without needing pangocairo when I check in configure.in also for pangoxft the same way as it is done for the cairo-gtk2 build. Due to this check pangoxft will be added to the MOZ_PANGO_LIBS variable and the mozilla-decoder.cpp problems are gone without pulling pango-cairo.
Wolfgang, Biesie it would be nice to get comments from you.

Reproducible: Always

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 3 • 18 years ago

but isn't this the same as bug 305185?

	Walter Meinl Assignee
	Comment 4 • 18 years ago

(In reply to comment #3)
> but isn't this the same as bug 305185?
> 
No, when I apply the (modified to the trunk) patch from 305185 I get the same errors starting with pango_fc_.....
except the pango_xft_get_font_map error, which is the one making trouble in 305185.
Moreover, when I apply the 305185 patch and backout the -z defs lines in configure.in (from bug 333640) it compiles.

One more thing, I just compiled the beast the first time with enable-static. The attached patches are not enough for building static. However adding EXTRA_DSO_LDOPTS += $(MOZ_PANGO_LIBS) to config/static-config.mk would solve the static problem.

	Walter Meinl Assignee
	Comment 6 • 18 years ago

(In reply to comment #5)
> Created an attachment (id=222532) [edit]
> additionally needed for building static
> 
Using this patch it is also possible to generate a static cairo-gtk2 build with gtk-2.8 without any further changes to the code (no pulling of pangocairo)

	Walter Meinl Assignee
	Comment 7 • 18 years ago

update summary address build config

	Walter Meinl Assignee
	Comment 8 • 18 years ago

Comment on attachment 222532 [details] [diff] [review]
additionally needed for building static (checked in on trunk)

Benjamin, I'll ask you for review for this patch, since you just gave review+ for an alternate patch in bug312951. That patch would certainly allow FF to be linked statically. However as the default toolkit for linux is now cairo-gtk2 TB now fails when statically linked on missing $(MOZ_PANGO_LIBS). Therefore, its probably the better way to include those in config/static-config.mk. (as was done in bug 332170 for Xlibs dependencies) This lets both be linked correctly.

	Walter Meinl Assignee
	Comment 9 • 18 years ago

After the check-ins of bug333640 into MOZILLA_1_8_BRANCH, I see the same linker problems as described in comment #0 for the trunk. Though the patch from bug305185 still applies cleanly, I get a bunch of unresolved symbols starting with "pango_fc_" when it tries to link libgfx_gtk.so. The patch is the same as for the trunk, slighly altered to apply and including the patch for building static.

	Walter Meinl Assignee
	Comment 10 • 18 years ago

Comment on attachment 226140 [details] [diff] [review]
both patches combined for the 1.8.1 BRANCH

Wolfgang, I've chosen you for review, as your patch from bug305185 doesn't work anymore for gtk2 2.8. The patch is similar to suggested patches in bug325720 and bug323671.

	Wolfgang Rosenauer [:wolfiR]
	Comment 11 • 18 years ago

Comment on attachment 226140 [details] [diff] [review]
both patches combined for the 1.8.1 BRANCH

>-    PKG_CHECK_MODULES(MOZ_PANGO, pango >= 1.6.0 pangoft2 >= 1.6.0)
>+    PKG_CHECK_MODULES(MOZ_PANGO, pango >= 1.6.0 pangoft2 >= 1.6.0 pangoxft >= 1.6.0)

I wonder if pango and pangoft2 is needed at all?

	Walter Meinl Assignee
	Comment 12 • 18 years ago

(In reply to comment #11)
> 
> I wonder if pango and pangoft2 is needed at all?
> 

-    PKG_CHECK_MODULES(MOZ_PANGO, pango >= 1.6.0 pangoft2 >= 1.6.0)
+    PKG_CHECK_MODULES(MOZ_PANGO, pangoxft >= 1.6.0)

You're right, we don't need to check for those I just built it successfully with the lines altered above. Output from config.status:
s%@MOZ_PANGO_LIBS@%  -lpangoxft-1.0 -lpangoft2-1.0 -lpango-1.0 -lgobject-2.0 -lgmodule-2.0 -ldl -lglib-2.0  %g

	Wolfgang Rosenauer [:wolfiR]
	Comment 14 • 18 years ago

Comment on attachment 226382 [details] [diff] [review]
updated branch patch as per Wolfgangs comment

Sorry that I haven't seen it last time but I have another issue with that.

> if test "$MOZ_ENABLE_PANGO"
> then
>     AC_DEFINE(MOZ_ENABLE_PANGO)
>-    PKG_CHECK_MODULES(MOZ_PANGO, pango >= 1.6.0 pangoft2 >= 1.6.0)
>+    PKG_CHECK_MODULES(MOZ_PANGO, pangoxft >= 1.6.0)
> 
>     AC_SUBST(MOZ_ENABLE_PANGO)
>     AC_SUBST(MOZ_PANGO_CFLAGS)
>     AC_SUBST(MOZ_PANGO_LIBS)
> fi

You wrote in a comment that gtk2 implies pango. That's partly true.
gtk2 really needs pangoxft libs but it doesn't automatically mean
that we build with --enable-pango
So your changes have only effect if you build with --enable-pango
it seems. I guess it doesn't work without it.
 
So I'd propose to modify the MOZ_ENABLE_XFT case as well similar to
the patch in bug 305185:

-    PKG_CHECK_MODULES(_PANGOCHK, pango >= 1.1.0)
+    PKG_CHECK_MODULES(MOZ_PANGO, pangoxft >= 1.1.0 pangox)

+AC_SUBST(MOZ_PANGO_CFLAGS)
+AC_SUBST(MOZ_PANGO_LIBS)

	Walter Meinl Assignee
	Comment 15 • 18 years ago

(In reply to comment #14)
> So your changes have only effect if you build with --enable-pango
> it seems. I guess it doesn't work without it.
For curiosity and in order to get error messages that could be helpful, I built bonecho checked out today, with --disable-pango. I thought that wouldn't work. I did not alter the patch. To my surprise, it built well and I got a running browser. I was even more surprised, that it built and run perfectly when I even did not apply any patch. I'd thought it was impossible to build gtk2 w/o pango.
So, I'm not sure, what to do with the configure.in part of the patch.
However, your comment opened my eyes that it is not enough to add the $MOZ_PANGO_LIBS in the ifdef MOZ_ENABLE_GTK2 part in gfx/src/gtk/Makefile.in. The correct way would probably be to do
+ifdef MOZ_ENABLE_PANGO
+STATIC_EXTRA_LIBS	+= $(MOZ_PANGO_LIBS)
+endif
+
in an extra section (like for config/static-config.mk

	Walter Meinl Assignee
	Comment 16 • 18 years ago

we can build also the trunk with --disable-pango when we use --enable-default-toolkit=gtk2 (with cairo-gtk2 pango is mandatory). Therefore I use ifdef MOZ_ENABLE_PANGO in gfx/src/gtk/Makefile.in

	Walter Meinl Assignee
	Comment 17 • 18 years ago

Wolfgang, see comment #15 why I did not yet include the additional changes for xft you requested in comment #14. In addition to those findings I was also able to build bonecho with --enable-static w/o any of the patches after disabling pango. Yet the build runs w/o any problems, when pango is disabled. So, please comment, if you still see the need for the additional xft changes you requested. It's no problem for me to merge them in, if they are necessary

	Wolfgang Rosenauer [:wolfiR]
	Comment 18 • 18 years ago

Comment on attachment 227072 [details] [diff] [review]
branch patch links against pango-libs only if --enable-pango is used

agreed, the disable-pango case is not needed since we only use pango and pangox (no pangoxft) then.

As my review is not sufficient for build config changes you should ask bsmedberg for addl. review.
The change only affects pango builds and doesn't pull in pangocairo for that.

	Wolfgang Rosenauer [:wolfiR]
	Comment 19 • 18 years ago

*** Bug 305185 has been marked as a duplicate of this bug. ***

	Walter Meinl Assignee
	Comment 20 • 18 years ago

Comment on attachment 227072 [details] [diff] [review]
branch patch links against pango-libs only if --enable-pango is used

Benjamin, as per Wolfgang's comment, I'll ask you for addl. review

	Walter Meinl Assignee
	Comment 21 • 18 years ago

Comment on attachment 227071 [details] [diff] [review]
patch for the trunk v2 linking against pango-libs separated

The according patch for the trunk, if anybody still wants to build with --enable-default-toolkit=gtk2
Please advice, if rev+ how to go on (I cant check in anything)

	Walter Meinl Assignee
	Comment 22 • 18 years ago

just noticed that xulrunner on the branch needs an extra patch in toolkit/library/Makefile.in Debian devs came to the same conclusion

	Walter Meinl Assignee
	Comment 23 • 18 years ago

For the trunk there's again a slightly different situation. For cairo-gtk2 MOZ_PANGO_LIBS are pulled in toolkit/library/libxul-rules.mk a file that doesn't exist in the branch. So we can build xulrunner for the cairo-gtk2 case but it also fails in the gtk2 case for gtk 2.8. I'd suggest to do a similar patch for the trunk as for the branch but to put it in libxul-rules.mk

	Walter Meinl Assignee
	Comment 24 • 18 years ago

According to his blog, Benjamin seems to be in the process of moving to a new house, probably therefore he's not able at the moment to review these patches. Anybody here who can recommend another reviewer for build changes?
For the sake of the distris relying on gtk+-2.8 we should get those trivial patches on the branch before ff-2.0 final is released

	Benjamin Smedberg
	Comment 25 • 18 years ago

Comment on attachment 227071 [details] [diff] [review]
patch for the trunk v2 linking against pango-libs separated

Wolfgang, can you sanity-check this for me? I don't really understand the difference between pango and pangoxft and what this all means.

	Benjamin Smedberg
	Comment 26 • 18 years ago

Comment on attachment 227072 [details] [diff] [review]
branch patch links against pango-libs only if --enable-pango is used

Why did we have pango/pangoft2 originally? Is that needed (why not)?

	Wolfgang Rosenauer [:wolfiR]
	Comment 27 • 18 years ago

Comment on attachment 227071 [details] [diff] [review]
patch for the trunk v2 linking against pango-libs separated

What we really need for pango is pangoxft (mozilla-decoder.cpp).

pangoxft itself requires pango and pangoft2 but this is only an internal requirement which is guaranteed through pangoxft's pkgconfig configuration.

	Walter Meinl Assignee
	Comment 28 • 18 years ago

Wolfgang, Benjamin, is there any sr needed for the changes in gfx/src/gtk in the trunk patch? If not could the trunk patches get checked in https://bugzilla.mozilla.org/attachment.cgi?id=227071 and for xulrunner https://bugzilla.mozilla.org/attachment.cgi?id=228059 ? I guess we would need those on the trunk before the branch patches may be approved, thanks

	Wolfgang Rosenauer [:wolfiR]
	Comment 29 • 18 years ago

For build config Ben's review are usually sufficient for trunk.
I'll submit the changes soon when the tree is open after the colo move.

	Wolfgang Rosenauer [:wolfiR]
	Comment 30 • 18 years ago

checked in on trunk:
Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1699; previous revision: 1.1698
done
Checking in gfx/src/gtk/Makefile.in;
/cvsroot/mozilla/gfx/src/gtk/Makefile.in,v  <--  Makefile.in
new revision: 1.127; previous revision: 1.126
done
Checking in toolkit/library/libxul-rules.mk;
/cvsroot/mozilla/toolkit/library/libxul-rules.mk,v  <--  libxul-rules.mk
new revision: 1.13; previous revision: 1.12
done

	Walter Meinl Assignee
	Comment 31 • 18 years ago

Comment on attachment 227072 [details] [diff] [review]
branch patch links against pango-libs only if --enable-pango is used

The according trunk checkins didn't affect any linux tinderbox, due to ifdefs this patch is only effective for the enable-pango case. It's low risk and would be of great help for distris relying on gtk+-2.8.

	Walter Meinl Assignee
	Comment 32 • 18 years ago

Comment on attachment 228058 [details] [diff] [review]
addl. patch for the branch to build xulrunner

also the XULrunner tinderbox stayed green after checkin of the according patch for the trunk

	David Baron :dbaron:
	Comment 33 • 18 years ago

Comment on attachment 227072 [details] [diff] [review]
branch patch links against pango-libs only if --enable-pango is used

a=dbaron on behalf of drivers.  Please check in to the MOZILLA_1_8_BRANCH and mark fixed1.8.1 when you have.

	David Baron :dbaron:
	Comment 34 • 18 years ago

Comment on attachment 228058 [details] [diff] [review]
addl. patch for the branch to build xulrunner

a=dbaron on behalf of drivers.  Please check in to the MOZILLA_1_8_BRANCH and mark fixed1.8.1 when you have.

	Walter Meinl Assignee
	Comment 35 • 18 years ago

Wolfgang, may I ask you again for your help, thanks.
One remark When you checked in the branch patch https://bugzilla.mozilla.org/attachment.cgi?id=216683 for bug332170 it seemed to happen that in the line
+STATIC_EXTRA_LIBS	+= $(MOZ_XFT_LIBS)
the distance to the "+=" signs was made of 5 spaces instead of a tab delimiter. The patch here against config/static-config.mk takes care of this. However, if you want to change/correct the whitespace this would be an opportunity.
And a further question, should we go these patches also for the 1_8_0_Branch?

	Walter Meinl Assignee
	Comment 36 • 18 years ago

With regard to https://bugzilla.mozilla.org/attachment.cgi?id=222532 that was checked in a while ago bug312951 is a duplicate of this bug here. I tried to mark bug312951, hadn't the rights to do so though.

	Wolfgang Rosenauer [:wolfiR]
	Comment 37 • 18 years ago

*** Bug 312951 has been marked as a duplicate of this bug. ***

	Wolfgang Rosenauer [:wolfiR]
	Comment 38 • 18 years ago

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1503.2.87; previous revision: 1.1503.2.86
done
Checking in config/static-config.mk;
/cvsroot/mozilla/config/static-config.mk,v  <--  static-config.mk
new revision: 3.23.2.5; previous revision: 3.23.2.4
done
Checking in gfx/src/gtk/Makefile.in;
/cvsroot/mozilla/gfx/src/gtk/Makefile.in,v  <--  Makefile.in
new revision: 1.121.8.2; previous revision: 1.121.8.1
done
Checking in toolkit/library/Makefile.in;
/cvsroot/mozilla/toolkit/library/Makefile.in,v  <--  Makefile.in
new revision: 1.16.2.6; previous revision: 1.16.2.5
done

and therefore finally fixed

	Wolfgang Rosenauer [:wolfiR]
	Comment 39 • 18 years ago

(In reply to comment #35)
> And a further question, should we go these patches also for the 1_8_0_Branch?

Personally I don't care much about 1.8.0 branch in this case since it's in "maintenance mode" only.

	Walter Meinl Assignee
	Comment 40 • 18 years ago

(In reply to comment #38)
> Checking in config/static-config.mk;
> /cvsroot/mozilla/config/static-config.mk,v  <--  static-config.mk
> new revision: 3.23.2.5; previous revision: 3.23.2.4
> done
Wolfgang, oops, the whitespace for the xft case is now fixed, but the pango fix is missing from that file after checkout
+ifdef MOZ_ENABLE_PANGO
+STATIC_EXTRA_LIBS	+= $(MOZ_PANGO_LIBS)
+endif
+

	Wolfgang Rosenauer [:wolfiR]
	Comment 41 • 18 years ago

sorry, I've missed it somehow.
Thanks for the heads up and -> fixed now

	Walter Meinl Assignee
	Comment 42 • 18 years ago

After checkin for bug349906 in gtk2 trunk builds pangoxft isn't a dependency anymore. I greped through the tree only thebes related files still depend on pangoxft. Thus, it makes no sense anymore to check in configure for the gtk2 case for the presence of pangoxft. We could revert the configure.in changes made in
https://bugzilla.mozilla.org/attachment.cgi?id=227071
to look for instance as they were before
     AC_DEFINE(MOZ_ENABLE_PANGO)
-    PKG_CHECK_MODULES(MOZ_PANGO, pangoxft >= 1.6.0)
+    PKG_CHECK_MODULES(MOZ_PANGO, pango >= 1.6.0 pangoft2 >= 1.6.0) 
     AC_SUBST(MOZ_ENABLE_PANGO)
However, Wolfgang you said in Comment#11 that you wonder if if pango and pangoft2 is needed at all. If we don't need pangoxft anymore and maybe also not pango and pangoft2, what should we check for in the pango-enabled case? Shall we do anything, it works as it is now, but it's no clean situation anymore.