Bug 44802 - REGRESSION (r65351): WebCore build fails due to attempting to directly access WebKitTools/Scripts
Summary: REGRESSION (r65351): WebCore build fails due to attempting to directly access...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P1 Blocker
Assignee: Mark Rowe (bdash)
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2010-08-27 16:07 PDT by Mark Rowe (bdash)
Modified: 2010-08-28 19:35 PDT (History)
3 users (show)

See Also:


Attachments
Something like this. (178.97 KB, patch)
2010-08-27 16:52 PDT, Mark Rowe (bdash)
no flags Details | Formatted Diff | Diff
Patch (186.18 KB, patch)
2010-08-27 17:39 PDT, Mark Rowe (bdash)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2010-08-27 16:07:44 PDT
<http://trac.webkit.org/changeset/65351> has broken the ability to build WebCore in production configurations.  The change to DerivedSources.make attempts to access directories outside of the WebCore source directory.  Projects are built independently and without access to any other source trees.

This needs to be fixed as soon as is possible as this change has completely broken the ability to build production builds of WebKit on Mac OS X and Windows.  If it cannot be fixed promptly we’ll need to roll this particular change out.
Comment 1 Eric Seidel (no email) 2010-08-27 16:15:24 PDT
Seems easy enough to fix.  Rolling out the change will just break your builds in other ways.
Comment 2 Mark Rowe (bdash) 2010-08-27 16:18:18 PDT
A second issue is that the script involved uses third-party Python modules.  That will also need to be addressed.
Comment 3 Eric Seidel (no email) 2010-08-27 16:22:30 PDT
The 3rd party python module "simplejson" is already provided in WebKitTools, which is why the tool was placed there.  When I placed the tool there, I was not aware of Apple's build limitation.  Thank you for bringing this to my attention.

Access to a json parser (which simplejson provides) is the only reason why the script was placed in WebKitTools.  One could simply check in a second copy of simplejson under WebCore to fix Apple's production build.
Comment 4 Eric Seidel (no email) 2010-08-27 16:25:09 PDT
The script need not require simplejson, any way to parse json files will do.  Other solutions including moving HTMLEntityNames.json away from json, or writing our own json-like parser are possible.  When the script was written originally by Adam, he used simplejson.  I assume he chose simplejson because it was already used by many other of our scripts and already checked into our repository.
Comment 5 Mark Rowe (bdash) 2010-08-27 16:28:46 PDT
(In reply to comment #3)
> The 3rd party python module "simplejson" is already provided in WebKitTools, which is why the tool was placed there.  When I placed the tool there, I was not aware of Apple's build limitation.  Thank you for bringing this to my attention.

It’s always been the case in the Mac OS X and Windows build systems that individual projects cannot access files outside of their source directory.  This is precisely the reason why delightful things such as forwarding headers exist.

> Access to a json parser (which simplejson provides) is the only reason why the script was placed in WebKitTools.  One could simply check in a second copy of simplejson under WebCore to fix Apple's production build.

Beyond being obviously unpleasant, I’m not sure that this would work.  One further constraint is that it’s not acceptable to modify the source tree in any way.  Python writes out .pyc files for modules that it imports, which I believe would violate that constraint.
Comment 6 Mark Rowe (bdash) 2010-08-27 16:30:59 PDT
(In reply to comment #4)
> The script need not require simplejson, any way to parse json files will do.  Other solutions including moving HTMLEntityNames.json away from json, or writing our own json-like parser are possible.  When the script was written originally by Adam, he used simplejson.  I assume he chose simplejson because it was already used by many other of our scripts and already checked into our repository.

Looking at the input file it seems like JSON is overkill for this file.  It’s basically a list of names and corresponding values.  Something simple like CSV would be sufficient as far as I can see.
Comment 7 Mark Rowe (bdash) 2010-08-27 16:52:28 PDT
Created attachment 65787 [details]
Something like this.
Comment 8 Adam Barth 2010-08-27 16:53:53 PDT
We can use whatever input format is most convenient.  I used JSON just because it's easy to work with in most modern languages.
Comment 9 Mark Rowe (bdash) 2010-08-27 16:57:42 PDT
So easy that it requires you install a third-party module…

I’ll flag this patch for review shortly.  I’m waiting on a couple of full world builds to complete to ensure that everything builds correctly.
Comment 10 Adam Barth 2010-08-27 16:58:20 PDT
Yeah, that looks fine.  Does that address the pyc issue as well?  (Of course, the final version will need to build on other platforms too.)
Comment 11 Adam Barth 2010-08-27 17:00:00 PDT
> So easy that it requires you install a third-party module…

simplejson is built into modern versions of Python.  We just need the external module to support Mr. Tiger.

> I’ll flag this patch for review shortly.  I’m waiting on a couple of full world builds to complete to ensure that everything builds correctly.

That patch is close, but there are a bunch of other build systems that need to call this script too.  If you grep for HTMLEntityNames.json, you can see how they work.
Comment 12 Mark Rowe (bdash) 2010-08-27 17:00:58 PDT
Yes, it addresses the .pyc issue as there are no .py modules within the source tree that the script imports.
Comment 13 Mark Rowe (bdash) 2010-08-27 17:02:04 PDT
(In reply to comment #11)
> > So easy that it requires you install a third-party module…
> 
> simplejson is built into modern versions of Python.  We just need the external module to support Mr. Tiger.

JSON support was added in Python 2.6.  A third-party module is needed even for Leopard.

> > I’ll flag this patch for review shortly.  I’m waiting on a couple of full world builds to complete to ensure that everything builds correctly.
> 
> That patch is close, but there are a bunch of other build systems that need to call this script too.  If you grep for HTMLEntityNames.json, you can see how they work.

Yep, I’m aware of that.
Comment 14 Adam Barth 2010-08-27 17:03:55 PDT
> Yes, it addresses the .pyc issue as there are no .py modules within the source tree that the script imports.

Oh, cool.
Comment 15 Adam Barth 2010-08-27 17:05:11 PDT
BTW, these files just moved into WebCore/html/parser.  You'll probably want to work from a revision in the past few minutes.
Comment 16 Adam Barth 2010-08-27 17:07:29 PDT
Also, thanks for working on fixing this issue.
Comment 17 Mark Rowe (bdash) 2010-08-27 17:39:43 PDT
Created attachment 65795 [details]
Patch

Updated patch.
Comment 18 Adam Barth 2010-08-27 17:46:24 PDT
Comment on attachment 65795 [details]
Patch

Thanks.

WebCore/html/parser/create-html-entity-table:101
 +  // THIS FILE IS GENERATED BY WebCore/html/create-html-entity-table
WebCore/html/create-html-entity-table => WebCore/html/parser/create-html-entity-table

If we were more awesome, we'd generate the path automagically
Comment 19 Mark Rowe (bdash) 2010-08-27 17:50:40 PDT
I noticed one further thing that needs to be included in this patch: updating the references to HTMLEntityNames.json in WebCore.xcodeproj.  While I’m there I’m going to stop copying the input file in to the framework wrapper, since copying it there is just stupid.  I’ll make these changes as I land.
Comment 20 Eric Seidel (no email) 2010-08-27 18:03:06 PDT
Attachment 65795 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3868031
Comment 21 Mark Rowe (bdash) 2010-08-28 19:35:06 PDT
Landed in r66319.