Bug 34822 - webkit-build-directory misuses terms
Summary: webkit-build-directory misuses terms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 34898
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-10 17:42 PST by Eric Seidel (no email)
Modified: 2010-02-17 12:26 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.20 KB, patch)
2010-02-10 18:06 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 2 Eric Seidel (no email) 2010-02-10 18:06:24 PST
Created attachment 48538 [details]
Patch
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.
Comment 8 Eric Seidel (no email) 2010-02-17 12:26:32 PST
Crash was bug 34898.