Bug 68463 - binaries in webkit.gyp should be split into a new file to break circular dependencies on webkit_support
Summary: binaries in webkit.gyp should be split into a new file to break circular depe...
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: Dirk Pranke
URL:
Keywords:
Depends on: 73382 73384 76505
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-20 13:16 PDT by Dirk Pranke
Modified: 2012-01-23 19:22 PST (History)
7 users (show)

See Also:


Attachments
Patch (39.31 KB, patch)
2011-09-20 13:21 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge, update w/ review feedback (39.28 KB, patch)
2011-09-20 16:58 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix most compile problems on linux (39.47 KB, patch)
2011-09-21 19:29 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix relative paths (39.56 KB, patch)
2011-09-22 15:28 PDT, 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-09-20 13:16:50 PDT
binaries in webkit.gyp should be split into a new file to break circular dependencies on webkit_support
Comment 1 Dirk Pranke 2011-09-20 13:21:14 PDT
Created attachment 108047 [details]
Patch
Comment 2 Dirk Pranke 2011-09-20 13:23:38 PDT
Tony, this is roughly what I had in mind to fix the circular dependencies between WebKit.gyp and webkit_support.gyp.

As an aside, why does the webkit target include the webkit_unittest_files in the component build? If we can fix that, then WebKit.gyp doesn't need to include DRT.gypi, which would seem like a good thing, but I assume there's a good reason for the include that I'm just not seeing ...
Comment 3 Tony Chang 2011-09-20 14:21:43 PDT
(In reply to comment #2)
> As an aside, why does the webkit target include the webkit_unittest_files in the component build? If we can fix that, then WebKit.gyp doesn't need to include DRT.gypi, which would seem like a good thing, but I assume there's a good reason for the include that I'm just not seeing ...

I'm not very familiar with the component build.  Victor did the work for webkit.dll right before switching teams.  Darin may also be familiar with the reasoning.
Comment 4 WebKit Review Bot 2011-09-20 14:22:05 PDT
Comment on attachment 108047 [details]
Patch

Attachment 108047 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9774389
Comment 5 Tony Chang 2011-09-20 16:09:02 PDT
Comment on attachment 108047 [details]
Patch

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

I just skimmed, but conceptually this looks fine.  r- because it looks like there are build errors.  Also, you'll have to find a way to not break the canary bots since src/webkit/webkit.gyp references WebKit.gyp:DumpRenderTree directly (probably need to do a 3 sided change).

> Tools/DumpRenderTree/DumpRenderTree.gyp:433
> +# Local Variables:
> +# tab-width:2
> +# indent-tabs-mode:nil
> +# End:
> +# vim: set expandtab tabstop=2 shiftwidth=2:

Remove this and locally use tools/emacs/chrome-filetypes.el or tools/vim/filetypes.vim intead.
Comment 6 Dirk Pranke 2011-09-20 16:58:30 PDT
Created attachment 108078 [details]
merge, update w/ review feedback
Comment 7 Dirk Pranke 2011-09-20 16:59:43 PDT
Uploading a new patch to see if the build errors disappear with a merge to HEAD. I'm still pondering the three-sided change issue.

As an aside, I'm pretty sure the component build doesn't work for a webkit-only checkout. It looks like WebTestingSupport is never built in that config.
Comment 8 WebKit Review Bot 2011-09-20 17:11:21 PDT
Comment on attachment 108078 [details]
merge, update w/ review feedback

Attachment 108078 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9763480
Comment 9 Dirk Pranke 2011-09-21 19:29:41 PDT
Created attachment 108269 [details]
fix most compile problems on linux
Comment 10 Dirk Pranke 2011-09-21 19:35:41 PDT
Okay, I think I have most of the compiling issues working, but for some reason, after I type make once (and things compile), if I type "rm out/Debug/DumpRenderTree; make -f Makefile.chromium" it doesn't do anything. Similarly, 'make -f Makefile.chromium out/Debug/DumpRenderTree' doesn't work (but make -f Makefile.chrome DumpRenderTree does).

It's as if the aliases in the targets aren't getting set up properly. Any ideas?
Comment 11 Evan Martin 2011-09-22 09:14:28 PDT
I'm not too familiar with this setup.  What is the difference between Makefile.chromium and Makefile.chrome?
Comment 12 Tony Chang 2011-09-22 11:56:04 PDT
Dirk made a typo.  There's only Makefile.chromium.

The reason out/Debug/DumpRenderTree doesn't work is because the output dir is in Source/WebKit/chromium/out.  E.g., make -f Makefile.chromium Source/WebKit/chromium/out/Debug/DumpRenderTree should work.
Comment 13 Tony Chang 2011-09-22 11:57:21 PDT
(In reply to comment #12)
> The reason out/Debug/DumpRenderTree doesn't work is because the output dir is in Source/WebKit/chromium/out.  E.g., make -f Makefile.chromium Source/WebKit/chromium/out/Debug/DumpRenderTree should work.

Nope, I'm wrong.  The output files are in out, not Source/WebKit/chromium.  Hmm, I dunno what's going on.  Maybe look at the generated Makefile?
Comment 14 Dirk Pranke 2011-09-22 13:37:51 PDT
Okay, the output *.target.chromium.mk files generated from my patch don't contain the 

# Add target alias to "all" target.
.PHONY: all
all: DumpRenderTree

lines. Any idea what might cause that? Does it have to do wit the fact that the gyp file being generated is not in the current working directory?
Comment 15 Evan Martin 2011-09-22 13:52:53 PDT
Relevant code:

    # Add an alias for each target (if there are any outputs).
    # Installable target aliases are created below.
    if ((self.output and self.output != self.target) and
        (self.type not in self._INSTALLABLE_TARGETS)):
      self.WriteMakeRule([self.target], [self.output],
                         comment='Add target alias', phony = True)
      if part_of_all:
        self.WriteMakeRule(['all'], [self.target],
                           comment = 'Add target alias to "all" target.',
                           phony = True)


self.target is the gyp target name, as specified in the input file (like "chrome_common").  self.output is the output filename for the current gyp target (like "out/Debug/DumpRenderTree").
Comment 16 Dirk Pranke 2011-09-22 14:16:39 PDT
Ugh. It turns out that there's a minor discrepancy in how gyp is handling the paths to the build file.

make.py:2550:

  # Find the list of targets that derive from the gyp file(s) being built.
  needed_targets = set()
  for build_file in params['build_files']:
    for target in gyp.common.AllTargets(target_list, target_dicts, build_file):
      needed_targets.add(target)

in gyp.common.AllTargets, build_file needs to prefix match the targets in target_list, so I need to match against "../../../../Tools/DumpRenderTree/DumpRenderTree.gyp:". However, in gyp_webkit, I pass in os.path.join(script_dir, "..", "..", "..", ".."). That first script_dir turns into '.', which throws off the prefix string match.

The script_dir isn't really needed (I don't think), so I can just fix that. However, it seems like this might be a bug in gyp -- it should probably be more resilient to path matches -- and I might want to throw in an os.chdir(script_dir) prior to running the gyp invocation, just to be more safe.
Comment 17 Dirk Pranke 2011-09-22 15:28:20 PDT
Created attachment 108412 [details]
fix relative paths
Comment 18 Adam Barth 2011-10-14 23:04:58 PDT
@tony^work: Would you be willing to review this patch?
Comment 19 Dirk Pranke 2011-10-15 14:43:27 PDT
I'm actually going to clear the review? bit on this ... I need to figure out how to roll out this patch before it's really ready.
Comment 20 Dirk Pranke 2011-11-16 17:35:06 PST
Okay, per conversation w/ Tony here's the plan:

We'll split out all of the executables into new .gyp files, a Source/WebKit/chromium/WebKitTests.gyp for TestWebKitAPI and webkit_unit_tests and a Tools/Tools.gyp for DRT, ImageDiff et al. 

DRT will also depend on the targets in TestWebKitAPI just to be able to pull in all of the executables in one place (although I'm kinda wondering if it makes more sense to have an AllWebKit.gyp instead somewhere).

In terms of rolling this out, life is kinda complicated by having to split these things over the two repos. The plan is:

1) create a new "build_drt_from_webkit_tools" flag in build/common.gypi in chromium (actual name TBD)
2) rev the webkit Source/WebKit/chromium/DEPS to pull in (1)
3) clone the executable targets into the new files and wrap them (in both places) behind the appropriate conditions.
4) what for (3) to get picked up by the downstream roll
5) flip the flag in build/common.gypi
6) remove the executable targets from WebKit.gyp.
Comment 21 Dirk Pranke 2011-11-29 18:31:22 PST
(In reply to comment #20)
> 
> 1) create a new "build_drt_from_webkit_tools" flag in build/common.gypi in chromium (actual name TBD)
> 2) rev the webkit Source/WebKit/chromium/DEPS to pull in (1)

These are done.

> 3) clone the executable targets into the new files and wrap them (in both places) behind the appropriate conditions.

This is now posted for review in bug 73382.

> 4) wait for (3) to get picked up by the downstream roll
> 5) flip the flag in build/common.gypi

This will happen in chromium bug 105826: http://code.google.com/p/chromium/issues/detail?id=105826

> 6) remove the executable targets from WebKit.gyp.

This will be tracked in bug 73384.

There's also a 7) remove the flag from build/common.gypi, which will be tracked in chromium bug 105827: http://code.google.com/p/chromium/issues/detail?id=105827