Bug 105671 – chromium: webkit-build-directory doesn't handle ninja output dirs

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

[WebKit Review Bot]

chromum: webkit-build-directory doesn't handle ninja output dirs
Requested by thakis_ on #webkit.

[Nico Weber]

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

[Nico Weber]

Actually, this appears to be working fine if --chromium is passed as argument.

[Nico Weber]

…or does it. nevermind.

[Nico Weber]

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.

[Nico Weber]

Created attachment 180577 [details]
Patch

[Nico Weber]

Patch attempt.

[Daniel Bates]

Comment on attachment 180577 [details]
Patch

Looks sane to me.

[Nico Weber]

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.

[WebKit Review Bot]

Comment on attachment 180577 [details]
Patch

Clearing flags on attachment: 180577

Committed r138406: <http://trac.webkit.org/changeset/138406>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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