Bug 12327 – Windows build bustage + vcproj/sln craziness

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Don Gibson]

The Windows build is busted (as usual).

In addition, the entire Windows build system is hosed, meaning that all my bustage fixes over the past couple months haven't made the BuildBot start working again.  In looking into this I found that all the Visual Studio files (.slns and .vcprojs) were basically hacked together in a disorganized and barely function fashion.

Patch coming to fix the Windows build and try and make the Windows project files and build tools slightly more sane and consistent.

[Don Gibson]

Created attachment 12545 [details]
patch for r18619

This patch is a bit out-of-date, so not requesting review until I make a new version that's updated to latest trunk.

This patch should make every .sln in the project able to completely compile in debug and release mode (I hope) as of trunk revision 18619.  (Several have been broken for who knows how long.)  This means the build-webkit.sh script that the BuildBot uses will work again.  To do this, many .slns were updated with correct dependency trees, and the .vcprojs were cleaned up to have saner include directories and library options.

Additionally, this introduces a uniform naming scheme for intermediate and output directories on Windows, which were previously scattered around willy-nilly.

[Don Gibson]

I suspect the reason that patch is so big is because many files which had been in SVN in DOS mode are in UNIX mode in my patch.  Does make it a bit hard to review, though...

[Don Gibson]

Created attachment 12549 [details]
patch for r18972

Updated to latest trunk.

[Don Gibson]

If some SVN admin can mark JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore.sln as non-binary, I can create an updated patch without the crazy MIME stuff in it.

Also, my patch is supposed to delete Image Viewer.aps entirely (VS temp file that never should have been added to the repo), so that's another thing that could just be taken care of by an admin to make the patch saner.

[Maciej Stachowiak]

I made the two SVN changes you requested. Care to respin the patch? Also I think the usefulness of Image Viewer is long past; perhaps it would be better to remove it from SVN entirely than to update the build files like this.

r- for respin 

[Daniel Knauth]

The patch compiles cleanly against r18972 both for Debug and Release
--> well done.

When starting Spinneret though, the stacks gets smashed in WebCore::FrameLoader::checkContentPolicy (Frameloader.cpp line 1929).
Any idea how to solve this?

[Don Gibson]

Created attachment 13186 [details]
patch for r19641

OK, sorry for the very long delay in updating this.

Note that this patch adds and removes some files, make sure you catch those properly when committing.

[Don Gibson]

(In reply to comment #6)
> When starting Spinneret though, the stacks gets smashed in
> WebCore::FrameLoader::checkContentPolicy (Frameloader.cpp line 1929).
> Any idea how to solve this?

If it's the same problem as last time I looked, someone is trying to use a pointer after its been freed, because the ownership semantics of things are all wrong, probably because so much stuff is stubbed out with "notImplemented();".  I haven't really looked into making this stuff actually work -- getting it to compile again is the key goal of this bug :)

[Adam Roben (:aroben)]

Comment on attachment 13186 [details]
patch for r19641

It's pretty hard to review this patch given the multiple files where every line has changed. Can you break this patch up into smaller chunks and annotate your changes for each?

[Daniel Knauth]

Adam, most of Don's changes are in the win32/COM code and in the (somewhat bloated) VC project files. Potential conflicts with other platforms can be only in:

 WebCore/config.h
 WebCore/loader/FrameLoaderClient.h
 WebCore/page/ChromeClient.h
 WebCore/page/ContextMenuClient.h
 WebCore/page/DragClient.h
 WebCore/platform/LocalizedStrings.h
 WebCore/platform/network/ResourceHandleInternal.h

[Don Gibson]

(In reply to comment #9)
> (From update of attachment 13186 [details] [edit])
> It's pretty hard to review this patch given the multiple files where every line
> has changed. Can you break this patch up into smaller chunks and annotate your
> changes for each?

I don't know why SVN thinks I've changed every line of the vcproj files.  Maybe I can figure that out.  My changes in those files are actually much less extensive than it appears.

I can try to break the patch into pieces, but I'm not sure it will be much easier to review, since there aren't many intertwined changes (almost everything is basically a standalone compile fix).

[Don Gibson]

Created attachment 13202 [details]
patch for r19659

OK, try this patch.  Looks like while editing the various .vcproj and .sln files some of them got switched from DOS to UNIX format.  I changed things so each is in the format it already was in, so none of the files should show up as "every line changed" anymore.  Note that probably someday someone should go through all these and make them sane (shouldn't everything in the repo be in UNIX format?).

Also updated to the latest trunk and fixed the build failure that introduced.

[Adam Roben (:aroben)]

Comment on attachment 13202 [details]
patch for r19659

This is certainly an improvement, but we shouldn't be overhauling the build system so thoroughly while also trying to just get things building again. Changing the build system like this will break existing contributors' configurations. Again, it's very hard to review a patch of this size at all, and especially hard without a detailed ChangeLog. I'd really like to see this patch split up into smaller chunks that can be reviewed and discussed separately.

[Daniel Knauth]

(In reply to comment #11)

> I don't know why SVN thinks I've changed every line of the vcproj files.  

It might help to set the property svn:eol-style on the project files to 'native' or 'CRLF', such that linefeeds get converted to Windows style on checkout. 

[Daniel Knauth]

(In reply to comment #13)
> This is certainly an improvement, but we shouldn't be overhauling the build
> system so thoroughly while also trying to just get things building again.
> Changing the build system like this will break existing contributors'
> configurations. 

AFAIK, the Windows build system has been broken at least since Q4 2006. For example, Release builds worked every other week, but Debug never compiled. In 2007, Release also stopped building.

How about splitting the patch in two parts, one for the project files, and another for the source changes. If things compile again on Windows after applying both, then at least the project files are OK. The sources changes should be a relatively small patch and therefore easy to review.

[Don Gibson]

(In reply to comment #13)
> (From update of attachment 13202 [details] [edit])
> This is certainly an improvement, but we shouldn't be overhauling the build
> system so thoroughly while also trying to just get things building again.
> Changing the build system like this will break existing contributors'
> configurations.

The reason for the extensive build system overhauls is that the build system is totally busted, which is why the buildbot hasn't worked for months.  None of the dependencies were in place at all and the fact that it had even worked for a little while was something of a miracle.  I've had many past "bustage fix" patches land that didn't actually turn the buildbot green for precisely this reason.

Admittedly, there's probably more in the patch than is strictly necessary -- I didn't need to make the output directories consistent while doing everything else.  But that's really the only extraneous change.  I could perhaps try and construct three patches, one that makes the build system work again (i.e. vcproj/sln fixes), one that makes everything compile (i.e. source fixes), and one that fixes the output paths (i.e. vcproj cleanup).

Note, wrt breaking existing configurations, I tested building these changes on top of an existing trunk checkout that I had (long) previously built successfully, and had no problems.  I'm not sure what, exactly, will break in contributors' configurations, especially when those contributors presumably haven't been able to build for a good four months.

[Don Gibson]

Created attachment 13285 [details]
patch for r19760

Updated the patch to fix the latest trunk.  I haven't yet had time to split this into more than one piece, so I didn't bother sticking a ChangeLog or review request on it.  Wanted to post it anyway since the changes for the latest trunk are nontrivial.

[Daniel Knauth]

(In reply to comment #16)

> I could perhaps try and
> construct three patches, one that makes the build system work again (i.e.
> vcproj/sln fixes), one that makes everything compile (i.e. source fixes), and
> one that fixes the output paths (i.e. vcproj cleanup).

What is the benefit of splitting build system and output path fixes? Both are due and do not interfere with other platforms.

As for the 'breaking configurations' issue, I can confirm that TOT only compiles *with* Don's fixes for quite a while now, so I at difficulty to understand what exactly can break that is not broken already.

[Don Gibson]

Created attachment 13308 [details]
patch for r19782

Updated to latest ToT, still haven't had time to split the patch.

I'm not sure my last patch would have worked.  I started running into all kinds of crazy issues when trying to compile today that led to me ripping out all the remaining vestiges of FrameWin, which I'd intended to do before but apparently hadn't managed.  Also, for some reason I was getting a bunch of headers that were not under the "#pragma warning(push, 0)" guards so they were all causing warnings-as-errors problems.  Rather than trying to guard everything in the universe I just went ahead and fixed the warnings.  This is the cause of most of the cross-platform header changes in this patch -- they shouldn't be functional changes, just warning fixes, except in a few cases where I did change the types used to avoid some pointless type conversion.

I've also been trying to get Spinneret to sort of run again.  This patch and the last one both fixed some errors, including the annoying & tricky "ESP not saved properly" error.  We still aren't setting up the DocumentLoader correctly so we crash when trying to use it initially, but that's progress :)

[Don Gibson]

Created attachment 13316 [details]
Split patch for r19793, part 1: .vcproj cleanup

OK, I'm trying to manually split the latest patch as requested, in hopes of easier review.  This is part 1 of 3, which does a lot of .vcproj cleanup (and some small script changes too).

I would not advise landing these split patches in isolation; if at all possible you should land a monolithic patch, since I haven't tested the various combinations of the patches, and since parts 2 and 3 are definitely necessary to fix the build, and part 1 should land now (while the build is busted) rather than later anyway.

[Don Gibson]

Created attachment 13320 [details]
Split patch for r19793, part 2: .vcproj and .sln build bustage

Here is part 2 of the split patch.  This adjusts the dependencies in the .slns, which should help the buildbot to work better, and makes other necessary .vcproj updates.

[Don Gibson]

Created attachment 13322 [details]
Split patch for r19793, part 3: Compile fixes

Last and largest part of the split patch, updates the source code to compile correctly, including a bunch of warning fixes that, as I mentioned before, I needed when compiling WebKit.

[Don Gibson]

Created attachment 13323 [details]
Monolithic patch for r19793, including unified ChangeLog comments

This is the monolothic version of the above three patches, including all ChangeLog comments, and is what I highly suggest you land instead of trying to land the above.

Once again, note that this patch removes several files, so be sure to svn delete them when landing.

[Adam Roben (:aroben)]

Comment on attachment 13320 [details]
Split patch for r19793, part 2: .vcproj and .sln build bustage

This looks good to me.

r=me

[Maciej Stachowiak]

Comment on attachment 13316 [details]
Split patch for r19793, part 1: .vcproj cleanup

I'm not sure what changes are here. Seems like one thing it will do is remove the _debug suffix for debug versions of the libraries, which I'm pretty sure we don't want.

[Don Gibson]

(In reply to comment #25)
> (From update of attachment 13316 [details] [edit])
> I'm not sure what changes are here. Seems like one thing it will do is remove
> the _debug suffix for debug versions of the libraries, which I'm pretty sure we
> don't want.

The various projects were all over the map in how they built things.  Some used suffixes, some used both suffixes and subdirectories, the directory schemes were seemingly random.

You're right that the _debug suffix is removed, but it doesn't seem like a good idea anyway.  Instead this builds everything into one consistent set of locations that are parallel for all vcprojs, and near the top of the output path all of it either goes into Debug/ or Release/.  That way your files in both modes look the same, to switch modes you just change what directory you're pointing to, which is generally nice for debugging, and makes writing and testing scripts that package up all your objects for distribution (say, for testing by a group elsewhere) a bit easier.

The other changes in that particular patch mainly hardcode fewer things (the directory paths are mostly constructed from builtin macros, to facilitate any future project additions or name changes), do slightly less copying of output to various locations, and as I mentioned before, use the correct syntax to set all the variables like link libraries, so the builder knows how to figure dependencies properly and the resulting vcprojs are shorter and more readable.

If you have specific things that were purposeful about the existing design that this changes, please share.  I was operating under the impression that most of the Windows build system was pretty slapdash and ill-maintained, and imposing some order would be a good thing, but obviously I don't want to break someone else's checkout -- I spent quite a bit of time trying to build all configurations of this and use all the different scripts I could find to make sure it all seemed to hang together well.

[Adam Roben (:aroben)]

Comment on attachment 13316 [details]
Split patch for r19793, part 1: .vcproj cleanup

I'm going to r- this based on Maciej's comments.

[Adam Roben (:aroben)]

Comment on attachment 13322 [details]
Split patch for r19793, part 3: Compile fixes

-#include <wtf/Platform.h>
+#include "wtf/Platform.h"

   This is the standard Mac "framework-style" #include, so this shouldn't change.

-void CSSPrimitiveValue::setFloatValue(unsigned short unitType, double floatValue, ExceptionCode& ec)
+void CSSPrimitiveValue::setFloatValue(unsigned short ut, double floatValue, ExceptionCode& ec)
 {
+    UnitTypes unitType = (UnitTypes)ut;

   Why not just change the type of the parameter to UnitTypes? Also, if we do end up casting, static_cast would be better.

+    virtual void copyNonAttributeProperties(const Element * /*source*/) {}

   The * should be next to the type.

-    class ResourceRequest;
+    struct ResourceRequest;

   We like to keep forward declarations of classes and structs separate, so just put this declaration below all the class declarations, with an empty line in between.

 private:
-    DeprecatedStringData(const DeprecatedStringData &);

   Why was this removed? It's used to make sure that DeprecatedStringData can't be copied (although really DeprecatedStringData should just inherit from Noncopyable).

+#if PLATFORM(MAC)
     String fileButtonNoFileSelectedLabel();
+#endif

   So far we've only #ifdef'd out strings that pertain to platform-specific features. "No file selected" is not a platform-specific feature, so I don't think we should add this #if.

+    unsigned short outlineSize() const { return (unsigned short)max(0, outlineWidth() + outlineOffset()); }

   static_cast would be better here.

+#include "FrameLoader.h"  // Need to give the compiler the right size of a
+                          // FramePolicyFunction so the stack layout doesn't
+                          // get hosed through functions which take one

   I think a comment at the end of the list of #includes would be better than having it inline like this.

   r- for now until the above are addressed. We'll also need to make sure that these changes don't break other platforms.

[Don Gibson]

(In reply to comment #27)
> (From update of attachment 13316 [details] [edit])
> I'm going to r- this based on Maciej's comments.

OK, but my comment #26 was my reply to Maciej's comments.  I'm still not really sure what it is I need to change/fix about this to gain r+.

(In reply to comment #28)
> -void CSSPrimitiveValue::setFloatValue(unsigned short unitType, double
> floatValue, ExceptionCode& ec)
> +void CSSPrimitiveValue::setFloatValue(unsigned short ut, double floatValue,
> ExceptionCode& ec)
>  {
> +    UnitTypes unitType = (UnitTypes)ut;
> 
>    Why not just change the type of the parameter to UnitTypes? Also, if we do
> end up casting, static_cast would be better.

I did originally do that, but these are called from auto-generated functions in the JS code, and changing the type caused all that code to break instead.  The solution there is, presumably, to figure out how to change the code generator to use the right types as well, but I didn't know how to do that.

As for C-style casts versus static_casts, for some reason I had it in my head that I needed to make sure this could be compiled by a C compiler... despite it being C++ code.  I don't know what I was thinking, I'll fix that.

>  private:
> -    DeprecatedStringData(const DeprecatedStringData &);
> 
>    Why was this removed? It's used to make sure that DeprecatedStringData can't
> be copied (although really DeprecatedStringData should just inherit from
> Noncopyable).

It was removed because there's already a "DeprecatedStringData(DeprecatedStringData &);" declared in the public section, and for some reason the MSVC compiler doesn't like having both of these and gives you warnings.  I tend to agree with it; the presence of both is misleading, and if a DeprecatedStringData should not be copied, then I should have removed the public function rather than the private one.  But I think it was actually used.  This is a case where I think the compiler is pointing out a real problem, but I don't know how to fix it :)

[Don Gibson]

Created attachment 13548 [details]
patch for r20064

Updated the monolithic patch to latest trunk, including fixes for review comments except for the items noted in comment #29.  I have (reluctantly) removed all the stuff from "split patch part 1" (the build system cleanup patch), which will hopefully make this more of a rubber-stamp.

[Don Gibson]

Created attachment 13671 [details]
patch for r20240

Updated to latest trunk.  Please review soon.

[Peter Kasting]

Created attachment 14163 [details]
Updated patch, vcproj/sln changes only

This patch is based on Don's earlier patch but includes only the vcproj/sln components.  I updated the patch to account for the few files that have been added/removed since the last version.  This won't unbreak the build, but it's a partial fix that should be easy to rubber-stamp and land.

[Peter Kasting]

Created attachment 14181 [details]
Add newly needed dependencies to install-win-extras

After discussion with othermaciej on IRC, this patch updates install-win-extras to download and install the relevant bits of pthreads-win32 and sqlite3 as dependencies, alongside the existing dependencies.  I've already updated the wiki page with the corresponding manual install instructions, both for these packages and the other pre-existing dependencies.

[Peter Kasting]

Comment on attachment 14163 [details]
Updated patch, vcproj/sln changes only

Hold off on reviewing this one, I'm going to update it to include the pthreads/sqlite dependencies.

[Peter Kasting]

(In reply to comment #33)
> Created an attachment (id=14181) [edit]
> Add newly needed dependencies to install-win-extras
> 
> After discussion with othermaciej on IRC, this patch updates install-win-extras
> to download and install the relevant bits of pthreads-win32 and sqlite3 as
> dependencies, alongside the existing dependencies.

Note: after running the patched install-win-extras, svn gives unknown file notifications for some of the newly-downloaded items.  I'm not sure how to make it ignore them like it's ignoring the other downloaded dependencies.  That should probably be fixed somehow.

[Peter Kasting]

Created attachment 14182 [details]
vcproj/sln patch, including pthreads/sqlite

This is a tweaked version of the previous vcproj/sln patch which properly sets the pthreads include dir (the sqlite dir was already set) and includes the sqlite-dependent files in WebCore/loader/icon/ in the build.

This doesn't add the pthreads/sqlite libraries to the link lines; I'll cross that bridge when I get to the point of being able to link, so I can test that I've done things correctly.

[Peter Kasting]

Created attachment 14191 [details]
Complete patch for r21100

This does everything needed to get the Windows port building again in debug and release modes: updates dependencies, includes all necessary .sln and .vcproj changes, fixes all compile and link errors.

I hope this patch will be easier to review than Don's last patch.  I didn't bother to fix any warnings and tried to limit my changes as much as possible to Windows-specific files or code, so this should be a near-rubber-stamp.

I did have to fix one error introduced in r21090, where the compiler could not determine which version of abs() to call (multiple overloads matched equally well).  Presumably on the Mac this is unambiguous for some reason.

[Peter Kasting]

Created attachment 14208 [details]
Complete patch for r21114

Updated to latest trunk, which fixed some of the problems my patch fixed.

[Nikolas Zimmermann]

I think this is obsolete now that we have a real windows port.

[Darin Adler]

Comment on attachment 13671 [details]
patch for r20240

Presumably we have to revisit this now that there's new Windows code in TOT. If not, feel free to set to review+ again.

[Darin Adler]

Comment on attachment 14208 [details]
Complete patch for r21114

Presumably we have to revisit this now that there's new Windows code in TOT. If not, feel free to set to review+ again.