Bug 79509 - Bash scripts should support LF endings only
Summary: Bash scripts should support LF endings only
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 78953
  Show dependency treegraph
 
Reported: 2012-02-24 10:49 PST by Ashod Nakashian
Modified: 2012-03-09 12:30 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.74 KB, patch)
2012-02-24 21:35 PST, Ashod Nakashian
ddkilzer: review-
ddkilzer: commit-queue-
Details | Formatted Diff | Diff
Patch to convert all bash scripts to LF eol-style and mark them as executable. (18.18 KB, patch)
2012-02-29 00:50 PST, Ashod Nakashian
ddkilzer: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Convert all bash scripts to LF eol-style and mark them as executable (17.78 KB, patch)
2012-03-08 04:41 PST, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Convert all bash scripts to LF eol-style and mark them as executable (17.77 KB, patch)
2012-03-08 06:04 PST, Ashod Nakashian
ddkilzer: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ashod Nakashian 2012-02-24 10:49:38 PST
Some bash scripts have svn:eol-style property set to LF while others have it set to Native, which on Windows is CRLF. This is causing issues on bash versions that find CR unexpected.

This bug is to set svn:eol-style to LF on all bash scripts (and possibly consider python and perl scripts as well). This is the most common denominator and should avoid potential issues.
Comment 1 Ashod Nakashian 2012-02-24 21:35:28 PST
Created attachment 128849 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2012-02-28 07:51:57 PST
Comment on attachment 128849 [details]
Patch

Please create the patch using ./Tools/Scripts/svn-create-patch so that the property changes are included in the patch.
Comment 3 Ashod Nakashian 2012-02-29 00:50:58 PST
Created attachment 129406 [details]
Patch to convert all bash scripts to LF eol-style and mark them as executable.

Created the patch using svn-create-patch as suggested David Kilzer. In addition to fixing eol-style to LF, I've marked all bash scripts (with extension sh) as executable as some had the property while others didn't.
Comment 4 David Kilzer (:ddkilzer) 2012-03-05 09:47:23 PST
Comment on attachment 129406 [details]
Patch to convert all bash scripts to LF eol-style and mark them as executable.

r=me
Comment 5 WebKit Review Bot 2012-03-05 10:59:31 PST
Comment on attachment 129406 [details]
Patch to convert all bash scripts to LF eol-style and mark them as executable.

Rejecting attachment 129406 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Kilz..." exit_code: 9 cwd: /mnt/git/webkit-commit-queue/

Failed to find the property value for the SVN property "svn:eol-style": "## -0,0 +1 ##
". at /mnt/git/webkit-commit-queue/Tools/Scripts/VCSUtils.pm line 1184, <ARGV> line 28.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Kilz..." exit_code: 9 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11821005
Comment 6 Eric Seidel (no email) 2012-03-08 01:59:04 PST
It looks like you may be hitting the same issue described in bug 80104.

I think svn-apply regressed at some point.  Sadly our most recent svn-apply maintainer has gone off to other things.

Who uses SVN anymore anyway? ;p
Comment 7 Ashod Nakashian 2012-03-08 04:06:16 PST
(In reply to comment #6)
> It looks like you may be hitting the same issue described in bug 80104.
> 
> I think svn-apply regressed at some point.  Sadly our most recent svn-apply maintainer has gone off to other things.

So are you saying generating the diff with git will fix the issue and will make the style server happy?

> 
> Who uses SVN anymore anyway? ;p

I was under the impression that although many use git, svn was still the de facto repo and use-case. Either that, or we're maintaining two repos and vc's when one is practically all that's used.
Comment 8 David Barr 2012-03-08 04:17:11 PST
(In reply to comment #5)
This is because svn 1.7 changed to unidiff output for property changes:
  http://subversion.apache.org/docs/release-notes/1.7.html#diff-properties
For property additions, conversion to the 1.6 format is as simple as stripping the header.
Is it possible to simply re-upload the patch with the svn-1.7 property diff headers removed? 
ie. strip all lines '## -0,0 +1 ##' from the patch.
Alternatively, one could regenerate the patch with svn 1.6.
Comment 9 Ashod Nakashian 2012-03-08 04:41:23 PST
Created attachment 130805 [details]
Convert all bash scripts to LF eol-style and mark them as executable

Per David's suggestion, stripped SVN 1.7 specific lines from the diff to make the style checker happy.
Comment 10 Ashod Nakashian 2012-03-08 06:04:12 PST
Created attachment 130812 [details]
Convert all bash scripts to LF eol-style and mark them as executable

Another fix to the patch to make it compatible with SVN 1.6 diff.
Comment 11 David Kilzer (:ddkilzer) 2012-03-08 14:23:30 PST
Comment on attachment 130812 [details]
Convert all bash scripts to LF eol-style and mark them as executable

r=me

Let's try this again!
Comment 12 WebKit Review Bot 2012-03-08 15:27:12 PST
Comment on attachment 130812 [details]
Convert all bash scripts to LF eol-style and mark them as executable

Rejecting attachment 130812 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
9.
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Kilz..." exit_code: 9 cwd: /mnt/git/webkit-commit-queue/

Failed to find the property value for the SVN property "svn:eol-style": "+LF
". at /mnt/git/webkit-commit-queue/Tools/Scripts/VCSUtils.pm line 1184, <ARGV> line 28.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Kilz..." exit_code: 9 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11903151
Comment 13 Daniel Bates 2012-03-08 20:25:51 PST
(In reply to comment #12)
> (From update of attachment 130812 [details])
> Rejecting attachment 130812 [details] from commit-queue.
> [...]
> Failed to find the property value for the SVN property "svn:eol-style": "+LF
> ". at /mnt/git/webkit-commit-queue/Tools/Scripts/VCSUtils.pm line 1184, <ARGV> line 28.

This patch will need to be landed by hand since svn-apply, which is used by the commit-queue to apply a patch to a working copy, only supports the svn:executable property. We have a FIXME in VCSUtils to recognize other SVN properties, <http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm?rev=106054#L1102>. We should also look to recognize the git equivalent of svn:eol-style, see gitattribute(5) <http://schacon.github.com/git/gitattributes.html>.
Comment 14 David Kilzer (:ddkilzer) 2012-03-08 22:04:08 PST
Landing locally on Lion did not work either:

$ ./Tools/Scripts/webkit-patch land-from-bug 79509
Fetching: https://bugs.webkit.org/show_bug.cgi?id=79509&ctype=xml
1 reviewed patch found on bug 79509.
Processing 1 patch from 1 bug.
Updating working directory
Updating OpenSource
U    Source/JavaScriptCore/wtf/url/api/ParsedURL.h
U    Source/JavaScriptCore/ChangeLog
U    Source/WebCore/ChangeLog
U    Source/WebCore/platform/KURLWTFURLImpl.h
Updated to revision 110261.
Updating Internal
At revision 41613.
Processing patch 130812 from bug 79509.
Reading Keychain for bugs.webkit.org account and password.  Click "Allow" to continue...
Logging in as ddkilzer@webkit.org...
Failed to run "[u'/Volumes/Data/OpenSource/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Kilzer']" exit_code: 25 cwd: /Volumes/Data/OpenSource

Failed to find the property value for the SVN property "svn:eol-style": "+LF
". at /Volumes/Data/OpenSource/Tools/Scripts/VCSUtils.pm line 1184, <ARGV> line 28.

Failed to run "[u'/Volumes/Data/OpenSource/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Kilzer']" exit_code: 25 cwd: /Volumes/Data/OpenSource



$ ./Tools/Scripts/svn-apply ~/Downloads/patch.txt 
Failed to find the property value for the SVN property "svn:eol-style": "+LF
". at /Volumes/Data/OpenSource/Tools/Scripts/VCSUtils.pm line 1184, <ARGV> line 28.
Comment 15 David Kilzer (:ddkilzer) 2012-03-08 22:05:15 PST
(In reply to comment #13)
> This patch will need to be landed by hand since svn-apply, which is used by the commit-queue to apply a patch to a working copy, only supports the svn:executable property.

Doh, just read the "landed by hand" part.  I won't have time to do this until after next week if anyone else wants to try.
Comment 16 Daniel Bates 2012-03-08 23:38:17 PST
(In reply to comment #15)
> (In reply to comment #13)
> > This patch will need to be landed by hand since svn-apply, which is used by the commit-queue to apply a patch to a working copy, only supports the svn:executable property.
> 
> Doh, just read the "landed by hand" part.  I won't have time to do this until after next week if anyone else wants to try.

I'll look to land it tomorrow unless someone else does it before then.
Comment 17 Ashod Nakashian 2012-03-09 03:10:54 PST
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > This patch will need to be landed by hand since svn-apply, which is used by the commit-queue to apply a patch to a working copy, only supports the svn:executable property.
> > 
> > Doh, just read the "landed by hand" part.  I won't have time to do this until after next week if anyone else wants to try.
> 
> I'll look to land it tomorrow unless someone else does it before then.

Thanks David and Daniel. I'll look into the VCSUtils script over the weekend to see if I can hack the missing property support.
Comment 18 Daniel Bates 2012-03-09 11:00:41 PST
Comment on attachment 130812 [details]
Convert all bash scripts to LF eol-style and mark them as executable

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Will remove before landing.
Comment 19 Daniel Bates 2012-03-09 11:09:23 PST
Committed r110306: <http://trac.webkit.org/changeset/110306>
Comment 20 Daniel Bates 2012-03-09 12:30:37 PST
(In reply to comment #19)
> Committed r110306: <http://trac.webkit.org/changeset/110306>

For some reason webkit-patch land 79509 committed only the changes to the ChangeLog files.

Committed the property changes in <http://trac.webkit.org/changeset/110318>.