Bug 52839 – [Qt][Symbian] Fix --minimal build

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Siddharth Mathur]

"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.

[Yael]

Created attachment 79856 [details]
Patch

Build fix for the plugins issue.

[Yael]

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.

[WebKit Commit Bot]

Comment on attachment 79856 [details]
Patch

Clearing flags on attachment: 79856

Committed r76466: <http://trac.webkit.org/changeset/76466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

Only the plugins part was fixed. We still have the USE_SYSTEM_MALLOC issue.
Reopening the bug.

[Siddharth Mathur]

/me Yael is awesome!

[Yael]

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.

[Laszlo Gombos]

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.

[Yael]

(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.

[Eric Seidel (no email)]

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?

[Laszlo Gombos]

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.

[Yael]

(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?

[Laszlo Gombos]

> > 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.

[Yael]

(In reply to comment #8)
> 
> Separate patch - this might qualify as a buildfix.

Fixed in http://trac.webkit.org/changeset/76606.

[Yael]

Created attachment 80081 [details]
Patch

Move definition of USE_SYSTEM_MALLOC out of pri file and put it in platform.h instead.

[Laszlo Gombos]

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.

[Yael]

(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.

[Yael]

Created attachment 80123 [details]
Patch

Address comment #16.

[Laszlo Gombos]

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.

[Yael]

(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.

[Yael]

Created attachment 80190 [details]
Patch

Address comment #19

[Laszlo Gombos]

Comment on attachment 80190 [details]
Patch

LGTM, r+. Thanks.

[WebKit Commit Bot]

Comment on attachment 80190 [details]
Patch

Clearing flags on attachment: 80190

Committed r76716: <http://trac.webkit.org/changeset/76716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76716 might have broken Leopard Intel Release (Tests)