Bug 37786 - new-run-webkit-tests: checking the build should be optional
Summary: new-run-webkit-tests: checking the build should be optional
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: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-18 17:51 PDT by Dirk Pranke
Modified: 2010-04-19 17:33 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.01 KB, patch)
2010-04-18 18:01 PDT, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff
revise w/ ojan's feedback (rename --check-build to --build) (2.72 KB, patch)
2010-04-19 15:16 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
tweak the help text, changelog (2.94 KB, patch)
2010-04-19 16:45 PDT, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-04-18 17:51:11 PDT
Checking whether or not a build is up-to-date can take 5-10 seconds or longer, and sometime you may wish to run the tests without rebuilding, so there should be a way to skip the check_build step.
Comment 1 Dirk Pranke 2010-04-18 18:01:30 PDT
Created attachment 53648 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-04-18 22:25:19 PDT
Comment on attachment 53648 [details]
Patch

Two notes:
1.  Sad that we need this.  Seems like a hack around build system lameness.
2.  I hate that optparse doesn't have built in support for --no-.  We need to make our own make_option wrapper which does automatic --no- generation as well as puts the default value in the help text.
Comment 3 Ojan Vafai 2010-04-19 08:03:21 PDT
(In reply to comment #2)
> 2.  I hate that optparse doesn't have built in support for --no-.  We need to
> make our own make_option wrapper which does automatic --no- generation as well
> as puts the default value in the help text.

Why do we need to have both flags? Since it defaults to True, don't we only need --no-check-build?

It seems like it's the actual rebuilding that slows things down. It's a shame to get rid of all the other checks in check_build just to avoid the rebuilding. How about adding a --no-build instead? I also think that would be more intuitive to users. If I didn't know the code, it wouldn't be clear to me that check-build is the thing that causes DRT to be built.
Comment 4 Dirk Pranke 2010-04-19 10:30:27 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > 2.  I hate that optparse doesn't have built in support for --no-.  We need to
> > make our own make_option wrapper which does automatic --no- generation as well
> > as puts the default value in the help text.
> 
> Why do we need to have both flags? Since it defaults to True, don't we only
> need --no-check-build?

This may be a personal thing, but I don't like having boolean command line flags where both versions of the flag aren't legal.
 
> It seems like it's the actual rebuilding that slows things down. It's a shame
> to get rid of all the other checks in check_build just to avoid the rebuilding.
> How about adding a --no-build instead? I also think that would be more
> intuitive to users. If I didn't know the code, it wouldn't be clear to me that
> check-build is the thing that causes DRT to be built.

This would be fine, and is probably safer.
Comment 5 Dirk Pranke 2010-04-19 15:16:55 PDT
Created attachment 53727 [details]
revise w/  ojan's feedback (rename --check-build to --build)
Comment 6 Ojan Vafai 2010-04-19 15:26:00 PDT
LGTM.
Comment 7 Eric Seidel (no email) 2010-04-19 16:12:37 PDT
Comment on attachment 53727 [details]
revise w/  ojan's feedback (rename --check-build to --build)

The option name and help are misleading.  This will only make sure that DumpRenderTree is built.  Not webkit itself.

I've been trying to use full sentences in option help (start with a capital, end with a period).

I think  the option names can stay, but we should fix the help to explain it only builds the helper tools, not WebKit.
Comment 8 Dirk Pranke 2010-04-19 16:45:56 PDT
Created attachment 53738 [details]
tweak the help text, changelog
Comment 9 Eric Seidel (no email) 2010-04-19 16:48:56 PDT
Comment on attachment 53738 [details]
tweak the help text, changelog

OK.
Comment 10 Dirk Pranke 2010-04-19 17:33:14 PDT
Committed r57858: <http://trac.webkit.org/changeset/57858>