Bug 73382 - split webkit gyp files for chromium build to break the circular dependency
Summary: split webkit gyp files for chromium build to break the circular dependency
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 73996
Blocks: 68463 73384
  Show dependency treegraph
 
Reported: 2011-11-29 18:19 PST by Dirk Pranke
Modified: 2011-12-07 16:01 PST (History)
3 users (show)

See Also:


Attachments
Patch (40.32 KB, patch)
2011-11-29 18:21 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback (72.21 KB, patch)
2011-11-30 14:38 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge to r101718 (72.62 KB, patch)
2011-12-01 16:33 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (73.28 KB, patch)
2011-12-05 18:01 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-11-29 18:19:07 PST
See bug 68463 for the context ... this change clones the build rules for all of the executables into new .gyp files. We're doing this so that targets in webkit_support can declare a dependency on webkit without introducing a circular dependency between WebKit.gyp and webkit_support.gyp (WebKit.gyp will not be allowed to depend on webkit_support.gyp, but Tools.gyp and WebKitUnitTests.gyp will, since DRT et al. need the libraries in webkti_support to build).
Comment 1 Dirk Pranke 2011-11-29 18:19:57 PST
Note that the duplication should be short-lived, and eventually we'll delete all the rules from WebKit.gyp once we can assume that the build_webkit_exes_from_webkit_gyp file is false.
Comment 2 Dirk Pranke 2011-11-29 18:21:02 PST
Created attachment 117091 [details]
Patch
Comment 3 WebKit Review Bot 2011-11-30 05:15:09 PST
Comment on attachment 117091 [details]
Patch

Attachment 117091 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10709151
Comment 4 Tony Chang 2011-11-30 10:18:21 PST
Comment on attachment 117091 [details]
Patch

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

r- for bot failures

> Source/WebKit/chromium/All.gyp:42
> +                'WebKit.gyp:*',
> +                'WebKitUnitTests.gyp:*',
> +                '../../../Tools/Tools.gyp:*',

Is it sufficient to just depend on webkit_unittests and DumpRenderTree?  That way we can be more confident that the other dependencies are correct.

> Source/WebKit/chromium/WebKit.gyp:1100
> +        # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> +        # leave the indentation alone to minimize whitespace diffs.

I would go ahead and indent. PrettyPatch should be smart enough to show that it's just an indention change.

> Source/WebKit/chromium/WebKitUnitTests.gyp:35
> +    # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> +    # leave the indentation alone to minimize whitespace diffs.

I would indent this properly too.

> Source/WebKit/chromium/gyp_webkit:136
> +  # Change to the base dir needed for the gyp file path above to work correctly.
> +  os.chdir(script_dir)

This failed on the bots since when gclient runs it, it's already in the script_dir.

> Tools/Tools.gyp:35
> +    # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> +    # leave the indentation alone to minimize whitespace diffs.

Same as above.
Comment 5 Dirk Pranke 2011-11-30 12:10:42 PST
(In reply to comment #4)
> > Source/WebKit/chromium/All.gyp:42
> > +                'WebKit.gyp:*',
> > +                'WebKitUnitTests.gyp:*',
> > +                '../../../Tools/Tools.gyp:*',
> 
> Is it sufficient to just depend on webkit_unittests and DumpRenderTree?  That way we can be more confident that the other dependencies are correct.
> 

I waffled on which way to do this. Your way should work as well, so I'll change to that.

> > Source/WebKit/chromium/WebKit.gyp:1100
> > +        # https://bugs.webkit.org/show_bug.cgi?id=68463. Until then, we
> > +        # leave the indentation alone to minimize whitespace diffs.
> 
> I would go ahead and indent. PrettyPatch should be smart enough to show that it's just an indention change.
> 

Ok, will do.

> > Source/WebKit/chromium/gyp_webkit:136
> > +  # Change to the base dir needed for the gyp file path above to work correctly.
> > +  os.chdir(script_dir)
> 
> This failed on the bots since when gclient runs it, it's already in the script_dir.
>

Yeah, I noticed that this didn't seem to be working right; I'm not sure why that is failing, but I'll delete it.
Comment 6 Dirk Pranke 2011-11-30 14:38:07 PST
Created attachment 117276 [details]
update w/ review feedback
Comment 7 Tony Chang 2011-11-30 14:44:26 PST
Comment on attachment 117276 [details]
update w/ review feedback

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

Please make sure the chromium ews bot passes before landing.

> Source/WebKit/chromium/gyp_webkit:133
> -               'WebKit.gyp'])
> +               os.path.join('All.gyp')])

Why os.path.join on a single item?
Comment 8 Dirk Pranke 2011-11-30 14:51:08 PST
(In reply to comment #7)
> (From update of attachment 117276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117276&action=review
> 
> Please make sure the chromium ews bot passes before landing.
> 

Roger.

> > Source/WebKit/chromium/gyp_webkit:133
> > -               'WebKit.gyp'])
> > +               os.path.join('All.gyp')])
> 
> Why os.path.join on a single item?

No reason; it's left over from a previous version. I will fix.
Comment 9 Dirk Pranke 2011-12-01 16:33:10 PST
Created attachment 117520 [details]
merge to r101718
Comment 10 WebKit Review Bot 2011-12-01 22:34:49 PST
Comment on attachment 117520 [details]
merge to r101718

Attachment 117520 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10693926

New failing tests:
fast/layers/clip-rects-transformed.html
fast/replaced/no-focus-ring-object.html
http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
http/tests/security/contentSecurityPolicy/object-src-no-url-allowed.html
fast/layers/clip-rects-transformed-2.html
http/tests/plugins/npapi-response-headers.html
fast/frames/sandboxed-iframe-about-blank.html
http/tests/plugins/get-url.html
http/tests/plugins/interrupted-get-url.html
fast/replaced/no-focus-ring-embed.html
compositing/plugins/invalidate_rect.html
fast/frames/iframe-plugin-load-remove-document-crash.html
editing/selection/selection-plugin-clear-crash.html
fast/replaced/invalid-object-with-fallback.html
http/tests/plugins/cross-frame-object-access.html
fast/frames/sandboxed-iframe-navigation-allowed.html
http/tests/plugins/geturlnotify-from-npp-destroystream.html
fast/events/tabindex-focus-blur-all.html
http/tests/plugins/third-party-cookie-accept-policy.html
Comment 11 Dirk Pranke 2011-12-05 18:01:14 PST
Created attachment 117971 [details]
Patch
Comment 12 Dirk Pranke 2011-12-07 16:01:41 PST
Committed r102201: <http://trac.webkit.org/changeset/102201>