Bug 34822 – webkit-build-directory misuses terms

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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?

[Mark Rowe (bdash)]

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.

[Eric Seidel (no email)]

Created attachment 48538 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 48538 [details]
Patch

Does this look good to you Mark?

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 48538 [details]
Patch

Bug 34898.

[WebKit Commit Bot]

Comment on attachment 48538 [details]
Patch

Clearing flags on attachment: 48538

Committed r54759: <http://trac.webkit.org/changeset/54759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Crash was bug 34898.