Bug 18428 - Patch to support building wxWebKit on mingw
Summary: Patch to support building wxWebKit on mingw
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit wx (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Major
Assignee: Kevin Ollivier
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-11 07:47 PDT by Alexander Vassilev
Modified: 2010-06-24 12:04 PDT (History)
4 users (show)

See Also:


Attachments
patch to webkit tree (39.27 KB, patch)
2008-04-11 07:48 PDT, Alexander Vassilev
no flags Details | Formatted Diff | Diff
patch to bakefile script (315 bytes, patch)
2008-04-11 07:49 PDT, Alexander Vassilev
no flags Details | Formatted Diff | Diff
readme and howto (5.25 KB, text/plain)
2008-04-11 07:49 PDT, Alexander Vassilev
no flags Details
bash script for creating the webkit patch (1.61 KB, text/plain)
2008-04-11 07:50 PDT, Alexander Vassilev
no flags Details
patch (37.90 KB, patch)
2008-04-11 12:15 PDT, Alexander Vassilev
avasilev: review-
Details | Formatted Diff | Diff
updated patch (39.08 KB, patch)
2008-04-20 04:20 PDT, Alexander Vassilev
no flags Details | Formatted Diff | Diff
updated mingw port patch (40.55 KB, patch)
2008-06-02 06:43 PDT, Alexander Vassilev
ddkilzer: review-
Details | Formatted Diff | Diff
path to fix a compilation error and disable a runtime assertion until its fixed (932 bytes, patch)
2008-06-02 07:00 PDT, Alexander Vassilev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Vassilev 2008-04-11 07:47:20 PDT
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'
Comment 1 Alexander Vassilev 2008-04-11 07:48:50 PDT
Created attachment 20480 [details]
patch to webkit tree
Comment 2 Alexander Vassilev 2008-04-11 07:49:24 PDT
Created attachment 20481 [details]
patch to bakefile script
Comment 3 Alexander Vassilev 2008-04-11 07:49:55 PDT
Created attachment 20482 [details]
readme and howto
Comment 4 Alexander Vassilev 2008-04-11 07:50:35 PDT
Created attachment 20483 [details]
bash script for creating the webkit patch
Comment 5 Alexander Vassilev 2008-04-11 07:51:41 PDT
I just figured out that I can attach files after creating the bug report, so all files are attached now
Comment 6 Alexander Vassilev 2008-04-11 12:15:23 PDT
Created attachment 20485 [details]
patch

Here is an updated patch agains svn revision 31813
Comment 7 Alp Toker 2008-04-20 00:20:41 PDT
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.)
Comment 8 Alexander Vassilev 2008-04-20 04:20:48 PDT
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.
Comment 9 Kevin Ollivier 2008-04-20 10:01:58 PDT
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... :)
Comment 10 Alexander Vassilev 2008-04-20 15:34:22 PDT
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".
Comment 11 Kevin Ollivier 2008-04-20 16:03:08 PDT
(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.
Comment 12 Alexander Vassilev 2008-04-20 17:03:47 PDT
> 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
Comment 13 Alexander Vassilev 2008-06-02 06:43:27 PDT
Created attachment 21460 [details]
updated mingw port patch
Comment 14 Alexander Vassilev 2008-06-02 07:00:40 PDT
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.
Comment 15 Kevin Ollivier 2008-06-02 12:01:53 PDT
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.
Comment 16 Alexander Vassilev 2008-06-02 15:08:30 PDT
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.
Comment 17 Kevin Ollivier 2008-06-02 15:18:19 PDT
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.
Comment 18 Alexander Vassilev 2008-06-02 15:39:20 PDT
I opened a bug for the Entity problem, is someone is interested: https://bugs.webkit.org/show_bug.cgi?id=19363
Comment 19 David Kilzer (:ddkilzer) 2008-07-01 08:23:16 PDT
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 @@
>+2008-06-02  Alexander Vassilev  <avasilev@voipgate.com>
>+
>+        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.

>+2008-06-02  Alexander Vassilev  <avasilev@voipgate.com>
>+
>+        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 @@
>+2008-06-02  Alexander Vassilev  <avasilev@voipgate.com>
>+
>+        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 @@
>+2008-06-02  Alexander Vassilev  <avasilev@voipgate.com>
>+
>+        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!
Comment 20 David Kilzer (:ddkilzer) 2008-07-01 08:25:34 PDT
(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

Comment 21 Kevin Ollivier 2010-06-24 12:04:05 PDT
We no longer use Bakefile as our build system so this patch is invalid.