Bug 96245 - [Qt] Fix generation of forward headers for generated files in WebCore on Windows
Summary: [Qt] Fix generation of forward headers for generated files in WebCore on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks: 76776
  Show dependency treegraph
 
Reported: 2012-09-10 01:29 PDT by Simon Hausmann
Modified: 2012-09-10 04:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.09 KB, patch)
2012-09-10 01:32 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2012-09-10 01:29:08 PDT
[Qt] Fix generation of forward headers for generated files in WebCore on Windows
Comment 1 Simon Hausmann 2012-09-10 01:32:59 PDT
Created attachment 163063 [details]
Patch
Comment 2 Tor Arne Vestbø 2012-09-10 02:58:06 PDT
Comment on attachment 163063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163063&action=review

> Source/WebKit2/DerivedSources.pri:148
> +    win32: eval($${header_target}.commands = ($${QMAKE_MKDIR} $$toSystemPath($$dest_dir) 2>nul || echo>nul))

It looks like we use $${QMAKE_CHK_DIR_EXISTS} || $${QMAKE_MKDIR} in the rest of Qt's makefiles, perhaps stick with that?
Comment 3 Simon Hausmann 2012-09-10 03:52:42 PDT
Comment on attachment 163063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163063&action=review

>> Source/WebKit2/DerivedSources.pri:148
>> +    win32: eval($${header_target}.commands = ($${QMAKE_MKDIR} $$toSystemPath($$dest_dir) 2>nul || echo>nul))
> 
> It looks like we use $${QMAKE_CHK_DIR_EXISTS} || $${QMAKE_MKDIR} in the rest of Qt's makefiles, perhaps stick with that?

The problem with that approach is that it doesn't work well with parallel rules that try to create the same directory ;(

IOW there's a race condition between the check if the directory exists and making it. If you have two jobs trying to do the same thing, then
both jobs determine simultaneously that the directory doesn't exist, both try to create the directory at the same time and only once succeeds. The
second one will see mkdir aborting because the directory already exists.

I tried removing the mkdir from this rule altogether and instead depend on the other generator that creates the directory also, but unfortunately when
depending on such a symbol target nmake will always call it and thus always re-generate the file.
Comment 4 Tor Arne Vestbø 2012-09-10 04:34:19 PDT
Comment on attachment 163063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163063&action=review

>>> Source/WebKit2/DerivedSources.pri:148
>>> +    win32: eval($${header_target}.commands = ($${QMAKE_MKDIR} $$toSystemPath($$dest_dir) 2>nul || echo>nul))
>> 
>> It looks like we use $${QMAKE_CHK_DIR_EXISTS} || $${QMAKE_MKDIR} in the rest of Qt's makefiles, perhaps stick with that?
> 
> The problem with that approach is that it doesn't work well with parallel rules that try to create the same directory ;(
> 
> IOW there's a race condition between the check if the directory exists and making it. If you have two jobs trying to do the same thing, then
> both jobs determine simultaneously that the directory doesn't exist, both try to create the directory at the same time and only once succeeds. The
> second one will see mkdir aborting because the directory already exists.
> 
> I tried removing the mkdir from this rule altogether and instead depend on the other generator that creates the directory also, but unfortunately when
> depending on such a symbol target nmake will always call it and thus always re-generate the file.

Ouch! You're right, that will not be pretty. Wonder why Qt gets away with that though.
Comment 5 WebKit Review Bot 2012-09-10 04:40:06 PDT
Comment on attachment 163063 [details]
Patch

Clearing flags on attachment: 163063

Committed r128045: <http://trac.webkit.org/changeset/128045>
Comment 6 WebKit Review Bot 2012-09-10 04:40:10 PDT
All reviewed patches have been landed.  Closing bug.