Bug 18428 – Patch to support building wxWebKit on mingw

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit wx

[Alexander Vassilev]

I have ported the wxWebKit build system to build wxwebkit with the mingw compiler and the gnu toolchain (using cygwin).Since I cannot attach files to bug reports here, I have sent the archive withthe patches to Kevin Olliver. HTe files are as follows:

 - wxwebkitmingw.patch - patch to the webkit source tree that modified the bakefile build system of wxwebkit and some source files of webkit to compile with mingw. The patch is made agains a checkout on 11 april 2008 around 13:30 UTC

 - bakefile.patch - bakefile nas a small inconsistency in its scripts that does not allow using mingw with gnu tools (bash). More details in the README file

 - README.txt - detailed description of how the patches should be applied and a howto for the mingw build
 - MakeMingwPatch.sh - a script that generates the webkit patch comapring only the files that have to be modified, avoiding diff-ing the whole webkit tree


NOTE: I have done a build test with the current svn but it fails due to problems with  DerivedSources/JavaScriptCore/grammar.cpp. IS svn currently broken?
Compiler output:

g++ -c -o obj-gnu/jscore_grammar.o -DBUILDING_WX__=1 -DUSE_SYSTEM_MALLOC  -O0 -g -Id:/devel/webkit_clean//WebKitLibraries/win/include  -Id:/devel/webkit_clean//WebKitLibraries/win/include -Id:/devel/webkit_clean//WebKitLibraries/win/include/pthreads -DPTW32_STATIC_LIB -I. -I./.. -I./DerivedSources/JavaScriptCore -I./ForwardingHeaders -I./kjs -I./pcre -I./wtf -I./wtf/unicode -DENABLE_XSLT=1 -DHAVE_FUNC_ISNAN -DHAVE_SYS_TIMEB_H=1 -DHAVE_FLOAT_H=1 -DHAVE_FUNC__FINITE=1 -mthreads -mno-cygwin  -fPIC -DPIC  -DBUILDING_WX__=1  -MTobj-gnu/jscore_grammar.o -MF`echo obj-gnu/jscore_grammar.o | sed -e 's,\.o$,.d,'` -MD DerivedSources/JavaScriptCore/grammar.cpp
DerivedSources/JavaScriptCore/grammar.cpp:1: warning: -fPIC ignored for target (all code is position independent)
../../kjs/grammar.y:54: error: `allowAutomaticSemicolon' declared as an `inline' variable
../../kjs/grammar.y:54: error: `Lexer' was not declared in this scope
../../kjs/grammar.y:54: error: expected primary-expression before ',' token
../../kjs/grammar.y:54: error: expected primary-expression before "int"
../../kjs/grammar.y:54: error: initializer expression list treated as compound expression
../../kjs/grammar.y: In function `int kjsyyparse(void*)':
../../kjs/grammar.y:742: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:808: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:850: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:918: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:922: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:927: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:929: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:934: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:936: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:996: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y:1022: error: `allowAutomaticSemicolon' cannot be used as a function
../../kjs/grammar.y: In function `bool allowAutomaticSemicolon(KJS::Lexer&, int)':
../../kjs/grammar.y:1292: error: `bool allowAutomaticSemicolon(KJS::Lexer&, int)' redeclared as different kind of symbol
../../kjs/grammar.y:54: error: previous declaration of `bool allowAutomaticSemicolon'
../../kjs/grammar.y:54: error: previous non-function declaration `bool allowAutomaticSemicolon'
../../kjs/grammar.y:1292: error: conflicts with function declaration `bool allowAutomaticSemicolon(KJS::Lexer&, int)'
make: *** [obj-gnu/jscore_grammar.o] Error 1
make: Leaving directory `/devel/webkit_clean/JavaScriptCore'

[Alexander Vassilev]

Created attachment 20480 [details]
patch to webkit tree

[Alexander Vassilev]

Created attachment 20481 [details]
patch to bakefile script

[Alexander Vassilev]

Created attachment 20482 [details]
readme and howto

[Alexander Vassilev]

Created attachment 20483 [details]
bash script for creating the webkit patch

[Alexander Vassilev]

I just figured out that I can attach files after creating the bug report, so all files are attached now

[Alexander Vassilev]

Created attachment 20485 [details]
patch

Here is an updated patch agains svn revision 31813

[Alp Toker]

I think some of the MSVC ifdefs can just be replaced with a check for WIN_OS rather than involving mingw in the matter.

There are various indentation issues in the patch.

(Not a full review, just some things I spotted.)

[Alexander Vassilev]

Created attachment 20695 [details]
updated patch

As far as the mingw ifdef's around usage of localtime() calls and the like - they are specific just for the mingw compiler, since it does not have the _r functions. Otherwise I have replaced some ifdef's that check both for MSVC or MINGW, with a simple check for WIN_OS platform. Also replaced tabs with spaces in the patch and removed some leftover debugging messages.

[Kevin Ollivier]

Sorry I've just finally had a chance to take a look at this. First comments:

1) Do not replace COMPILER(MSVC) or COMPILER(MSVC7) with PLATFORM(WIN_OS). When PLATFORM(WIN_OS) is not used, it is often because there are Windows ports which should not or can not have that code in them. (e.g. QT and GTK build on Windows as well) Especially so with COMPILER(MSVC7), as that define indicates a fix for a problem that was resolved in MSVC8. We need to add PLATFORM(MINGW) define and use that instead. See JavaScriptCore/wtf/Platform.h for how to define a platform.

2) Please do not use <if cond="(FORMAT=='gnu') and (PLATFORM_WIN32 != '1')"> syntax. The parentheses are unnecessary and even in your code are sometimes used, and sometimes not. It's best to use one consistent style among all the code, and a majority of the code is already formatted as <if cond="FORMAT=='gnu' and PLATFORM_WIN32 != '1'">.

3) Please remove the <echo> statement in the Bakefiles. WX_ROOT not being set is not always a problem, and the build script should give an error when it is.

4) Please do not add static library support in this patch. We have a very serious issue with static library support on almost all platforms because we get symbol conflicts between the wx builtin libpng, which cause WebKit libraries to use the (too old) libpng included with wx at runtime. I'd be happy to see if we can get this resolved for all platforms, but that should be a separate bug and patch so that we can tackle that issue on its own. One step at a time... :)

[Alexander Vassilev]

Yes, I think this is the best way - to use a compiler specific check. I just tried to follow the advice in Comment #7 about the mingw ifdefs.

As for the conditions in the bakefiles, I was just cautious with the bakefile syntax, was as a precaution to be sure the logical operator precedence is as I want it, some sort of defensive programming :) Ok so I will remove the braces

Echo statements removed in the updated patch from Comment #8

About the static version - I think I experienced exactly what you mean, but the conflict happens (at least on windows) when I try to link with a static version of wxWidgets. If I make a static build of wxWebkit that uses a shared version of wxWidgets, there is no problem at all - at link time the linker sees only wxWebkit's libjpeg and libpng, not the builtin in the wxWidgets shared libs. But this is on windows and mingw. If this is not the case with other platforms, then ok, I remove the static build. But I am writing this just to be sure we are meaning the same thing by "static build".

[Kevin Ollivier]

(In reply to comment #10)
> Yes, I think this is the best way - to use a compiler specific check. I just
> tried to follow the advice in Comment #7 about the mingw ifdefs.
> 
> As for the conditions in the bakefiles, I was just cautious with the bakefile
> syntax, was as a precaution to be sure the logical operator precedence is as I
> want it, some sort of defensive programming :) Ok so I will remove the braces
> 
> Echo statements removed in the updated patch from Comment #8
> 
> About the static version - I think I experienced exactly what you mean, but the
> conflict happens (at least on windows) when I try to link with a static version
> of wxWidgets. If I make a static build of wxWebkit that uses a shared version
> of wxWidgets, there is no problem at all - at link time the linker sees only
> wxWebkit's libjpeg and libpng, not the builtin in the wxWidgets shared libs.
> But this is on windows and mingw. If this is not the case with other platforms,
> then ok, I remove the static build. But I am writing this just to be sure we
> are meaning the same thing by "static build".
> 

Right, this is not the case on other platforms, and I didn't want to cause confusion about what combinations can be supported. The issue can be fixed now that gcc has added visibility support in  4.0, but wxWidgets did not add support for building with visibility flags until 2.9, which is not in a good enough state to be used for wxWebKit development yet. :( So to fix this for real we'd have to either backport the 2.9 visibility handling to 2.8 or implement image loading so that it uses wx APIs rather than libpng directly, which may be difficult as I'm not sure wx's image classes are good at handling data being streamed in.

[Alexander Vassilev]

> Right, this is not the case on other platforms, and I didn't want to cause
> confusion about what combinations can be supported. The issue can be fixed now
> that gcc has added visibility support in  4.0, but wxWidgets did not add
> support for building with visibility flags until 2.9, which is not in a good
> enough state to be used for wxWebKit development yet. :( So to fix this for
> real we'd have to either backport the 2.9 visibility handling to 2.8 or
> implement image loading so that it uses wx APIs rather than libpng directly,
> which may be difficult as I'm not sure wx's image classes are good at handling
> data being streamed in.
> 

Ok, so I remove the static support

[Alexander Vassilev]

Created attachment 21460 [details]
updated mingw port patch

[Alexander Vassilev]

Created attachment 21462 [details]
path to fix a compilation error and disable a runtime assertion until its fixed

1 Fix for broken compilation in Entity.h - WebCore::Entity::Entity having a default constructor but inherits from a class with no default constructor. This is rather a temporary hack than a fix, but since the Entity calss is only a stub that prevents compilation errors it probably should be ok. Hopefully someone fixes this the right way
2 Assertion failure in FontCache.cpp under MINGW - disabled the ASSERT macro for this file only, as a temporary solution

Probably this patch should be submitted as a solution to a separate bug, but since I have encountered the problem while compiling with MINGW, hte problem could be manifesting only under MINGW, even though its seems not compiler-related.

[Kevin Ollivier]

Hi Alexander,

Thanks for the update. Sorry it's taking time, have been swamped with other projects. For the assertion problem in FontCache.cpp, I'm pretty sure that the patch in bug 19310 will fix your issue. The Entity problem is something I haven't run across though - you might want to file it as a separate bug, as it will probably be looked at much quicker that way.

[Alexander Vassilev]

Hi Kevin,
thanks for the fast response. I will open a bug for FontCache then. I was also quite busy recently, now had some time to finish the mingw stuff.

[Kevin Ollivier]

Hi Alexander,

Just to clarify, bug 19310 has already been filed for the FontCache.cpp issue (it doesn't directly FontCache.cpp because the real issue is in FontPlatformData.h), the one that doesn't have a bug yet is the Entity.cpp issue.

[Alexander Vassilev]

I opened a bug for the Entity problem, is someone is interested: https://bugs.webkit.org/show_bug.cgi?id=19363

[David Kilzer (:ddkilzer)]

Comment on attachment 21460 [details]
updated mingw port patch

I can't comment on the Bakefile or WX-specific changes, but there are some other issues that need to be addressed before this patch may land.

>--- JavaScriptCore/ChangeLog	(revision 34300)
>+++ JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>[phone]  Alexander Vassilev  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * jscore.bkl:
>+        * kjs/DateMath.cpp:
>+        (KJS::getLocalTime):
>+        * kjs/testkjs.cpp:
>+        * wtf/Platform.h:
>+        * wtf/Threading.h:
>+        (WTF::atomicIncrement):
>+        (WTF::atomicDecrement):

Please reference the bug number, but title and bug URL in every ChangeLog entry.  See other entries for reference.

Also, please provide a summary statement about the changes, or comment on each individual file (or method) change.  Again, see other ChangeLog entries for examples.

>-    #if COMPILER(MSVC7)
>+    #if (COMPILER(MSVC7) || COMPILER(MINGW32))

We don't normally add outer parenthesis in this case since they're not necessary.

>Index: JavaScriptCore/kjs/testkjs.cpp
>===================================================================
>--- JavaScriptCore/kjs/testkjs.cpp	(revision 34267)
>+++ JavaScriptCore/kjs/testkjs.cpp	(working copy)
>@@ -53,7 +53,7 @@
> #endif
> 
> #if PLATFORM(WIN_OS)
>-#include <crtdbg.h>
>+//#include <crtdbg.h>
> #include <windows.h>
> #endif
> 

We don't commit commented-out code.  This probably needs #if/#endif macro protection.

Also note that testkjs.cpp has changed to Shell.cpp, so this patch has some minor "bit rot".

>+/*in mingw windows header winbase.h InterlckedIncrement() and the like are defined to accept plain LPVOID, not volatile,
>+    so we avoid compile error, but what should really be fixed is hte mingw header! */

Please format comments like others found in the source, specifically use capital letters to start sentences and add spaces betweeen "/*" and the text.

>[phone]  Alexander Vassilev  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Port to the MINGW compiler and the cygwin environment. Fix for time functions that
>+        are not available under MINGW. Removed includes for gdiplus system headers that are
>+        not necessary but add gdiplus dependency which MINGW does not have. Changed order of
>+        includes in FormDataStreamCurl.cpp since stdint.h gets included before the point
>+        where we define the macro __STDC_LIMIT_MACROS, so we did not get SIZE_MAX defined
>+
>+        * loader/FTPDirectoryDocument.cpp:
>+        (WebCore::processFileDateString):
>+        * loader/FTPDirectoryParser.cpp:
>+        * platform/network/curl/FormDataStreamCurl.cpp:
>+        * platform/wx/wxcode/win/non-kerned-drawing.cpp:
>+        * webcore-base.bkl:
>+        * webcore-wx.bkl:

Please include bug number, bug title and bug URL in the ChangeLog.

The description of the changes is good, but it may be more useful to denote individual changes in the list of files.

>Index: WebCore/loader/FTPDirectoryDocument.cpp
>===================================================================
>--- WebCore/loader/FTPDirectoryDocument.cpp	(revision 34267)
>+++ WebCore/loader/FTPDirectoryDocument.cpp	(working copy)
>@@ -264,7 +264,12 @@ static String processFileDateString(cons
>     // If it was today or yesterday, lets just do that - but we have to compare to the current time
>     struct tm now;
>     time_t now_t = time(NULL);
>-    localtime_r(&now_t, &now);
>+	#ifndef __MINGW32__
>+		localtime_r(&now_t, &now);
>+	#else
>+	/*MINGW doesnt have localtime_r() or localtime_s() yet*/
>+		now = *localtime(&now_t);
>+	#endif
>     
>     // localtime does "year = current year - 1900", compensate for that for readability and comparison purposes
>     now.tm_year += 1900;

Don't re-indent code when adding #ifndef/#else/#endif macros.

Please add spaces between "/*' and "*/" and comment text.  (This comment is kind of stating the obvious as well; it may not be needed.)

>Index: WebCore/platform/network/curl/FormDataStreamCurl.cpp
>===================================================================
>--- WebCore/platform/network/curl/FormDataStreamCurl.cpp	(revision 34267)
>+++ WebCore/platform/network/curl/FormDataStreamCurl.cpp	(working copy)
>@@ -27,8 +27,6 @@
>  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
> 
>-#include "config.h"
>-
> // We need to define __STDC_LIMIT_MACROS to define SIZE_MAX.
> #ifndef __STDC_LIMIT_MACROS
> #define __STDC_LIMIT_MACROS
>@@ -38,6 +36,7 @@
> #include <stdint.h>
> #endif
> 
>+#include "config.h"
> #include "FormDataStreamCurl.h"
> 
> #include "CString.h"

The #include "config.h" line is a special case; please do not move it.

>-#include "gdiplus.h"
>- 
>+//#include "gdiplus.h"
>+

Again, we don't commit commented-out code.  Please use a #if/#endif guard.

>Index: WebKit/wx/ChangeLog
>===================================================================
>--- WebKit/wx/ChangeLog	(revision 34300)
>+++ WebKit/wx/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>[phone]  Alexander Vassilev  <[e-mail]>
>+
>+        Reviewed by NOBODY (OOPS!).
>+        This patch ports the wxWebkit build system to support the MINGW compiler
>+        in combiantion with the cygwin environment
>+        
>+        * Bakefiles.bkgen:
>+        * WebView.cpp:
>+        * dependencies.bkl:
>+        * presets/wxwebkit.bkl:
>+        * wxwebkit.bkl:
>+        * wxwk-settings.bkl:

Please reference bug number, bug title and bug URL; leave a blank line after the "Reviewed by" line.


>+#ifdef __WXMSW__
>+#include <wx/msw/private.h> //for wxGetInstance()
>+#endif

Please leave a space between "//" and the comment text.

>Index: WebKitTools/ChangeLog
>===================================================================
>--- WebKitTools/ChangeLog	(revision 34300)
>+++ WebKitTools/ChangeLog	(working copy)
>@@ -1,3 +1,12 @@
>[phone]  Alexander Vassilev  <[e-mail]>
>+
>+        PAtch to the wxWebkit build system to support building with the MINGW compiler
>+        and cygwin environment
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * wx/browser/browser.bkl:
>+        * wx/build-wxwebkit:

Same ChangeLog comments apply.

r- for comment formatting and commented out code in patch.

Please address these issues, and resubmit the patch for review.  Thanks for contributing, Alexander!

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> (From update of attachment 21460 [details] [edit])
> I can't comment on the Bakefile or WX-specific changes, but there are some
> other issues that need to be addressed before this patch may land.

I also noticed some tab characters in the patch.  We use 4 spaces instead of a tab character in all WebKit source.  See this page for more details:

http://webkit.org/coding/coding-style.html

[Kevin Ollivier]

We no longer use Bakefile as our build system so this patch is invalid.