Bug 27323 - Better support for non-Cygwin SVN on Windows
Summary: Better support for non-Cygwin SVN on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-15 17:15 PDT by Peter Kasting
Modified: 2009-08-17 16:27 PDT (History)
7 users (show)

See Also:


Attachments
Fixes, part 1 (8.76 KB, patch)
2009-07-16 14:00 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fixes, part 2 (2.25 KB, patch)
2009-07-16 16:05 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fixes, part 3 (52.88 KB, patch)
2009-07-16 17:15 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fixes, part 4 (2.50 KB, patch)
2009-07-17 12:14 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fixes, part 5 (3.50 KB, patch)
2009-07-17 15:27 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fixes, part 6 (5.60 KB, patch)
2009-07-17 17:59 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fixes, part 7 (2.73 KB, patch)
2009-07-20 15:05 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Use a common determineSVNRoot() in multiple scripts (5.65 KB, patch)
2009-07-20 18:13 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Only add Cygwin to PATH when necessary (attempt #2) (58.89 KB, patch)
2009-07-20 18:34 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Only add Cygwin to PATH when necessary (attempt #2) (53.59 KB, patch)
2009-07-21 10:30 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Fix WebCore/DerivedSources.make "Duplicate value!" problem (3.11 KB, patch)
2009-07-21 15:28 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Update auto-version.sh to handle different line endings (1.44 KB, patch)
2009-07-21 15:29 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Handle varying line endings in svn-apply and svn-unapply (2.89 KB, patch)
2009-08-03 16:39 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Only add Cygwin to PATH when necessary (53.84 KB, patch)
2009-08-04 14:10 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Strip line endings at more places in auto-version.sh (1.91 KB, patch)
2009-08-04 16:41 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Even more line-ending-stripping for auto-version.sh (1.28 KB, patch)
2009-08-10 16:19 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
More line-ending stripping for svn-create-patch (1.39 KB, patch)
2009-08-11 12:53 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Change (.*\S+)\s*$ to (.+?)[\r\n]*$ (7.44 KB, patch)
2009-08-12 13:28 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Only add Cygwin to PATH when necessary (55.84 KB, patch)
2009-08-17 14:27 PDT, Peter Kasting
sfalken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-07-15 17:15:55 PDT
Right now there are a few issues with using a non-cygwin SVN:
* Various shell scripts are checked out with eol-style=native rather than eol-style=LF, confusing bash when svn doesn't interpret "native" on Windows as "LF"
* Some prebuild steps explicitly add cygwin to the path, which means scripts that call `svn` get the wrong SVN.

The solution to the first bullet is to change the existing eol-style on these scripts, and change their line endings.  One concern raised on IRC is whether Qt WebKit for Windows will still build here.  I think it should, as the problem mentioned was for "native perl" and the proposal is to only change the shell scripts, so I'm not too worried about this.

The solution to the second bullet is basically to find the .vsprops and .vcproj files that do an explicit "set PATH" and change that step to:

%SystemDrive%\cygwin\bin\which.exe bash
if errorlevel 1 set PATH=%SystemDrive%\cygwin\bin;%PATH%

For people with cygwin already in their path, this doesn't modify the path.  For people without it, this adds the default cygwin install location (just like the existing code does); people without cygwin in their path and without it in the default location aren't supported by either the old or new code.

I'll add more problems here as I encounter them.
Comment 1 Peter Kasting 2009-07-16 14:00:01 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 2 Adam Roben (:aroben) 2009-07-16 14:53:37 PDT
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.
Comment 3 Peter Kasting 2009-07-16 14:57:02 PDT
(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.
Comment 4 Peter Kasting 2009-07-16 15:06:07 PDT
(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 5 Peter Kasting 2009-07-16 15:57:04 PDT
Comment on attachment 32890 [details]
Fixes, part 1

Clearing flags since I landed a modified version of this in r44993.
Comment 6 Peter Kasting 2009-07-16 16:05:53 PDT
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).
Comment 7 David Kilzer (:ddkilzer) 2009-07-16 16:28:55 PDT
(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 8 David Kilzer (:ddkilzer) 2009-07-16 16:34:48 PDT
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 9 Peter Kasting 2009-07-16 16:46:13 PDT
Comment on attachment 32899 [details]
Fixes, part 2

Landed in r45998, clearing flags.
Comment 10 Peter Kasting 2009-07-16 17:15:19 PDT
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&#x0D;&#x0A;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.
Comment 11 Joseph Pecoraro 2009-07-16 20:09:16 PDT
(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.
Comment 12 Alexey Proskuryakov 2009-07-17 10:14:46 PDT
With "part 2" landed, Subversion on Mac always complains:

$ svn-create-patch JavaScriptCore
svn: Path '..' ends in '..', which is unsupported for this operation
Comment 13 David Kilzer (:ddkilzer) 2009-07-17 10:47:53 PDT
(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>; }
Comment 14 David Kilzer (:ddkilzer) 2009-07-17 10:49:33 PDT
(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;
Comment 15 Peter Kasting 2009-07-17 11:22:15 PDT
(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.
Comment 16 Joseph Pecoraro 2009-07-17 11:25:03 PDT
> 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?
Comment 17 David Kilzer (:ddkilzer) 2009-07-17 11:57:43 PDT
(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.  :)
Comment 18 Peter Kasting 2009-07-17 12:14:44 PDT
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 19 David Kilzer (:ddkilzer) 2009-07-17 12:58:27 PDT
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
Comment 20 Joseph Pecoraro 2009-07-17 13:16:13 PDT
> > +                { 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 21 Peter Kasting 2009-07-17 13:17:41 PDT
Comment on attachment 32963 [details]
Fixes, part 4

Landed in r46054, clearing flags.
Comment 22 Peter Kasting 2009-07-17 13:27:46 PDT
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.
Comment 23 Peter Kasting 2009-07-17 15:01:28 PDT
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 24 Peter Kasting 2009-07-17 15:03:23 PDT
Comment on attachment 32904 [details]
Fixes, part 3

Landed in r46060, clearing flags.
Comment 25 Peter Kasting 2009-07-17 15:27:29 PDT
Created attachment 32981 [details]
Fixes, part 5

Fixes for a couple of the above issues
Comment 26 David Kilzer (:ddkilzer) 2009-07-17 15:59:21 PDT
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 27 Peter Kasting 2009-07-17 16:02:02 PDT
Comment on attachment 32981 [details]
Fixes, part 5

Landed in r46064, clearing flags
Comment 28 Peter Kasting 2009-07-17 17:59:13 PDT
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 29 David Kilzer (:ddkilzer) 2009-07-17 19:22:07 PDT
Comment on attachment 32992 [details]
Fixes, part 6

r=me as long as this has been tested in a UNIX environment as well.
Comment 30 Peter Kasting 2009-07-18 11:45:04 PDT
(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 31 Peter Kasting 2009-07-18 11:50:38 PDT
Comment on attachment 32992 [details]
Fixes, part 6

Landed in r46095, clearing flags.
Comment 32 Peter Kasting 2009-07-20 15:05:40 PDT
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 33 Peter Kasting 2009-07-20 15:16:55 PDT
Comment on attachment 33110 [details]
Fixes, part 7

Landed in r46134, clearing flags
Comment 34 Peter Kasting 2009-07-20 17:02:16 PDT
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.
Comment 35 Peter Kasting 2009-07-20 18:09:10 PDT
Adding darin since ddkilzer noted he might object to my next patch.
Comment 36 Peter Kasting 2009-07-20 18:13:14 PDT
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.
Comment 37 Peter Kasting 2009-07-20 18:34:56 PDT
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.)
Comment 38 Peter Kasting 2009-07-21 10:04:27 PDT
(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.
Comment 39 Peter Kasting 2009-07-21 10:30:40 PDT
Created attachment 33186 [details]
Only add Cygwin to PATH when necessary (attempt #2)

Here's the cleaned version.
Comment 40 Peter Kasting 2009-07-21 15:28:06 PDT
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.
Comment 41 Peter Kasting 2009-07-21 15:29:22 PDT
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.
Comment 42 Darin Adler 2009-07-22 11:04:03 PDT
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.
Comment 43 Peter Kasting 2009-07-22 11:14:57 PDT
(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.
Comment 44 Darin Adler 2009-07-22 12:26:33 PDT
(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.
Comment 45 Peter Kasting 2009-07-22 12:37:03 PDT
(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.
Comment 46 Darin Adler 2009-07-22 12:43:40 PDT
(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.
Comment 47 Peter Kasting 2009-07-22 12:46:53 PDT
(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 48 Peter Kasting 2009-07-22 13:04:44 PDT
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 49 David Kilzer (:ddkilzer) 2009-07-22 13:05:05 PDT
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 50 David Kilzer (:ddkilzer) 2009-07-22 13:10:29 PDT
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 51 Peter Kasting 2009-07-22 13:33:07 PDT
Comment on attachment 33131 [details]
Use a common determineSVNRoot() in multiple scripts

Landed in r46236, clearing flags
Comment 52 Peter Kasting 2009-07-22 13:35:15 PDT
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.
Comment 53 Peter Kasting 2009-07-24 15:02:47 PDT
(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 54 Adam Roben (:aroben) 2009-07-27 11:39:51 PDT
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 55 Peter Kasting 2009-07-27 11:54:53 PDT
Comment on attachment 33220 [details]
Update auto-version.sh to handle different line endings

Landed in r46424, clearing flags.
Comment 56 Peter Kasting 2009-08-03 16:39:59 PDT
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 57 David Kilzer (:ddkilzer) 2009-08-03 17:07:03 PDT
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 58 Peter Kasting 2009-08-03 18:31:09 PDT
Comment on attachment 34017 [details]
Handle varying line endings in svn-apply and svn-unapply

Landed in r46742, clearing flags.
Comment 59 Peter Kasting 2009-08-04 14:10:48 PDT
Created attachment 34084 [details]
Only add Cygwin to PATH when necessary

Updated patch in hopes of it applying cleanly to the current tree.
Comment 60 Peter Kasting 2009-08-04 15:03:32 PDT
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.
Comment 61 Peter Kasting 2009-08-04 16:41:38 PDT
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 62 Adam Barth 2009-08-06 16:11:26 PDT
Comment on attachment 34099 [details]
Strip line endings at more places in auto-version.sh

Looks reasonable.  That script is kind of crazy.
Comment 63 Adam Barth 2009-08-06 16:16:33 PDT
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 64 Peter Kasting 2009-08-06 17:27:58 PDT
Comment on attachment 34099 [details]
Strip line endings at more places in auto-version.sh

Landed in r46872, clearing flags.
Comment 65 Eric Seidel 2009-08-06 18:27:32 PDT
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.
Comment 66 Peter Kasting 2009-08-10 16:19:08 PDT
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 67 Peter Kasting 2009-08-10 16:47:32 PDT
Comment on attachment 34526 [details]
Even more line-ending-stripping for auto-version.sh

Landed in r47012, clearing flags.
Comment 68 Peter Kasting 2009-08-11 10:23:19 PDT
(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.
Comment 69 Peter Kasting 2009-08-11 12:53:04 PDT
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 70 Darin Adler 2009-08-11 12:59:50 PDT
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
Comment 71 Peter Kasting 2009-08-11 13:17:10 PDT
(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 72 Peter Kasting 2009-08-11 14:14:31 PDT
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.
Comment 73 Darin Adler 2009-08-11 16:25:18 PDT
(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.
Comment 74 Peter Kasting 2009-08-12 13:28:29 PDT
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 75 Peter Kasting 2009-08-12 15:16:24 PDT
Comment on attachment 34685 [details]
Change (.*\S+)\s*$ to (.+?)[\r\n]*$

Landed in r47154, clearing flags.
Comment 76 Steve Falkenburg 2009-08-17 14:08:03 PDT
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.
Comment 77 Peter Kasting 2009-08-17 14:27:52 PDT
Created attachment 34992 [details]
Only add Cygwin to PATH when necessary

Added cmd /c to WTFCommon.vsprops change.
Comment 78 Peter Kasting 2009-08-17 15:12:40 PDT
Fixed in r47392.
Comment 79 Eric Seidel 2009-08-17 16:27:11 PDT
Looks like this landed in http://trac.webkit.org/changeset/47392.  Closing bug.