Bug 105671 - chromium: webkit-build-directory doesn't handle ninja output dirs
Summary: chromium: webkit-build-directory doesn't handle ninja output dirs
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: WebKit Review Bot
URL:
Keywords:
Depends on:
Blocks: 104434
  Show dependency treegraph
 
Reported: 2012-12-21 17:04 PST by WebKit Review Bot
Modified: 2012-12-21 19:58 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.95 KB, patch)
2012-12-21 17:29 PST, Nico Weber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2012-12-21 17:04:29 PST
chromum: webkit-build-directory doesn't handle ninja output dirs
Requested by thakis_ on #webkit.
Comment 1 Nico Weber 2012-12-21 17:05:19 PST
eseidel: BuildSlaveSupport/built-product-archive:    _buildDirectory = subprocess.Popen(['perl', os.path.join(os.path.dirname(__file__), "..", "Scripts", "webkit-build-directory"),
[5:00pm] eseidel: thakis_: it appears the webkit build bots seem to care about webkit-build-directory workign correctly
Comment 2 Nico Weber 2012-12-21 17:12:34 PST
Actually, this appears to be working fine if --chromium is passed as argument.
Comment 3 Nico Weber 2012-12-21 17:13:25 PST
…or does it. nevermind.
Comment 4 Nico Weber 2012-12-21 17:19:26 PST
This needs  a patch like this:

--- a/Tools/Scripts/webkitdirs.pm
+++ b/Tools/Scripts/webkitdirs.pm
@@ -222,7 +222,7 @@ sub determineBaseProductDir
 
         undef $baseProductDir unless $baseProductDir =~ /^\//;
     } elsif (isChromium()) {
-        if (isLinux() || isChromiumAndroid() || isChromiumMacMake()) {
+        if (isLinux() || isChromiumAndroid() || isChromiumMacMake() || isChromiumNinja()) {
             $baseProductDir = "$sourceDir/out";



…but isChromiumNinja() calls configuration() which in turn calls the function this patch is in, leading to infinite recursion.
Comment 5 Nico Weber 2012-12-21 17:29:38 PST
Created attachment 180577 [details]
Patch
Comment 6 Nico Weber 2012-12-21 17:29:56 PST
Patch attempt.
Comment 7 Daniel Bates 2012-12-21 17:36:30 PST
Comment on attachment 180577 [details]
Patch

Looks sane to me.
Comment 8 Nico Weber 2012-12-21 17:42:58 PST
tony^work: thakis_: yes, the webkit waterfalls use a builder / tester split
[5:33pm] tony^work: thakis_: http://build.webkit.org/waterfall?category=Chromium
[5:33pm] TabAtkins left the chat room. (Quit: Lost terminal)
[5:34pm] psolanki left the chat room. (Read error: Connection reset by peer)
[5:34pm] thakis_: tony^work: looks like stuff always gets unzipped into webkitbuild, so that probably Just Works
[5:34pm] tony^work: thakis_: All the code for zipping/unzipping should be in Tools/BuildSlaveSupport
[5:34pm] thakis_: as both xcodebuild and out end up in the same place on the test

tony^work: we probably need to teach the zipping script to zip the right files
tony^work: that's the built-product-archive script, I think
[5:38pm] thakis_: dydz: thanks!
[5:38pm] tony^work: Tools/BuildSlaveSupport/built-product-archive
[5:39pm] thakis_: tony^work: that just calls out to webkit-build-directory, which https://bugs.webkit.org/show_bug.cgi?id=105671 is fixing



…sounds like this patch helps with the builder/tester split, too.
Comment 9 WebKit Review Bot 2012-12-21 18:13:45 PST
Comment on attachment 180577 [details]
Patch

Clearing flags on attachment: 180577

Committed r138406: <http://trac.webkit.org/changeset/138406>
Comment 10 WebKit Review Bot 2012-12-21 18:13:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Seidel (no email) 2012-12-21 18:36:17 PST
Comment on attachment 180577 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:1207
> +    # This function can be called from baseProductDir(), which in turn is
> +    # called by configuration(). So calling configuration() here leads to
> +    # infinite recursion. Gyp writes both Debug and Release at the same time
> +    # by default, so just check the timestamp on the Release build.ninja file.
> +    my $config = "Release";

This was what had held me up before.  Neat to see we only need Release here.
Comment 12 Eric Seidel (no email) 2012-12-21 19:58:05 PST
I think this is a pretty elegant solution and I'm no longer worried about the build-webkit --chromium switch to ninja.  I think that's OK to fire whenever.  I'm happy to commit it any time after next Tuesday (since it's now vacation time). :)