Bug 52839 - [Qt][Symbian] Fix --minimal build
Summary: [Qt][Symbian] Fix --minimal build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 41464
  Show dependency treegraph
 
Reported: 2011-01-20 13:27 PST by Siddharth Mathur
Modified: 2011-01-26 15:05 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.36 KB, patch)
2011-01-22 13:45 PST, Yael
no flags Details | Formatted Diff | Diff
Patch (1.16 KB, patch)
2011-01-24 08:43 PST, Yael
eric: review-
Details | Formatted Diff | Diff
Patch (3.08 KB, patch)
2011-01-25 10:42 PST, Yael
laszlo.gombos: review-
Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2011-01-25 14:32 PST, Yael
laszlo.gombos: review-
Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2011-01-26 06:55 PST, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Siddharth Mathur 2011-01-20 13:27:34 PST
"build-webkit --qt --release --symbian --minimal" has 2-3 small issues for QtWebkit.dll which will be good to fix quickly. Upside is that ARM UDEB builds are then much smaller and rebuilding is quite fast (even on a measly 4 core machine)!

- linker error with unresolved PluginPackage methods (those in PluginPackageSymbian.cpp) 
- linker error with unresolved PluginPackage::unload()
- USE_SYSTEM_MALLOC is set to 0 via qmake command-line args (due to --minimal it seems) , but is set to 1 explictly for symbian in all our .pro/.pri . This causes the autogenerated .mmp files to contain _both_ flags set. Leads to warnings at a minimum from compiler.
Comment 1 Yael 2011-01-22 13:45:52 PST
Created attachment 79856 [details]
Patch

Build fix for the plugins issue.
Comment 2 Yael 2011-01-22 15:10:12 PST
Siddharth, USE_SYSTEM_MALLOC is turned off by default. You have to turn it on by passing --system-malloc to build-webkit.
Based on your comment it seems we always want this to be on for Symbian builds, so I will upload a patch to do that.
Comment 3 WebKit Commit Bot 2011-01-23 11:28:49 PST
Comment on attachment 79856 [details]
Patch

Clearing flags on attachment: 79856

Committed r76466: <http://trac.webkit.org/changeset/76466>
Comment 4 WebKit Commit Bot 2011-01-23 11:28:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Yael 2011-01-24 07:17:04 PST
Only the plugins part was fixed. We still have the USE_SYSTEM_MALLOC issue.
Reopening the bug.
Comment 6 Siddharth Mathur 2011-01-24 08:20:00 PST
/me Yael is awesome!
Comment 7 Yael 2011-01-24 08:43:50 PST
Created attachment 79927 [details]
Patch

USE_SYSTEM_MALLOC is controlled by webkit.pri for Symbian and should never be turned off.
When building the qmake paramaters list for Symbian, do not add it to the list.
Comment 8 Laszlo Gombos 2011-01-24 09:46:11 PST
It would be good to avoid the special treatment for Symbian in build-webkit (and in webkitdirs.pm) especially that that the problem we're trying to solve is not specific to Symbian (e.g. I suspect that Windows minimal build would have the same problem).

I suggest to move setting USE_SYSTEM_MALLOC out from the Qt-specific make-system (WebKit.pri) and set it instead in common source-code (Platform.h like e.g. Android). By doing so build-webkit --minimal will no longer set USE_SYSTEM_MALLOC to 0. This would require adding a guard to TCSystemAlloc.cpp and including it for every Qt build.
Comment 9 Yael 2011-01-24 10:50:42 PST
(In reply to comment #8)
> It would be good to avoid the special treatment for Symbian in build-webkit (and in webkitdirs.pm) especially that that the problem we're trying to solve is not specific to Symbian (e.g. I suspect that Windows minimal build would have the same problem).
> 
> I suggest to move setting USE_SYSTEM_MALLOC out from the Qt-specific make-system (WebKit.pri) and set it instead in common source-code (Platform.h like e.g. Android). By doing so build-webkit --minimal will no longer set USE_SYSTEM_MALLOC to 0. This would require adding a guard to TCSystemAlloc.cpp and including it for every Qt build.

Sorry, I did not realize that we check webkit.pri to figure out the defaults :(
I'll update the patch as you suggested.
Comment 10 Eric Seidel (no email) 2011-01-24 13:14:23 PST
Comment on attachment 79927 [details]
Patch

?  You're grepping the passed build flags?  This doesn't make sense.

If you pass FOO= before a command, it gets set in the environment.  If you pass FOO= after build-webkit, it's supposed to treat all arguments it doesnt' understand as arguments to be passed to the underlying build system.  I assume that's what you're tryign to do here?  But if so, why special case USE_SYSTEM_MALLOC.  Are we sure thsi is the right fix?
Comment 11 Laszlo Gombos 2011-01-24 13:33:22 PST
I seem to have a problem with the previously landed patch as linking still fails:

   Error: L6218E: Undefined symbol RFs::DriveToChar(int, TChar&) (referred from
PluginDatabaseSymbian.o).
   Error: L6218E: Undefined symbol RFs::Close() (referred from PluginDatabaseSym
bian.o).
   Error: L6218E: Undefined symbol RFs::Connect(int) (referred from PluginDataba
seSymbian.o).
   Error: L6218E: Undefined symbol RFs::DriveList(TBuf8<(int)26>&) const (referr
ed from PluginDatabaseSymbian.o).

It seem that "LIBS += -lefsrv" should be outside of the ENABLE_NETSCAPE_PLUGIN_API guard for Symbian.
Comment 12 Yael 2011-01-24 13:52:56 PST
(In reply to comment #11)
> I seem to have a problem with the previously landed patch as linking still fails:
> 
>    Error: L6218E: Undefined symbol RFs::DriveToChar(int, TChar&) (referred from
> PluginDatabaseSymbian.o).
>    Error: L6218E: Undefined symbol RFs::Close() (referred from PluginDatabaseSym
> bian.o).
>    Error: L6218E: Undefined symbol RFs::Connect(int) (referred from PluginDataba
> seSymbian.o).
>    Error: L6218E: Undefined symbol RFs::DriveList(TBuf8<(int)26>&) const (referr
> ed from PluginDatabaseSymbian.o).
> 
> It seem that "LIBS += -lefsrv" should be outside of the ENABLE_NETSCAPE_PLUGIN_API guard for Symbian.

You are probably right. Should we fix all the issues in one bug or create one per patch?
Comment 13 Laszlo Gombos 2011-01-24 14:56:06 PST
> > It seem that "LIBS += -lefsrv" should be outside of the ENABLE_NETSCAPE_PLUGIN_API guard for Symbian.
> 
> You are probably right. Should we fix all the issues in one bug or create one per patch?

Separate patch - this might qualify as a buildfix.
Comment 14 Yael 2011-01-25 10:22:25 PST
(In reply to comment #8)
> 
> Separate patch - this might qualify as a buildfix.

Fixed in http://trac.webkit.org/changeset/76606.
Comment 15 Yael 2011-01-25 10:42:39 PST
Created attachment 80081 [details]
Patch

Move definition of USE_SYSTEM_MALLOC out of pri file and put it in platform.h instead.
Comment 16 Laszlo Gombos 2011-01-25 13:29:03 PST
Comment on attachment 80081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80081&action=review

r- to fix the Windows build.

> Source/JavaScriptCore/wtf/Platform.h:786
> +

This would only set USE_SYSTEM_MALLOC for the Symbian port and would break for example the Windows port of QtWebKit. I suggest we repeat the original rule from WebKit.pri. 

We already have a section for setting memory management related defaults for the Qt port, so perhaps we can add this rule there.

 #if PLATFORM(QT)
 /* We must not customize the global operator new and delete for the Qt port. */
 #define ENABLE_GLOBAL_FASTMALLOC_NEW 0
+#if !PLATFORM(UNIX) || OS(SYMBIAN)
+#define USE_SYSTEM_MALLOC 1
+#endif
 #endif

> Source/JavaScriptCore/wtf/TCSystemAlloc.cpp:34
> +#if !(defined(USE_SYSTEM_MALLOC) && USE_SYSTEM_MALLOC)

Can we just use !USE(SYSTEM_MALLOC) instead ?

> Source/JavaScriptCore/wtf/wtf.pri:45
> +SOURCES += wtf/TCSystemAlloc.cpp

I think we should move this entry together with the rest of the SOURCES list - alphabetically ordered.
Comment 17 Yael 2011-01-25 13:51:19 PST
(In reply to comment #16)
> (From update of attachment 80081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80081&action=review
> 
> r- to fix the Windows build.
> 
Thank you for your review, and the offline explanation of how to fix the windows build.
As you pointed out, we cannot use !USE(SYSTEM_MALLOC) .
I will upload a new patch soon, addressing all the other issues.
Comment 18 Yael 2011-01-25 14:32:33 PST
Created attachment 80123 [details]
Patch

Address comment #16.
Comment 19 Laszlo Gombos 2011-01-26 06:03:48 PST
Comment on attachment 80123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80123&action=review

r- to fix the guard, otherwise looks good.

> Source/JavaScriptCore/wtf/Platform.h:834
> +#if !(PLATFORM(UNIX) || OS(SYMBIAN))

I think this should be "!PLATFORM(UNIX) || OS(SYMBIAN)" - the test above is false for Symbian.
Comment 20 Yael 2011-01-26 06:39:51 PST
(In reply to comment #19)
> (From update of attachment 80123 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80123&action=review
> 
> r- to fix the guard, otherwise looks good.
> 
> > Source/JavaScriptCore/wtf/Platform.h:834
> > +#if !(PLATFORM(UNIX) || OS(SYMBIAN))
> 
> I think this should be "!PLATFORM(UNIX) || OS(SYMBIAN)" - the test above is false for Symbian.

Good catch :)
A bad last minute change I did.
Comment 21 Yael 2011-01-26 06:55:42 PST
Created attachment 80190 [details]
Patch

Address comment #19
Comment 22 Laszlo Gombos 2011-01-26 13:30:58 PST
Comment on attachment 80190 [details]
Patch

LGTM, r+. Thanks.
Comment 23 WebKit Commit Bot 2011-01-26 13:59:29 PST
Comment on attachment 80190 [details]
Patch

Clearing flags on attachment: 80190

Committed r76716: <http://trac.webkit.org/changeset/76716>
Comment 24 WebKit Commit Bot 2011-01-26 13:59:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2011-01-26 15:05:03 PST
http://trac.webkit.org/changeset/76716 might have broken Leopard Intel Release (Tests)