Summary: | build-webkit: silence output of 'which' | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebKit2 | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Minor | CC: | bfulgham, commit-queue, dbates, mcatanzaro, ossy | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2015-01-08 19:07:31 PST
Created attachment 244315 [details]
Patch
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.
The existing function works fine for ninja, but in the Java tradition, eclipse does not support --version. Created attachment 244412 [details]
Patch
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 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. 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. Created attachment 244414 [details]
Patch
Comment on attachment 244414 [details] Patch Clearing flags on attachment: 244414 Committed r178243: <http://trac.webkit.org/changeset/178243> All reviewed patches have been landed. Closing bug. |