Bug 68463 – binaries in webkit.gyp should be split into a new file to break circular dependencies on webkit_support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

binaries in webkit.gyp should be split into a new file to break circular dependencies on webkit_support

[Dirk Pranke]

Created attachment 108047 [details]
Patch

[Dirk Pranke]

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

[Tony Chang]

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

[WebKit Review Bot]

Comment on attachment 108047 [details]
Patch

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

[Tony Chang]

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.

[Dirk Pranke]

Created attachment 108078 [details]
merge, update w/ review feedback

[Dirk Pranke]

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.

[WebKit Review Bot]

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

[Dirk Pranke]

Created attachment 108269 [details]
fix most compile problems on linux

[Dirk Pranke]

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?

[Evan Martin]

I'm not too familiar with this setup.  What is the difference between Makefile.chromium and Makefile.chrome?

[Tony Chang]

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.

[Tony Chang]

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

[Dirk Pranke]

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?

[Evan Martin]

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

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 108412 [details]
fix relative paths

[Adam Barth]

@tony^work: Would you be willing to review this patch?

[Dirk Pranke]

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.

[Dirk Pranke]

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.

[Dirk Pranke]

(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