build-webkit: silence output of 'which'

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

When webkitdirs.pm runs 'which' it redirects its stdout to /dev/null. At least GNU which prints to stderr when a binary is not found, so we should redirect stderr as well. This fixes an annoying warning that prints at the top of the output of build-webkit on distros that rename ninja to ninja-build.

[Michael Catanzaro]

Created attachment 244315 [details]
Patch

[Daniel Bates]

Comment on attachment 244315 [details]
Patch

We should take this opportunity to write these functions in terms of webkitdirs::commandExists() instead of duplicating effort. Notice that webkitdirs::commandExists() assumes that a command that exists will emit a non-empty string to stdout when passed --version. We may need to improve the implementation of webkitdirs::commandExists() if it's insufficient to use to test for the existence of ninja, eclipse, and ninja-build.

[Michael Catanzaro]

The existing function works fine for ninja, but in the Java tradition, eclipse does not support --version.

[Michael Catanzaro]

Created attachment 244412 [details]
Patch

[Michael Catanzaro]

The Windows support is speculative, based on [1] and [2]. I hope that will be tested by the EWS?

[1] http://technet.microsoft.com/en-us/library/cc753148.aspx
[2] http://support.microsoft.com/kb/110930

[Daniel Bates]

Comment on attachment 244412 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:896
> +    return $? == 0;

$? encodes both the exit status and signal code that caused termination of the child process and I'm unclear whether the signal code would be zero under normal circumstances on Windows. Regardless, we should make use of exitStatus() extract the exit status of the child before comparing to 0. Specifically, we should pass the return value of system() to exitStatus().

> Tools/Scripts/webkitdirs.pm:1774
> +    return commandExists('ninja') || commandExists('ninja-build');

Nit: ' (single quote) => " (double quote)

> Tools/Scripts/webkitdirs.pm:1779
> +    return commandExists('eclipse');

Ditto.

[Daniel Bates]

The Windows portion of this patch looks sane to me given the referenced documents in comment 5.

CC'ed Brent Fulgham who is familiar with the Windows command line environment.

[Michael Catanzaro]

Created attachment 244414 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 244414 [details]
Patch

Clearing flags on attachment: 244414

Committed r178243: <http://trac.webkit.org/changeset/178243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.