Bug 38149 - check-webkit-style should not run the carriage checker on Windows-only files
Summary: check-webkit-style should not run the carriage checker on Windows-only files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-26 16:45 PDT by Dumitru Daniliuc
Modified: 2010-04-27 14:56 PDT (History)
3 users (show)

See Also:


Attachments
patch (2.52 KB, patch)
2010-04-26 16:51 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (3.33 KB, patch)
2010-04-26 17:29 PDT, Dumitru Daniliuc
hamaji: review-
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-04-26 16:45:11 PDT
The carriage checker should not be invoked on Windows-only files, such as *.vcproj.
Comment 1 Dumitru Daniliuc 2010-04-26 16:51:22 PDT
Created attachment 54349 [details]
patch
Comment 2 Chris Jerdonek 2010-04-26 17:05:09 PDT
(In reply to comment #1)
> Created an attachment (id=54349) [details]

-        # FIXME: We should probably use the SVN "eol-style" property
-        #        or a white list to decide whether or not to do
-        #        the carriage-return check. Originally, we did the
-        #        check only if (os.linesep != '\r\n').

Is there a reason to delete the FIXME notes re: the SVN "eol-style" property?  It seems that would be worth preserving in some form.
Comment 3 Dumitru Daniliuc 2010-04-26 17:29:14 PDT
Created attachment 54351 [details]
patch

Added back the SVN eol-style comment. Sorry, I thought introducing a whitelist might be enough.
Comment 4 Chris Jerdonek 2010-04-26 23:30:06 PDT
In looking at the code, I see that .vcproj files are not currently set to be processed as text files (or as any type of file for that matter -- e.g. C++, Python, etc.):

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L356 

Given that, would it make more sense simply to add .vcproj files to the list of files that should be skipped altogether?

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L177

Otherwise, it seems like .vcproj should be added to the list of text file extensions.
Comment 5 Shinichiro Hamaji 2010-04-27 00:17:00 PDT
Comment on attachment 54351 [details]
patch

Thanks for addressing this issue.

Putting r- as I agree with Chris. Maybe we should just put .vcproj file into the skipped list.

> Index: LayoutTests/storage/quota-tracking.html
> ===================================================================
> --- LayoutTests/storage/quota-tracking.html	(revision 58269)
> +++ LayoutTests/storage/quota-tracking.html	(working copy)

This change isn't related to this bug.
Comment 6 Chris Jerdonek 2010-04-27 06:48:53 PDT
I filed this related report, which will indirectly take care of issues like the one reported here:

https://bugs.webkit.org/show_bug.cgi?id=38197
Comment 7 Chris Jerdonek 2010-04-27 08:55:42 PDT
(In reply to comment #6)
> I filed this related report, which will indirectly take care of issues like the
> one reported here:
> 
> https://bugs.webkit.org/show_bug.cgi?id=38197

I submitted a patch for bug 38197 which should also address the issue in this report.

If we want .vcproj files to be checked as text files (so that the tab check will be done on them, for example), then we may want to go forward with an additional variable as was proposed above to control which files should be exempted from the carriage return check.  The patch in bug 38197 does not do this because .vcproj files are implicitly skipped altogether.  This is because they are not in the list of recognized text file extensions.
Comment 8 Dumitru Daniliuc 2010-04-27 14:06:38 PDT
I'm closing this bug since the patch for bug 38197 takes care of this problem. If it turns out that it's not OK to skip vcproj files altogether, then we can reopen the bug.
Comment 9 Chris Jerdonek 2010-04-27 14:56:13 PDT
(In reply to comment #8)
> I'm closing this bug since the patch for bug 38197 takes care of this problem.
> If it turns out that it's not OK to skip vcproj files altogether, then we can
> reopen the bug.

If this is creating a problem that needs to be fixed right away, you can add ".vcproj" to the list of skipped files without warning (one-line change):

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/checker.py?rev=58263#L177

I probably can't get to updating my patch to bug 38197 until tonight at the earliest.