Description
Peter Kasting
2009-07-15 17:15:55 PDT
Created attachment 32890 [details]
Fixes, part 1
Found a few more script issues with the scripts to prepare a change, so here's a patch that fixes those and sets the eol-style for shell scripts. This does not update all the build steps to conditionally set the path; I figured including that in this change would just make it hard to review. That patch will come next.
I created this patch using the modified copies of prepare-ChangeLog and svn-create-patch here, so they at least work for me now :)
Comment on attachment 32890 [details] Fixes, part 1 > +++ WebKitTools/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2009-07-16 Peter Kasting <pkasting@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=27323 > + Improve support for non-Cygwin SVNs on Windows, as well as WebKit > + checkouts hosted inside other checkouts. > + > + * Scripts/prepare-ChangeLog: Fix a case of adding "\n" to the ChangeLog without normalizing. Normalize file paths early instead of late so all stages of the script work. Modify regexes so that trailing whitespace (e.g. \r) isn't included in filenames. > + * Scripts/svn-create-patch: Use a regex instead of chomp so we cut off line endings even if they don't match Perl's. Determine SVN root by looking for Repository Root string and aborting when it's missing or different than what we've already seen. Please wrap these long lines. > @@ -338,7 +338,7 @@ foreach my $prefix (sort keys %files) { > $bugDescription = "Need a short description and bug URL (OOPS!)" unless $bugDescription; > print CHANGE_LOG normalizeLineEndings(" $bugDescription\n", $endl) if $bugDescription; > print CHANGE_LOG normalizeLineEndings(" $bugURL\n", $endl) if $bugURL; > - print CHANGE_LOG "\n"; > + print CHANGE_LOG normalizeLineEndings("\n", $endl); Seems like all of these lines could just use $endl instead of \n and then they wouldn't have to call normalizeLineEndings at all. > @@ -1289,7 +1289,7 @@ sub findOriginalFileFromSvn($) > my $baseUrl; > open INFO, "$SVN info . |" or die; > while (<INFO>) { > - if (/^URL: (.+)/) { > + if (/^URL: (.+\S+)\s*/) { > $baseUrl = $1; > } Do we still need the .+ part of this regex? Seems like its only purpose at this point is to allow leading spaces, which doesn't seem like something we want. Should we use \r?\n$ at the end instead of \s* ? That seems a little clearer about the intent. Ditto for the other similar changes you made to regexes. > @@ -467,10 +471,26 @@ sub determineSvnRoot() > my $path = '.'; > my $parent = '../'; > my $devnull = File::Spec->devnull(); > - my $exitCode; > + my $repositoryRoot; > while (1) { > - $exitCode = system("svn info $path 2> $devnull > $devnull")/256; > - last if $exitCode; > + my $thisRoot; > + open INFO, "svn info '$path' |" or die; > + while (<INFO>) { > + if (/^Repository Root: (.+)/) { > + $thisRoot = $1; > + } > + } > + close INFO; > + > + # It's possible (e.g. for developers of some ports) to have a WebKit > + # checkout in a subdirectory of another checkout. So abort if the > + # repository root suddenly changes. > + last if !$thisRoot; > + if (!$repositoryRoot) { > + $repositoryRoot = $thisRoot; > + } > + last if $thisRoot ne $repositoryRoot; > + This seems like a very separate fix from the rest of this patch. Could you split it out into its own change? r=me if you do the determineSvnRoot change separately. You should get Dave Kilzer to review that part. (In reply to comment #2) > Do we still need the .+ part of this regex? Seems like its only purpose at this > point is to allow leading spaces, which doesn't seem like something we want. Yes: A filename can have spaces in the middle ("Foo Bar Baz"). > Should we use \r?\n$ at the end instead of \s* ? That seems a little clearer > about the intent. Ditto for the other similar changes you made to regexes. I wasn't sure in all cases whether the \n had already been stripped. I could use \r?\n? though. > This seems like a very separate fix from the rest of this patch. Could you > split it out into its own change? Sure. Will fix things and check in. (In reply to comment #2) > Seems like all of these lines could just use $endl instead of \n and then they > wouldn't have to call normalizeLineEndings at all. I went to change this but then realized: * Some of the variables here could contain embedded newlines * Just wrapping all print statements is clearer (and more likely to convince future editors to do the same) than wrapping some things but not others So I elected not to make that change. Comment on attachment 32890 [details] Fixes, part 1 Clearing flags since I landed a modified version of this in r44993. Created attachment 32899 [details]
Fixes, part 2
ddkilzer: aroben suggested you were the right person to review this.
The motivation behind this is that it's possible for at least Chromium (perhaps other ports) to have a layered checkout, with the top level being from the Chromium repo and a subdirectory ("WebKit/") being from the WebKit repo. This makes it possible to test WK changes directly in a Chromium build and then submit without having to copy things over to a separate checkout (error-prone). This works well except that svn-create-patch gets confused about the checkout root, so here I modify the algorithm to halt when we see a different Repository Root than we started with (or no root at all, if we're in a standalone WK checkout).
(In reply to comment #6) > Created an attachment (id=32899) [details] > Fixes, part 2 > > ddkilzer: aroben suggested you were the right person to review this. > > The motivation behind this is that it's possible for at least Chromium (perhaps > other ports) to have a layered checkout, with the top level being from the > Chromium repo and a subdirectory ("WebKit/") being from the WebKit repo. This > makes it possible to test WK changes directly in a Chromium build and then > submit without having to copy things over to a separate checkout (error-prone). > This works well except that svn-create-patch gets confused about the checkout > root, so here I modify the algorithm to halt when we see a different Repository > Root than we started with (or no root at all, if we're in a standalone WK > checkout). Oops. See Bug 26999 Comment #12. Had I known this was an issue for Chromium, I would have insisted the fix be done for that bug. Comment on attachment 32899 [details] Fixes, part 2 > my $path = '.'; > my $parent = '../'; > my $devnull = File::Spec->devnull(); > - my $exitCode; > + my $repositoryRoot; > while (1) { > - $exitCode = system("svn info $path 2> $devnull > $devnull")/256; > - last if $exitCode; > + my $thisRoot; > + open INFO, "svn info '$path' |" or die; > + while (<INFO>) { > + if (/^Repository Root: (.+)/) { > + $thisRoot = $1; > + } > + } > + close INFO; > + > + # It's possible (e.g. for developers of some ports) to have a WebKit > + # checkout in a subdirectory of another checkout. So abort if the > + # repository root suddenly changes. > + last if !$thisRoot; > + if (!$repositoryRoot) { > + $repositoryRoot = $thisRoot; > + } > + last if $thisRoot ne $repositoryRoot; > + > $last = $path; > $path = $parent . $path; > } This change looks fine. I notice another change that I missed in Bug 26999 that could be made to make the code a bit more Windows- (or non-UNIX-) friendly: - my $parent = '../'; + my $parent = '..'; - $path = $parent . $path; + $path = File::Spec->catdir($parent, $path); Feel free to make this change if you'd like, but it's not necessary. r=me Comment on attachment 32899 [details] Fixes, part 2 Landed in r45998, clearing flags. Created attachment 32904 [details]
Fixes, part 3
While this patch looks large, it's just a search-and-replace of:
set PATH=%SystemDrive%\cygwin\bin;%PATH%
...with:
%SystemDrive%\cygwin\bin\which.exe bash
if errorlevel 1 set PATH=%SystemDrive%\cygwin\bin;%PATH%
There is one place I didn't touch: JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore.make. I wasn't sure what the right syntax in here was (I don't know makefiles) or even what this makefile was used for. Feel free to give direction.
(In reply to comment #9) > (From update of attachment 32899 [details]) > Landed in r45998, clearing flags. Haha, like David we didn't think nested SVN repositories was common. Your fix looks like exactly what I would have done. Thanks for doing this =). A little nitpick. You could break out of the <INFO> loop once you've gotten the data you want: > 769 while (<INFO>) { > 480 if (/^Repository Root: (.+)/) { > 481 $thisRoot = $1; > +++ last; > 482 } > 483 } But the loop should be nice and quick anyways, so its nothing major. Cheers. With "part 2" landed, Subversion on Mac always complains: $ svn-create-patch JavaScriptCore svn: Path '..' ends in '..', which is unsupported for this operation (In reply to comment #11) > Haha, like David we didn't think nested SVN repositories was common. Your fix > looks like exactly what I would have done. Thanks for doing this =). > > A little nitpick. You could break out of the <INFO> loop once you've gotten > the data you want: > > > 769 while (<INFO>) { > > 480 if (/^Repository Root: (.+)/) { > > 481 $thisRoot = $1; > > +++ last; > > 482 } > > 483 } > > But the loop should be nice and quick anyways, so its nothing major. Actually, you don't want to stop consuming <INFO> because you could get "broken pipe" warnings from svn. Instead, you'd want to consume the rest of <INFO> doing something like this before "last" (untested): { local $/ = undef; <INFO>; } (In reply to comment #12) > With "part 2" landed, Subversion on Mac always complains: > > $ svn-create-patch JavaScriptCore > svn: Path '..' ends in '..', which is unsupported for this operation That's the end-condition when it reaches the first parent directory that's not in svn. Peter, we should redirect STDERR to /dev/null in this case (or consume it and ignore it): open INFO, "svn info '$path' 2> /dev/null |" or die; (In reply to comment #14) > Peter, we should redirect STDERR to /dev/null in this case (or consume it and > ignore it): > > open INFO, "svn info '$path' 2> /dev/null |" or die; Doing this, except using $devnull (conveniently defined in that function) instead of /dev/null. I'm also changing "\r?\n" to be "\r?\n?" in a couple places where it seems like that'd be safer. I'm not planning to try and skip the rest of <INFO> once we've parsed the repository root, because there are only a few input lines to read, and I'm not confident in my nonexistent perl skills to not screw something up. I have also noticed that svn-create-patch is producing bogus patches for me, that have extra \rs in some places (specifically, where we're removing lines from a file), which makes the patches not apply/unapply cleanly. I'm going to try and figure this out too so I can just post one patch for review. If I can't figure it out pretty soon I'll just post what I have anyway so Mac people don't start getting worried over this error spew. > Actually, you don't want to stop consuming <INFO> because you could get "broken > pipe" warnings from svn. Instead, you'd want to consume the rest of <INFO> > doing something like this before "last" (untested): > > { local $/ = undef; <INFO>; } Ahh, good to know. I didn't know svn would complain about that (I hadn't tested with svn). Slurping is certainly the next best thing. (tested and works). (In reply to comment #14) > Peter, we should redirect STDERR to /dev/null in this case (or consume it and > ignore it): > > open INFO, "svn info '$path' 2> /dev/null |" or die; There are multiple places that use /dev/null directly. I originally added (and its still in the code but unused): my $devnull = File::Spec->devnull(); In your opinion Is it worth using File::Spec for this? Does that make it more cross-platform compatible? If not then the line I mention above should be removed. [MIDAIR Collision, cool feature Bugzilla!!] - Seeing as Peter is going to use $devnull. Should this change be rolled out to other scripts using '/dev/null' directly? (In reply to comment #16) > There are multiple places that use /dev/null directly. I originally added (and > its still in the code but unused): > > my $devnull = File::Spec->devnull(); > > In your opinion Is it worth using File::Spec for this? Does that make it more > cross-platform compatible? If not then the line I mention above should be > removed. Yes, I'd say it could be done opportunistically, although if you want to do all of them at once, file a bug and attach a patch. :) Created attachment 32963 [details]
Fixes, part 4
This corrects the issues with svn-create-patch and adds the changes mentioned above.
The issue with me getting bogus patches seems to be an svn bug that I may have to work around by setting svn:eol-style on everything (ack).
Comment on attachment 32963 [details] Fixes, part 4 > +my $devnull = File::Spec->devnull(); I think $devNull would probably read a bit better since it's technically two words. (I would use $dev_null in Python.) > - $mimeType =~ s/\r?\n$//g; > + $mimeType =~ s/\r?\n?$//g; This pattern seems a bit strange since it could end up replacing nothing. I think this pattern might work better: $mimeType =~ s/[\r\n]+$//g; > - $line =~ s/\r?\n$//g; > + $line =~ s/\r?\n?$//; Ditto. > - print `svn cat ${sourceFile} | diff -u /dev/null - | tail -n +3`; > + print `svn cat ${sourceFile} | diff -u $devnull - | tail -n +3`; $devNull > + # Ignore error messages in case we've run past the root of the checkout. > + open INFO, "svn info '$path' 2> $devnull |" or die; $devNull > while (<INFO>) { > if (/^Repository Root: (.+)/) { > $thisRoot = $1; > + { local $/ = undef; <INFO>; } # Eat the rest of the input. > } > } I think "Consume" would be better than "Eat" here, unless you're feeling hungry. :) r=me > > + { local $/ = undef; <INFO>; } # Eat the rest of the input. > > I think "Consume" would be better than "Eat" here, unless you're feeling > hungry. :) For fun, I thought the official term for this was "Slurping": http://www.sysarch.com/Perl/slurp_article.html#fast%20slurping Comment on attachment 32963 [details] Fixes, part 4 Landed in r46054, clearing flags. After fix patch 3 lands, here are the issues I'm aware of: * update-webkit doesn't seem to call resolve-ChangeLogs when there's a ChangeLog conflict. * svn diff works badly on files w/o svn:eol-style, frequently producing patches that do not apply/unapply cleanly. * *CSSPropertyNames.in are not parsed correctly during the WebCoreGenerated build; the build terminates thinking that "\r" is a non-unique name. Not sure what solves the first bullet yet (haven't looked into it). The only solution I have to the second bullet is to try and apply/enforce svn:eol-style everywhere (it already exists on most files), which I'm not sure would be received well. The third bullet should be solvable with some changes to DerivedSources.make, makeprop.pl, and makevalues.pl; or I could apply eol-style=LF to *CSSPropertyNames.in. Not sure which is better. More issues I've found: * commit-log-editor produced a commit log with "../../../../../../" prepended to each section name when trying to check in patch 3 above. * resolve-Changelogs seems to remove \r from the "upstream" portion of a ChangeLog that originally had it (leaving the file with inconsistent line endings). * I screwed up my regexes in prepare-ChangeLog in a way that will work badly for 1-character filenames (not a big deal, but I should fix). I have fixes locally for the third bullet here and the first bullet in comment 22. Comment on attachment 32904 [details] Fixes, part 3 Landed in r46060, clearing flags. Created attachment 32981 [details]
Fixes, part 5
Fixes for a couple of the above issues
Comment on attachment 32981 [details]
Fixes, part 5
r=me
You might mention you "stole" normalizePath() from prepare-ChangeLog in the entry for update-webkit. :)
Comment on attachment 32981 [details] Fixes, part 5 Landed in r46064, clearing flags Created attachment 32992 [details]
Fixes, part 6
This makes resolve-ChangeLogs work significantly better for non-Cygwin SVNs.
One concern I have is my usage of --binary. Without this, the ChangeLog files were being converted from CRLF (how they were checked out) to LF by patch, except for the new local changes, which were preserved as CRLF, leaving the ChangeLogs in a mixed state. I _think_ it should be safe on all platforms to use --binary here to tell diff and patch to treat line endings as a black box and not modify them, but I admit it's not what happens in svn-create-patch, svn-apply, svn-unapply, etc., which (on my non-Cygwin SVN system) currently strip CRs from everywhere (resulting in files whose line endings are completely changed, which doesn't bother svn if the files have an svn:eol-style set). I'm concerned that if I add more usage of --binary to all these scripts that files checked out with eol-style "native" and then modified will result in patches which can't be cleanly applied on OSes different from the one which created them. By contrast, the changes here only affect the internal workings of resolve-ChangeLogs, so they can't cause such problems, unless the local ChangeLogs have mixed endings before running resolve-ChangeLogs.
What a mess.
Comment on attachment 32992 [details]
Fixes, part 6
r=me as long as this has been tested in a UNIX environment as well.
(In reply to comment #29) > (From update of attachment 32992 [details]) > r=me as long as this has been tested in a UNIX environment as well. I just had abarth check it on a Mac and he reported it worked fine for him, so I'll land. Comment on attachment 32992 [details] Fixes, part 6 Landed in r46095, clearing flags. Created attachment 33110 [details]
Fixes, part 7
This addresses a problem reported by Darin Adler (which was causing his patches to contain strings like "../../../../../../Filename.ext") and an inconsistency reported by Adam Barth.
Comment on attachment 33110 [details] Fixes, part 7 Landed in r46134, clearing flags Comment on attachment 32904 [details] Fixes, part 3 Un-obsoleting so I know this is no longer in the tree; it got backed out in r46139 since it caused bug 27468. Adding darin since ddkilzer noted he might object to my next patch. Created attachment 33131 [details]
Use a common determineSVNRoot() in multiple scripts
This implements abarth's suggestion of creating a centralized determineSVNRoot() that multiple scripts can call.
The main goal of this is to use it in commit-log-editor, where the old code resulted in commit logs like "../../../../../../JavaScriptCore:" in my hybrid checkout.
ddkilzer said that darin has expressed a preference for keeping svn-create-patch independent, which would imply copy-and-pasting this code instead of (or in addition to?) putting it in VCSUtils.pm. I can do either; I'm biased against copy-and-pasting unless it's important. Let me know.
Created attachment 33132 [details]
Only add Cygwin to PATH when necessary (attempt #2)
This is patch 3 plus "cmd /c" after the "if errorlevel 1 set PATH" call.
This shouldn't be committed until it's tested with someone at Apple who was having problems with patch 3 and found to solve the problem. (Personally, I'm somewhat skeptical.)
(In reply to comment #37) > Created an attachment (id=33132) [details] > Fixes, part 9 > > This is patch 3 plus "cmd /c" after the "if errorlevel 1 set PATH" call. Oops, this also has the patch above it contained within. I'll repost a clean version when I get in to work today. Created attachment 33186 [details]
Only add Cygwin to PATH when necessary (attempt #2)
Here's the cleaned version.
Created attachment 33219 [details]
Fix WebCore/DerivedSources.make "Duplicate value!" problem
This makes DerivedSources.make and make{prop,values}.pl a more permissive in what kind of line endings they'll expect coming out of WebCore/css/*CSSPropertyNames.in.
This may incidentally help the Apple developers who were having problems with the "only add Cygwin to PATH when necessary" patch; I'm not sure.
Created attachment 33220 [details]
Update auto-version.sh to handle different line endings
auto-version.sh had a couple different ways it could fail to deal with different line endings, so this makes it handle them all correctly.
It’s true that originally the svn-* scripts were not WebKit-specific in any way and had no dependency on the WebKit machinery. I think that was really great, and I’m sad that they are now becoming WebKit-specific. If that’s not absolutely necessary, I would love to avoid it. I don’t have anything else specific to add beyond that general design goal. (In reply to comment #42) > It’s true that originally the svn-* scripts were not WebKit-specific in any way > and had no dependency on the WebKit machinery. I think that was really great, > and I’m sad that they are now becoming WebKit-specific. If that’s not > absolutely necessary, I would love to avoid it. I'm kind of unclear on what you're saying so let me clarify. Are things non-WebKit-specific without my patch? If yes, it seems like using copy-and-paste (of this function) would address your concerns, right? I confess I don't understand the motivation or benefits for the design goal you're stating, but I'm not opposed to it either, so if that would be a better solution I'm happy to do that. (In reply to comment #43) > Are things non-WebKit-specific without my patch? I think so. A quick perusal doesn't show anything specific to the WebKit directory organization or project standards. I haven’t reviewed your patch, but if you’ve added dependency, on, say webkitdirs.pm, then that would be crossing a line in my mind. (In reply to comment #44) > I haven’t reviewed your patch, but if you’ve added dependency, on, say > webkitdirs.pm, then that would be crossing a line in my mind. I added a dependency on VCSUtils.pm, which also required FindBin magic. No dependency on webkitdirs.pm. Not sure where that stacks up for you. Feel free to glance over my patch if you want, it's the "Use a common determineSVNRoot()" one. (In reply to comment #45) > I added a dependency on VCSUtils.pm, which also required FindBin magic. No > dependency on webkitdirs.pm. More dependencies on libraries are regrettable, but the only alternative is ever-larger scripts with more and more code in each script, so this change seems OK. (In reply to comment #46) > (In reply to comment #45) > > I added a dependency on VCSUtils.pm, which also required FindBin magic. No > > dependency on webkitdirs.pm. > > More dependencies on libraries are regrettable, but the only alternative is > ever-larger scripts with more and more code in each script, so this change > seems OK. OK. Thanks for taking the time to clarify! Comment on attachment 33220 [details]
Update auto-version.sh to handle different line endings
ddkilzer suggests aroben to review this.
He also notes that "sed -r" isn't a recognized switch on OS X, but since this script is in win/tools/ hopefully that doesn't matter.
Comment on attachment 33131 [details] Use a common determineSVNRoot() in multiple scripts > Index: WebKitTools/Scripts/svn-create-patch > =================================================================== > --- WebKitTools/Scripts/svn-create-patch (revision 46134) > +++ WebKitTools/Scripts/svn-create-patch (working copy) > @@ -44,19 +44,21 @@ use strict; > use warnings; > > use Config; > -use Cwd; > +use Cwd (); # "use Cwd;" causes warnings if we later (indirectly) "use POSIX;" For consistency, please use "qw()" here in case anyone wants to import methods later and is confused by the bare parenthesis: use Cwd qw(); # "qw()" prevents warnings with "use POSIX;" below r=me Comment on attachment 33219 [details] Fix WebCore/DerivedSources.make "Duplicate value!" problem > - if sort $(WEBCORE_CSS_PROPERTY_NAMES) | uniq -d | grep -E '^[^#]'; then echo 'Duplicate value!'; exit 1; fi > + if sort $(WEBCORE_CSS_PROPERTY_NAMES) | uniq -d | grep -Ev '(^#)|(^[[:space:]]*$)'; then echo 'Duplicate value!'; exit 1; fi Nit: I generally like to put spaces between arguments (rather than merging them): "-E -v". This isn't necessary, though. > - if sort CSSValueKeywords.in | uniq -d | grep -E '^[^#]'; then echo 'Duplicate value!'; exit 1; fi > + if sort CSSValueKeywords.in | uniq -d | grep -Ev '(^#)|(^[[:space:]]*$)'; then echo 'Duplicate value!'; exit 1; fi > perl "$(WebCore)/css/makevalues.pl" Change this to "-E -v", too, if you change the above command. > Index: WebCore/css/makeprop.pl > =================================================================== > --- WebCore/css/makeprop.pl (revision 46178) > +++ WebCore/css/makeprop.pl (working copy) > @@ -26,9 +26,9 @@ use warnings; > open NAMES, "<CSSPropertyNames.in" || die "Could not find CSSPropertyNames.in"; > my @names = (); > while (<NAMES>) { > - next if (m/#/); > - chomp $_; > - next if ($_ eq ""); > + next if (m/(^#)|(^\s*$)/); > + # Input may use a different EOL sequence than $/, so avoid chomp. > + $_ =~ s/[\r\n]+$//g; > push @names, $_; > } > close(NAMES); I think it may be safest to put the \r\n replacement before the "next" line unless you're sure that Perl is going to treat all forms of line endings equally on all platforms when applying regular expressions. (I don't know the answer myself, but it's more common to chomp() before you try to match, so I'd follow that pattern anyway.) > Index: WebCore/css/makevalues.pl > =================================================================== > --- WebCore/css/makevalues.pl (revision 46178) > +++ WebCore/css/makevalues.pl (working copy) > @@ -26,9 +26,9 @@ use warnings; > open NAMES, "<CSSValueKeywords.in" || die "Could not open CSSValueKeywords.in"; > my @names = (); > while (<NAMES>) { > - next if (m/#/); > - chomp $_; > - next if ($_ eq ""); > + next if (m/(^#)|(^\s*$)/); > + # Input may use a different EOL sequence than $/, so avoid chomp. > + $_ =~ s/[\r\n]+$//g; > push @names, $_; > } > close(NAMES); Ditto. r=me with the changes to makeprop.pl and makevalues.pl. Comment on attachment 33131 [details] Use a common determineSVNRoot() in multiple scripts Landed in r46236, clearing flags Comment on attachment 33219 [details] Fix WebCore/DerivedSources.make "Duplicate value!" problem Landed in r46237 (got r+ from ddkilzer on IRC to not modify make*.pl), clearing flags. (In reply to comment #39) > Created an attachment (id=33186) [details] > Only add Cygwin to PATH when necessary (attempt #2) On IRC, jessieberlin reports that applying this patch results in the generated autoversion.h containing inline \rs. This is precisely what the other patch above (to auto-version.sh) is designed to correct, but supposedly applying that does not fix the problem. I am baffled. Comment on attachment 33220 [details] Update auto-version.sh to handle different line endings > - PROPOSEDVERSION=$(cat "$SRCPATH/VERSION") > + PROPOSEDVERSION=`cat $SRCPATH/VERSION | sed -r 's/(.*\S+)\s*/\1/'` Seems like the \s* part of the regex isn't needed, since regex matching is greedy (ditto for the other sed command). r=me Comment on attachment 33220 [details] Update auto-version.sh to handle different line endings Landed in r46424, clearing flags. Created attachment 34017 [details]
Handle varying line endings in svn-apply and svn-unapply
I didn't realize I never fixed these two scripts...
Comment on attachment 34017 [details] Handle varying line endings in svn-apply and svn-unapply > - if (/^Index: (.+)/) { > + if (/^Index: (.*\S+)\s*$/) { This change is not needed. $_ has already been stripped of newlines and carriage returns previously: while (<>) { s/([\n\r]+)$//mg; > - if (/^Index: (.*)/) { > + if (/^Index: (.*\S+)\s*$/) { This is also not needed for the same reason. r=me without the above two changes. Comment on attachment 34017 [details] Handle varying line endings in svn-apply and svn-unapply Landed in r46742, clearing flags. Created attachment 34084 [details]
Only add Cygwin to PATH when necessary
Updated patch in hopes of it applying cleanly to the current tree.
According to jessieberlin on #webkit, the latest patch on here still causes \r to be inserted inside autoversion.h for her clean and incremental builds on Windows in Apple's environment. Created attachment 34099 [details]
Strip line endings at more places in auto-version.sh
This is a blind attempt at fixing the problems Apple Windows engineers run into when applying my other patch. I suspect some internal script is either changing PRODUCTVERSION's contents or setting $RC_PROJECTSOURCEVERSION, and with my other patch applied the guilty party gets a \r ending. This should fix that.
Comment on attachment 34099 [details]
Strip line endings at more places in auto-version.sh
Looks reasonable. That script is kind of crazy.
Comment on attachment 34099 [details]
Strip line endings at more places in auto-version.sh
Clearing the commit-queue flag. The tool isn't smart enough to deal with bugs with many patches yet.
Comment on attachment 34099 [details] Strip line endings at more places in auto-version.sh Landed in r46872, clearing flags. Comment on attachment 34084 [details]
Only add Cygwin to PATH when necessary
Could we have a helper script which does this instead?
So we could call:
path_helper_script my:new:fancy:path:which:the:helper:script:will:do:fancy:stuff:to.
Created attachment 34526 [details]
Even more line-ending-stripping for auto-version.sh
I got jessieberlin to send me the (broken) autoversion.h generated in her checkout with the PATH patch on this bug applied. Lo and behold $(whoami) was returning a value with a trailing CR! So strip line endings from $(whoami) and $(date), too.
Comment on attachment 34526 [details] Even more line-ending-stripping for auto-version.sh Landed in r47012, clearing flags. (In reply to comment #66) > Created an attachment (id=34526) [details] > Even more line-ending-stripping for auto-version.sh After having landed this patch, I hear from jessieberlin that the Apple Windows build now works with my Cygwin patch that's still up for review. Created attachment 34584 [details]
More line-ending stripping for svn-create-patch
Ran into a case in svn-create-patch that I hadn't fixed yet when creating a patch today. The added file generated by "svn mv" wasn't being diffed properly.
Comment on attachment 34584 [details] More line-ending stripping for svn-create-patch > - if (/^URL: (.+)/) { > + if (/^URL: (.*\S+)\s*$/) { A simpler way to write this is: (.+?)\s*$ But I guess it's OK as is. I think stripping trailing whitespace is an unnecessarily subtle way to allow for arbitrary line endings, and is the kind of thing that requires a comment or function name to make it clear. I haven't seen it before, although I assume we've been doing this elsewhere, but with changes like this one we're making these scripts more subtle that I don't think future maintainers will have a way of understanding what they need to preserve or change. There's nothing in this regular expression that says "line endings" to me. r=me (In reply to comment #70) > I think stripping trailing whitespace is an unnecessarily subtle way to allow > for arbitrary line endings, and is the kind of thing that requires a comment or > function name to make it clear. > > I haven't seen it before, although I assume we've been doing this elsewhere, We have indeed put this same expression in a number of places. I agree it's kind of subtle. I stripped all whitespace (rather than just trailing \rs and \ns) to be more general, but perhaps that was a mistake. I could modify this patch, or write a followon patch, to change all the cases of this to something else; either the expression you propose above, or something else (perhaps "(.+?)[\r\n]*$" ?). Let me know what you think is the best route forward and I'll take it. Comment on attachment 34584 [details] More line-ending stripping for svn-create-patch Went ahead and landed in r47061. I'm happy to modify scripts in any way you like in a future patch. (In reply to comment #71) > We have indeed put this same expression in a number of places. I agree it's > kind of subtle. I stripped all whitespace (rather than just trailing \rs and > \ns) to be more general, but perhaps that was a mistake. I could modify this > patch, or write a followon patch, to change all the cases of this to something > else; either the expression you propose above, or something else (perhaps > "(.+?)[\r\n]*$" ?). Let me know what you think is the best route forward and > I'll take it. Adding a comment would be fine. A pattern that makes it clearer it's about line endings would also be fine. A variable name that makes it clear what the pattern is about would also work. Any of the above. Created attachment 34685 [details]
Change (.*\S+)\s*$ to (.+?)[\r\n]*$
OK, changed all the instances I could find to use an expression clearly related to stripping line-endings.
Comment on attachment 34685 [details] Change (.*\S+)\s*$ to (.+?)[\r\n]*$ Landed in r47154, clearing flags. For your change to WTFCommon.vsprops, it would be good to add a cmd /c to your change, since otherwise I think the build could error out if there were no other prebuild steps. Created attachment 34992 [details]
Only add Cygwin to PATH when necessary
Added cmd /c to WTFCommon.vsprops change.
Fixed in r47392. Looks like this landed in http://trac.webkit.org/changeset/47392. Closing bug. |