Last Comment Bug 259033 - Qt build patch
: Qt build patch
Status: VERIFIED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: Ports: Qt (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: timeless
: Christian :Biesinger (don't email me, ping me on IRC)
:
Mentors:
Depends on: 259035 259036 259037 259038
Blocks:
  Show dependency treegraph
 
Reported: 2004-09-12 17:02 PDT by Zack Rusin
Modified: 2008-07-31 01:11 PDT (History)
13 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Qt Mozilla build patch (228.40 KB, patch)
2004-09-12 17:03 PDT, Zack Rusin
benjamin: review+
jst: superreview+
Details | Diff | Splinter Review
Qt Mozilla build patch 2 (11.54 KB, patch)
2004-09-14 08:16 PDT, Zack Rusin
no flags Details | Diff | Splinter Review
Qt build patch (11.49 KB, patch)
2004-09-17 18:36 PDT, Zack Rusin
no flags Details | Diff | Splinter Review
build patch 4 (11.46 KB, patch)
2004-09-18 20:28 PDT, Zack Rusin
no flags Details | Diff | Splinter Review

Description Zack Rusin 2004-09-12 17:02:46 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.7) Gecko/20040823 Firefox/0.9.3

In this bug I'll attach the build system patch that incorporates the changes
needed to build the Qt port.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Comment 1 Zack Rusin 2004-09-12 17:03:54 PDT
Created attachment 158684 [details] [diff] [review]
Qt Mozilla build patch
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-13 05:08:27 PDT
Comment on attachment 158684 [details] [diff] [review]
Qt Mozilla build patch

 # Add explicit X11 dependency when building against X11 toolkits
-ifneq (,$(filter gtk gtk2 xlib,$(MOZ_WIDGET_TOOLKIT)))
+ifneq (,$(filter gtk gtk2 qt xlib,$(MOZ_WIDGET_TOOLKIT)))
 LIBS += $(XLDFLAGS) $(XLIBS)

we suck... there's this nice MOZ_X11 subst in configure.in, although
autoconf.mk.in doesn't use it...

mozilla patches don't need to contain configure diffs, it is autogenerated upon
checkin


note to self: widget patch is in Bug 259036. Are the gfx changes available
anywhere?
Comment 3 Benjamin Smedberg [:bsmedberg] 2004-09-13 05:41:41 PDT
Comment on attachment 158684 [details] [diff] [review]
Qt Mozilla build patch

>Index: configure.in

>+	dnl The following warning breaks Qt
>+	if test ! "$QTDIR"; then
>+    	_WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS}"
>+	fi

What's this about? Did you forget something?

>+qt)
>+	MOZ_ENABLE_QT=1
>+    MOZ_ENABLE_XREMOTE=1
>+	TK_CFLAGS='$(MOZ_QT_CFLAGS)'
>+	TK_LIBS='$(MOZ_QT_LDFLAGS)'
>+	AC_DEFINE(MOZ_WIDGET_QT)

Whitespace nit: follow existing conventions (use spaces here, not tabs)

>+dnl    AM_PATH_QT($QT_VERSION,,
>+dnl      AC_MSG_ERROR(Test for QT failed.))

What's up? We don't generally check in code which is commented-out.

>-if test "$MOZ_ENABLE_GTK" \
>+if test "$MOZ_ENABLE_GTK" || test "$MOZ_ENABLE_QT" \
> || test "$MOZ_ENABLE_XLIB" \
> || test "$MOZ_ENABLE_GTK2"

grr, please be consistent 

>Index: embedding/browser/Makefile.in

>-DIRS=webBrowser build chrome
>+DIRS=webBrowser build chrome qt

Do we build this dir even if qt is not the toolkit in use?

>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+#ifdef MOZ_WIDGET_QT
>+#undef CursorShape /*X.h defines it as 0,
>+                     qnamespace.h makes an enum type by that name
>+                   */
>+#endif

I would prefer "#ifdef CursorShape instead of #ifdef MOZ_WIDGET_QT

>Index: widget/src/xpwidgets/Makefile.in

> 		  uconv \
> 		  unicharutil \
>+		docshell view intl \
> 		  $(NULL)

follow existing convention, one dir per line, with consistent
tab-tab-space-space indenting

Fix those nits and attach a new patch. I also want jst to review the changes to
the plugin code.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2004-09-13 07:17:55 PDT
Comment on attachment 158684 [details] [diff] [review]
Qt Mozilla build patch

sr=jst with bsmedbergs nits fixed.
Comment 5 Zack Rusin 2004-09-13 12:15:36 PDT
Actually this baby is essentially what you used to have in the tree for the 
old Qt port. I assumed that one was already reviewed and ok. All yours truly 
did was boost the Qt requirement and add the embedding dir (btw, yeah, that 
one needs to be in ifdefs as well).  
 
I'll update it after work and will send an update tomorrow morning. And to 
answer the question the full set of patches for the Qt port is in: 259035 
(GFX), 259036 (widget), 259037 (webshell) and 259038 (embedding). I guess it'd 
be nice to have a top level bug depending on those to follow the general 
progress of the merge but without internet connection at home I can't really 
do it at the moment. 
Comment 6 Zack Rusin 2004-09-14 08:12:36 PDT
(In reply to comment #3) 
> (From update of attachment 158684 [details] [diff] [review]) 
> >Index: configure.in 
>  
> >+      dnl The following warning breaks Qt 
> >+      if test ! "$QTDIR"; then 
> >+      _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS}" 
> >+      fi 
>  
> What's this about? Did you forget something? 
 
It was in the original patch. I removed it. 
 
> >+qt) 
> >+      MOZ_ENABLE_QT=1 
> >+    MOZ_ENABLE_XREMOTE=1 
> >+      TK_CFLAGS='$(MOZ_QT_CFLAGS)' 
> >+      TK_LIBS='$(MOZ_QT_LDFLAGS)' 
> >+      AC_DEFINE(MOZ_WIDGET_QT) 
>  
> Whitespace nit: follow existing conventions (use spaces here, not tabs) 
 
That's also something that has been orginally in your tree. BTW, you don't 
really have a convention there, xlib, photon, cairo-xlib use tabs - others 
spaces. I switched Qt to spaces, but it would make sense to make it all 
consistent if the spaces are a convention. 
 
> >+dnl    AM_PATH_QT($QT_VERSION,, 
> >+dnl      AC_MSG_ERROR(Test for QT failed.)) 
>  
> What's up? We don't generally check in code which is commented-out. 
 
Also something that was orginally in your tree. Fixed that. 
 
> >-if test "$MOZ_ENABLE_GTK" \ 
> >+if test "$MOZ_ENABLE_GTK" || test "$MOZ_ENABLE_QT" \ 
> > || test "$MOZ_ENABLE_XLIB" \ 
> > || test "$MOZ_ENABLE_GTK2" 
>  
> grr, please be consistent  
 
Also not me. Fixed :) 
 
> >Index: embedding/browser/Makefile.in 
>  
> >-DIRS=webBrowser build chrome 
> >+DIRS=webBrowser build chrome qt 
>  
> Do we build this dir even if qt is not the toolkit in use? 
 
Yap, that was wrong. I just added it for us and forgot to remove it. 
 
> >Index: modules/plugin/base/src/nsPluginHostImpl.cpp 
>  
> >+#ifdef MOZ_WIDGET_QT 
> >+#undef CursorShape /*X.h defines it as 0, 
> >+                     qnamespace.h makes an enum type by that name 
> >+                   */ 
> >+#endif 
>  
> I would prefer "#ifdef CursorShape instead of #ifdef MOZ_WIDGET_QT 
 
Makes sense. Moved it. 
 
> >Index: widget/src/xpwidgets/Makefile.in 
>  
> >                 uconv \ 
> >                 unicharutil \ 
> >+              docshell view intl \ 
> >                 $(NULL) 
>  
> follow existing convention, one dir per line, with consistent 
> tab-tab-space-space indenting 
 
That wasn't really a problem. This part was just plain wrong. I did it because 
we had some problems regenerating the Makefile because of some funky things I 
did and since I didn't feel like messing with it at the time I just made the 
whole thing link against it. Fixed it. 
Thanks for taking a look at it. 
Comment 7 Zack Rusin 2004-09-14 08:16:00 PDT
Created attachment 158877 [details] [diff] [review]
Qt Mozilla build patch 2
Comment 8 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-15 08:58:32 PDT
hm...
aclocal: configure.in: 3343: macro `AM_PATH_QT' not found in library
Comment 9 Zack Rusin 2004-09-15 11:05:18 PDT
(In reply to comment #8) 
> aclocal: configure.in: 3343: macro `AM_PATH_QT' not found in library 
Oh, yeah, the patch should of course not have the : 
+    AM_PATH_QT($QT_VERSION,, 
+      AC_MSG_ERROR(Test for Qt failed.)) 
lines. Sorry about that. 
 
 
 
Comment 10 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-15 14:57:46 PDT
in allmakefiles.sh:
widget/src/qt

that should be widget/src/qt/Makefile

otherwise you'll have a file called qt in the objdir, which causes the build to
fail...
Comment 11 Zack Rusin 2004-09-17 18:36:41 PDT
Created attachment 159279 [details] [diff] [review]
Qt build patch

Fixes the allmakefiles issue plus updates to cleanly apply with HEAD
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-18 07:40:49 PDT
Comment on attachment 159279 [details] [diff] [review]
Qt build patch

+    MOZ_ENABLE_XREMOTE=1

so does qt actually support xremote? because if it doesn't, this will do bad
things, the user-visible one being that bookmarks aren't loaded in browser
windows (well, due to sucky error handling, but still)
Comment 13 Zack Rusin 2004-09-18 20:28:24 PDT
Created attachment 159370 [details] [diff] [review]
build patch 4

No, it doesn't I was entertaining the thought of supporting it and didn't think
that adding something in will cause such problems.
Comment 14 Zack Rusin 2004-09-26 11:15:38 PDT
I just updated gfx, widget and embedding patches. Could someone please either
let me know whether there's anything wrong with them or commit them?
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-28 09:25:19 PDT
so, it seems that currently embedding/browser/qt is never built. shouldn't it be
built, similar to embedding/browser/gtk, by mozilla/Makefile.in? (maybe it
shouldn't, I don't know :) )

Comment 16 Zack Rusin 2004-09-28 09:51:31 PDT
I disabled it for a bit. Now it's actually working fine so we can enable it by
default. I'm not 100% committed to this api yet so I don't want to have people
jumping on it yet. I can enable it though if that's easier.
Comment 17 Zack Rusin 2004-10-10 21:19:48 PDT
Qt Mozilla is now in CVS.
Comment 18 Stefan Borggraefe 2004-10-11 00:01:49 PDT
I just tried to build a Qt Seamonkey and it seems the file qlist.h is missing.
Or am I doing something wrong? I used the following .mozconfig:

ac_add_options --enable-freetype2
ac_add_options --enable-crypto
ac_add_options
--enable-extensions=wallet,irc,help,inspector,typeaheadfind,xml-rpc,xmlextras,venkman,cookie,content-packs,transformiix,universalchardet,webservices,spellcheck,gnomevfs,negotiateauth,sroaming
ac_add_options --enable-svg
ac_add_options --enable-svg-renderer-libart
ac_add_options --disable-installer
ac_add_options --enable-debug
ac_add_options --disable-optimize
mk_add_options MOZ_INTERNAL_LIBART_LGPL=1
export MOZ_INTERNAL_LIBART_LGPL=1
ac_add_options --enable-default-toolkit=qt
ac_add_options --with-qt
ac_add_options --with-qtdir=/usr/share/qt3
ac_add_options --enable-chrome-format=symlink
Comment 19 Zack Rusin 2004-10-11 06:44:02 PDT
qlist.h is a deprecated header which I simply forgot to removed and apperently 
some distros stopped including it. Either way I just fixed it. 
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-15 08:26:07 PDT
vrfy fixed. qt is now in the tree.

Note You need to log in before you can comment on or make changes to this bug.