| 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 / Tests | Assignee: | Daniel Bates <dbates> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | abarth, abecsi, aroben, carol, ddkilzer, eric, gustavo, 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
Daniel Bates
2011-02-02 13:43:47 PST
Also observed in bug #53573. 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. (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. 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) On Qt EWS now we use patch 2.5.9. After upgrading to Squeeze we will use 2.6. (in 2-3 weeks) 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. 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). 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. 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. (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. Created attachment 88946 [details]
Patch
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 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.) 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. Reverted r83394 for reason: Patch Committed r83395: <http://trac.webkit.org/changeset/83395> 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. (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....) (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. (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 :) Created attachment 88947 [details]
Patch
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 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).
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).
Created attachment 89055 [details]
Patch and unit tests
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.
(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 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. (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." 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.
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.
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.
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 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.
Committed r83603: <http://trac.webkit.org/changeset/83603> |