Bug 36957 - check-webkit-style: fails silently on non-existent files
Summary: check-webkit-style: fails silently on non-existent files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-01 06:52 PDT by Chris Jerdonek
Modified: 2010-04-03 00:48 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (9.19 KB, patch)
2010-04-02 10:26 PDT, Chris Jerdonek
hamaji: review+
cjerdonek: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2010-04-01 06:52:57 PDT
check-webkit-style doesn't always give a warning on non-existent files.  For example--

> check-webkit-style non-existent_file
Total errors found: 0 in 1 files
>

We should probably check for this at the beginning of check_file().  If the file doesn't exist, we should either error out or log a warning, and not increment the file_count.
Comment 1 Chris Jerdonek 2010-04-01 06:57:17 PDT
We might also want to log a warning in the case below so it's more obvious that something has gone wrong:

    # We fail when style errors are found or there are no checked files.
    sys.exit(error_count > 0 or file_count == 0)
Comment 2 Chris Jerdonek 2010-04-02 10:26:58 PDT
Created attachment 52422 [details]
Proposed patch
Comment 3 Shinichiro Hamaji 2010-04-03 00:06:48 PDT
Comment on attachment 52422 [details]
Proposed patch

> +        if not os_path_exists(file_path):
> +            _log.error("File does not exist: %s" % file_path)
> +            return

Does this mean we won't terminate the program? If so, I slightly prefer to raise an exception here. If a user invokes the style checker by

% check-webkit-style not_exist WebCore

the error message might be missed due to a lot of style errors in WebCore, I guess.

Otherwise, looks good. Thanks for this change!
Comment 4 Chris Jerdonek 2010-04-03 00:16:20 PDT
(In reply to comment #3)
> (From update of attachment 52422 [details])
> > +        if not os_path_exists(file_path):
> > +            _log.error("File does not exist: %s" % file_path)
> > +            return
> 
> Does this mean we won't terminate the program? If so, I slightly prefer to
> raise an exception here.

I wasn't sure so I chose to be more conservative.  I'm fine with an exception and will change it.  Thanks for the review!
Comment 5 Chris Jerdonek 2010-04-03 00:48:45 PDT
Comment on attachment 52422 [details]
Proposed patch

Committed:

http://trac.webkit.org/changeset/57048