Bug 53625

Summary: svn-apply and/or patch(1) has trouble applying patches that makes changes to files with Windows line endings
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, aroben, carol, ddkilzer, eric, gns, levin, mrobinson, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 53607    
Attachments:
Description Flags
Patch
none
Patch
levin: review-
Work-in-progress patch
none
Patch and unit tests
none
Patch and unit tests
none
Patch and unit tests eric: review+

Description Daniel Bates 2011-02-02 13:43:47 PST
As observed in bug #53348 and #53607, svn-apply and/or patch(1) has trouble applying patches that make changes to files with Windows line endings.
Comment 1 Daniel Bates 2011-02-02 13:45:38 PST
Also observed in bug #53573.
Comment 2 Daniel Bates 2011-02-02 13:55:48 PST
I did not encounter any issues when using svn-apply to apply the patch for bug #53348, and bug #53607 (attachment 80936 [details] - https://bugs.webkit.org/attachment.cgi?id=80936).

I am using Mac OS 10.6.6 with patch(1) version 2.5.8.

I am curious as to the whether of patch(1) being used on the various EWS bots.

CC'ing Gustavo Silva with regards to the version patch(1) on the GTK EWS bot.
Comment 3 Daniel Bates 2011-02-02 13:57:23 PST
(In reply to comment #2)
> I am curious as to the whether of patch(1) being used on the various EWS bots.
> 
Wow, that came out terribly mangled.

I meant to write:

I am curious as to what version of patch(1) is installed on the various EWS bots.
Comment 4 Daniel Bates 2011-02-02 17:07:53 PST
On IRC today (02/02/11), Gustavo Silva wrote:
[14:17] <kov> dydx, hey, 2.6 is the patch I have on the EWS (it's got whatever is in debian unstable usually)
Comment 5 Csaba Osztrogon√°c 2011-02-18 09:09:38 PST
On Qt EWS now we use patch 2.5.9. 
After upgrading to Squeeze we will use 2.6. (in 2-3 weeks)
Comment 6 Martin Robinson 2011-02-18 09:14:59 PST
It seems the issue, at least in the case of the GTK+ EWS, is that some files are checked out with CRLF line endings and some with newlines. To ensure proper patch application, I believe that Unix machines need to be using newline line-endings for every file. There are a few knobs you can turn for git checkouts. Gustavo is looking into what the appropriate knob is.
Comment 7 Eric Seidel (no email) 2011-02-18 09:49:30 PST
I'm happy to set up any and all knobs on the commit bots.  Especially if they're documented in the WebKit Git instructions (on the wiki).
Comment 8 Carol Szabo 2011-02-18 11:02:22 PST
I am seeing issues on EWS with patching WebCore.vcproj on the mac based bots and GTK+.
Patching with non-matching line endings is a pain.
The way I addressed the issue in the EWS like systems that I built was partly by rules, partly by software:
The rules are:
1. All text files in the system would be required to have the svn:eol-style property set to native.
2. All files that appear as text files, but for whatever reason cannot have the svn:eol-style property set to native should have svn:mime-type property set to something like application/octet-stream.

The software would use flip or a similar tool to convert the line-endings in the patch to the configuration native to the architecture the buildbot runs in before application.

This should not be that hard to implement. I am volunteering to help.
Comment 9 Gustavo Noronha (kov) 2011-03-14 10:23:39 PDT
I discussed my theory about this on IRC once, and just told dbates about it in an email reply, but thought I'd share it here. There is a .gitattributes file in our tree. It forces CRLF on some file types, including *.vcproj files:

$ grep crlf .gitattributes | wc -l
226
$ grep vcproj .gitattributes 
*.vcproj eol=crlf
$ 

Older checkouts (from before those rules being added) do not seem to suffer from the problem (i.e., my own checkout that runs on a system very much like the EWS bot works, as does Martin's). I'm also happy to apply any knobs, but reading docs and trying (not too much since my time's been quite limited) I didn't find anything that would guarantee the effect we want. Also, since it's not been mentioned explicitly in this bug yet, notice that the commit-queue suffers from the same issue - quite likely because it's got a new checkout itself.
Comment 10 Daniel Bates 2011-03-14 12:02:40 PDT
(In reply to comment #9)
> I discussed my theory about this on IRC once, and just told dbates about it in an email reply, but thought I'd share it here. There is a .gitattributes file in our tree. It forces CRLF on some file types, including *.vcproj files:
> 
> $ grep crlf .gitattributes | wc -l
> 226
> $ grep vcproj .gitattributes 
> *.vcproj eol=crlf
> $ 
> 

Older versions of Git (e.g. 1.7.0.3) do not support the eol attribute. Instead, the line endings for checked out files are influenced by various factors, including the value of core.autocrlf.

From a brief check, our .vcproj files have svn:eol-style native. We should look to have both Git and SVN match. Additionally, we should either consider requiring a minimum version of Git (that support the eol attribute) and/or  teach svn-apply/svn-unapply how to compensate for older Git versions.
Comment 11 David Levin 2011-04-09 22:29:23 PDT
Created attachment 88946 [details]
Patch
Comment 12 Adam Barth 2011-04-09 22:40:25 PDT
Comment on attachment 88946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88946&action=review

That's horrible, but also amazing.

> Tools/Scripts/svn-apply:335
> +            if (basename($fullPath) =~ "\.vcproj\$" || basename($fullPath) =~ "\.vsprops\$") {

I think there's some sneaky way of writing regular expressions in perl that avoids the need to \ the $.
Comment 13 David Levin 2011-04-09 23:00:43 PDT
Comment on attachment 88946 [details]
Patch

Committed as http://trac.webkit.org/changeset/83394.

Also I found the way of not escaping the $ use // instead of "" (I don't speak perl colloquially.)
Comment 14 Daniel Bates 2011-04-09 23:07:59 PDT
I know this patch has already been reviewed and landed in <http://trac.webkit.org/changeset/83394>.

I wanted to add some remarks. The patch is incorrect and will break svn-apply/unapply for all people with an SVN checkout on a Unix file system (this includes SVN under a default installation of Cygwin). This is because the Visual Studio project files have been checked into the repository with Unix line endings and have svn:eol-style: native. So, these files will have Unix new lines in an SVN checkout on a Unix file system.

Note, this patch will fix the issue for people who use Git version greater than 1.7.0.3. Since the commit/EWS bots seem to using this version of Git, this patch will resolve the issue. At the very least, there should be a FIXME comment.
Comment 15 David Levin 2011-04-09 23:18:06 PDT
Reverted r83394 for reason:

Patch

Committed r83395: <http://trac.webkit.org/changeset/83395>
Comment 16 Adam Barth 2011-04-09 23:21:49 PDT
It was foolish of me to review this patch without waiting for dbates.  I just got so excited about the prospect of fixing this bug.  Sorry Dan.
Comment 17 David Levin 2011-04-09 23:22:21 PDT
(In reply to comment #14)
>  The patch is incorrect and will break svn-apply/unapply for all people with an SVN checkout on a Unix file system (this includes SVN under a default installation of Cygwin). ...

Thanks for the info. Rolled out. http://trac.webkit.org/changeset/83395

I suppose patch could be more sophisticated to look at the very first line ending of the file and use that to decided if we are dealing with DOS endings or not.

Would that work?

I really want to get this fixed as I hear about people hitting this often enough (and then I get pings to help land patches....)
Comment 18 Daniel Bates 2011-04-09 23:30:46 PDT
(In reply to comment #17)
> (In reply to comment #14)
> >  The patch is incorrect and will break svn-apply/unapply for all people with an SVN checkout on a Unix file system (this includes SVN under a default installation of Cygwin). ...
> 
> Thanks for the info. Rolled out. http://trac.webkit.org/changeset/83395
> 
> I suppose patch could be more sophisticated to look at the very first line ending of the file and use that to decided if we are dealing with DOS endings or not.

I take it you mean svn-apply not patch. Yes, this is the approach I'm taking.

> 
> Would that work?

I believe it will work.

> 
> I really want to get this fixed as I hear about people hitting this often enough (and then I get pings to help land patches....)

I agree. For completeness, I am currently working on a fix for this issue and hope to post it tomorrow (04/10) / Monday. That being said, I would prefer we fix this as soon as possible. So, feel free to post an updated patch or a temporary workaround that we can use.
Comment 19 David Levin 2011-04-09 23:33:35 PDT
(In reply to comment #18)
> I take it you mean svn-apply not patch. Yes, this is the approach I'm taking.
s/patch/my patch/ for my comment :)
Comment 20 David Levin 2011-04-09 23:44:29 PDT
Created attachment 88947 [details]
Patch
Comment 21 Daniel Bates 2011-04-10 00:03:03 PDT
Comment on attachment 88947 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88947&action=review

This patch is OK. That being said, we will also need to update svn-unapply and, if possible, add unit tests for this change. Towards this, we should look to extract the file EOL logic into a function in VCSUtils.pm so that it can be shared by both svn-apply/unapply.

I suggest that we add a FIXME comment above your change to indicate that we should add similar functionality to svn-unapply and/or extract some of this code and move it to VCSUtils.pm.

> Tools/Scripts/svn-apply:339
> +                # Think of it as escaping. Patch will convert \r\n to \n, so \r\r\n becomes \r\n.

Nit: Patch => Patch(1)
Comment 22 David Levin 2011-04-10 00:13:19 PDT
Comment on attachment 88947 [details]
Patch

After discussing this more with Daniel, this patch would work ok, but has issues that should be addressed and he has a patch that would be more comprehensive (that should come on Monday and no one will probably hit this much during the weekend, so we'll ditch this version).
Comment 23 Daniel Bates 2011-04-11 02:13:46 PDT
Created attachment 88978 [details]
Work-in-progress patch

Work-in-progress patch. Needs change log and some more unit tests. Will continue working on it today (04/11).
Comment 24 Daniel Bates 2011-04-11 12:40:13 PDT
Created attachment 89055 [details]
Patch and unit tests
Comment 25 WebKit Review Bot 2011-04-11 12:45:20 PDT
Attachment 89055 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Daniel Bates 2011-04-11 12:52:37 PDT
(In reply to comment #25)
> [...]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
> Total errors found: 4 in 5 files

The presence of these tab characters on these lines is expected since these lines are part of sample diff headers.
Comment 27 Eric Seidel (no email) 2011-04-11 13:04:12 PDT
Comment on attachment 89055 [details]
Patch and unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=89055&action=review

> Tools/Scripts/VCSUtils.pm:454
> +sub parseEOL($)

Maybe parseFirstEOL?

> Tools/Scripts/VCSUtils.pm:872
> +sub parseDiff($$;$)

No clue what your fancy semi-colon does.
Comment 28 Daniel Bates 2011-04-11 13:46:05 PDT
(In reply to comment #27)
> (From update of attachment 89055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89055&action=review
> 
> > Tools/Scripts/VCSUtils.pm:454
> > +sub parseEOL($)
> 
> Maybe parseFirstEOL?

Will change.

> 
> > Tools/Scripts/VCSUtils.pm:872
> > +sub parseDiff($$;$)
> 
> No clue what your fancy semi-colon does.

As per <http://perldoc.perl.org/perlsub.html#Prototypes>: "A semicolon (;) separates mandatory arguments from optional arguments."
Comment 29 Daniel Bates 2011-04-11 13:57:51 PDT
Created attachment 89081 [details]
Patch and unit tests

Renamed parseEOL() to parseFirstEOL() as suggested by Eric Seidel.

Also, renamed fileEOL() to firstEOLInFile() in VCSUtils.pm and parseEOLWithString() to firstEOLInString() in parseEOL.pl so as to be more descriptive.
Comment 30 WebKit Review Bot 2011-04-11 14:00:42 PDT
Attachment 89081 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Daniel Bates 2011-04-11 14:02:52 PDT
Created attachment 89082 [details]
Patch and unit tests

Renamed unit test file parseEOL.pl to parseFirstEOL.pl to reflect the name change of the function it's testing.
Comment 32 WebKit Review Bot 2011-04-11 14:06:05 PDT
Attachment 89082 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:66:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:67:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:74:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffWithMockFiles.pl:75:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Eric Seidel (no email) 2011-04-11 15:02:56 PDT
Comment on attachment 89082 [details]
Patch and unit tests

I'm willing to rubber-stamp this.  If you need a more indepth review, you'll need to consult with Darin or Adam Roben or someone who speaks perl better than I.
Comment 34 Daniel Bates 2011-04-12 10:18:54 PDT
Committed r83603: <http://trac.webkit.org/changeset/83603>