Bug 140278 - build-webkit: silence output of 'which'
Summary: build-webkit: silence output of 'which'
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Minor
Assignee: Michael Catanzaro
Depends on:
Reported: 2015-01-08 19:07 PST by Michael Catanzaro
Modified: 2015-01-10 15:30 PST (History)
5 users (show)

See Also:

Patch (1.44 KB, patch)
2015-01-08 19:13 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.90 KB, patch)
2015-01-10 10:06 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.91 KB, patch)
2015-01-10 13:01 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-01-08 19:07:31 PST
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.
Comment 1 Michael Catanzaro 2015-01-08 19:13:28 PST
Created attachment 244315 [details]
Comment 2 Daniel Bates 2015-01-08 20:03:04 PST
Comment on attachment 244315 [details]

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.
Comment 3 Michael Catanzaro 2015-01-10 09:43:06 PST
The existing function works fine for ninja, but in the Java tradition, eclipse does not support --version.
Comment 4 Michael Catanzaro 2015-01-10 10:06:36 PST
Created attachment 244412 [details]
Comment 5 Michael Catanzaro 2015-01-10 10:07:32 PST
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
Comment 6 Daniel Bates 2015-01-10 11:52:02 PST
Comment on attachment 244412 [details]

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');

Comment 7 Daniel Bates 2015-01-10 11:57:07 PST
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.
Comment 8 Michael Catanzaro 2015-01-10 13:01:12 PST
Created attachment 244414 [details]
Comment 9 WebKit Commit Bot 2015-01-10 15:29:57 PST
Comment on attachment 244414 [details]

Clearing flags on attachment: 244414

Committed r178243: <http://trac.webkit.org/changeset/178243>
Comment 10 WebKit Commit Bot 2015-01-10 15:30:00 PST
All reviewed patches have been landed.  Closing bug.