Bug 38149 – check-webkit-style should not run the carriage checker on Windows-only files

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

The carriage checker should not be invoked on Windows-only files, such as *.vcproj.

[Dumitru Daniliuc]

Created attachment 54349 [details]
patch

[Chris Jerdonek]

(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.

[Dumitru Daniliuc]

Created attachment 54351 [details]
patch

Added back the SVN eol-style comment. Sorry, I thought introducing a whitelist might be enough.

[Chris Jerdonek]

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.

[Shinichiro Hamaji]

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.

[Chris Jerdonek]

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

[Chris Jerdonek]

(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.

[Dumitru Daniliuc]

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.

[Chris Jerdonek]

(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.