Bug 73382 – split webkit gyp files for chromium build to break the circular dependency

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

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).

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 117091 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117091 [details]
Patch

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

[Tony Chang]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 117276 [details]
update w/ review feedback

[Tony Chang]

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?

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 117520 [details]
merge to r101718

[WebKit Review Bot]

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

[Dirk Pranke]

Created attachment 117971 [details]
Patch

[Dirk Pranke]

Committed r102201: <http://trac.webkit.org/changeset/102201>