|Summary:||webkit-build-directory misuses terms|
|Product:||WebKit||Reporter:||Eric Seidel (no email) <eric>|
|Component:||Tools / Tests||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||commit-queue, dpranke, mrowe|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:||34898|
Description Eric Seidel (no email) 2010-02-10 17:42:26 PST
From an email from Mark Rowe: Added: trunk/WebKitTools/Scripts/webkit-build-directory (0 => 54634) --- trunk/WebKitTools/Scripts/webkit-build-directory (rev 0) +++ trunk/WebKitTools/Scripts/webkit-build-directory 2010-02-11 01:24:04 UTC (rev 54634) @@ -0,0 +1,65 @@ +my $programName = basename($0); +my $usage = <<EOF; +Usage: $programName [options] + --base Show the root build directory instead of one corresponding to the current target (e.g. Debug, Release) + --debug Show build directory for the Debug target + -h|--help Show this help message + --release Show build directory for the Release target +EOF The terminology used here is wrong. Debug and Release are configurations, not targets. A target is a result of a build (such as derived source file generation, or the WebKit framework). The documentation for --base is also confusing. It talks about a "root build directory" while the option name implies it is the "base build directory". Presumably they're the same thing so we should pick a single name for them. + +setConfiguration(); # Figure out from the command line if we're --debug or --release or the default. … this is why the method to process these flags uses the term configuration ;-) It would probably be clearer to explain the behavior in terms of two modes: one that shows the base build directory, and a second that shows the per-configuration build directory. I would suggest using explicit flags to select the mode of operation, since as it stands the default behavior is somewhat confusing (it will show the per-configuration build directory for the active configuration). == end email == I'm about to prepare a patch to fix the Configuration/Target confusion. I'm not sure I understand the suggestion with regards to the flags. The current behavior is designed to be "correct" for any shell script/Makefile which wants to know where to put output. But you're right, that may be confusing to users just running the script without args. Could you give an example of another interface you'd prefer?
Comment 1 Mark Rowe (bdash) 2010-02-10 17:49:13 PST
The current behavior seems to be: webkit-build-directory -> WebKitBuild/Release webkit-build-directory --debug -> WebKitBuild/Debug webkit-build-directory --base -> WebKitBuild I’m basically suggesting that we do the following: webkit-build-directory -> error, need to specify which directory you want. webkit-build-directory --configuration-build-directory -> WebKitBuild/Release webkit-build-directory --configuration-build-directory --debug -> WebKitBuild/Debug webkit-build-directory --base-build-directory -> WebKitBuild I don’t think that it is intuitive for a script named webkit-build-directory to default to giving you the per-configuration build directory. Making it explicit which directory is being requested is much clearer. The option names I suggested may be a little verbose. I just picked the most literal names possible.
Comment 3 Eric Seidel (no email) 2010-02-11 15:37:21 PST
Comment on attachment 48538 [details] Patch Does this look good to you Mark?
Comment 4 WebKit Commit Bot 2010-02-12 12:58:00 PST
Comment on attachment 48538 [details] Patch Rejecting patch 48538 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12157 test cases. websocket/tests/bufferedAmount-after-close.html -> crashed Exiting early after 1 failures. 12124 tests run. 500.30s total testing time 12123 test cases (99%) succeeded 1 test case (<1%) crashed 7 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/261095
Comment 5 Eric Seidel (no email) 2010-02-12 14:46:03 PST
Comment on attachment 48538 [details] Patch Bug 34898.
Comment 6 WebKit Commit Bot 2010-02-14 20:42:08 PST
Comment on attachment 48538 [details] Patch Clearing flags on attachment: 48538 Committed r54759: <http://trac.webkit.org/changeset/54759>
Comment 7 WebKit Commit Bot 2010-02-14 20:42:17 PST
All reviewed patches have been landed. Closing bug.