prepare-Changelog and svn-create-patch should optionally run check-webkit-style.

[James Craig]

prepare-changelog and svn-create-patch should automatically run check-webkit-style. I forget to do this probably half the time.

[James Craig]

Adding -s|--style param to both those scripts.

[James Craig]

Created attachment 228439 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/16506103>

[Alexey Proskuryakov]

Personally, I'd be willing to experiment with making it a default behavior. A change to default behavior would need to be discussed on webkit-dev, of course.

I'm not sure how many people use these scripts vs. webkit-patch. I use these and never use webkit-patch, but I believe that a lot of people do the opposite.

[James Craig]

Okay. I'll make that the default and create a new patch with a nullable flag. E.g. --nostyle if you want to disable the style checks.

[James Craig]

Created attachment 228562 [details]
patch

Added as default to prepare-Changelog, but couldn't figure out a good way to set as default in svn-create-patch without causing a loop. Lots of scripts call svn-create-patch, including check-webkit-style indirectly which causes a loop. Most of the time these are system calls instead of submodules (probably b/c the scripts are a mix of Python and Perl), so I could not use the "unless (caller)" conditional reliably. Seems fine to me to make it a flag. I just set up a bash alias for svn-create-patch to use the --style flag. I added --no-style in a few of the callers, too.

[Alexey Proskuryakov]

Thank you! Please do start a discussion on webkit-dev.

[James Craig]

Comment on attachment 228562 [details]
patch

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

> Tools/Scripts/prepare-ChangeLog:603
> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";

Perl veterans… Is this the recommended way to run a Python script from the same directory as the currently executing Perl script?

> Tools/Scripts/svn-create-patch:148
> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";

Ditto.

[Zoltan Horvath]

Comment on attachment 228562 [details]
patch

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

>> Tools/Scripts/prepare-ChangeLog:603
>> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";
> 
> Perl veterans… Is this the recommended way to run a Python script from the same directory as the currently executing Perl script?

What about

system("check-webkit-style") == 0 or die "Failed to check-webkit-style.";

like we have it in the svn-apply/svn-unapply scripts.

[James Craig]

Comment on attachment 228562 [details]
patch

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

>>> Tools/Scripts/prepare-ChangeLog:603
>>> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";
>> 
>> Perl veterans… Is this the recommended way to run a Python script from the same directory as the currently executing Perl script?
> 
> What about
> 
> system("check-webkit-style") == 0 or die "Failed to check-webkit-style.";
> 
> like we have it in the svn-apply/svn-unapply scripts.

It'd have to be the script in the current directory to work for me. Sometimes I have multiple SVN checkouts, and the system path you've suggested above would be a global, potentially masking some failure in the current directory if it was not the one used in $PATH.

[Zoltan Horvath]

Comment on attachment 228562 [details]
patch

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

>>>> Tools/Scripts/prepare-ChangeLog:603
>>>> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";
>>> 
>>> Perl veterans… Is this the recommended way to run a Python script from the same directory as the currently executing Perl script?
>> 
>> What about
>> 
>> system("check-webkit-style") == 0 or die "Failed to check-webkit-style.";
>> 
>> like we have it in the svn-apply/svn-unapply scripts.
> 
> It'd have to be the script in the current directory to work for me. Sometimes I have multiple SVN checkouts, and the system path you've suggested above would be a global, potentially masking some failure in the current directory if it was not the one used in $PATH.

Fair enough. The way you did it, seems fine!

> Tools/Scripts/svn-create-patch:81
> +my $checkWebKitStyle = 0;

I support make it default.

The change looks good to me in general.

[James Craig]

Comment on attachment 228562 [details]
patch

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

>> Tools/Scripts/svn-create-patch:81
>> +my $checkWebKitStyle = 0;
> 
> I support make it default.
> 
> The change looks good to me in general.

See comment #6. I'd like to leave this a flag for now and change the default if and when the infinite recursion edge cases can be worked out.

[Daniel Bates]

Comment on attachment 228562 [details]
patch

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

> Tools/ChangeLog:9
> +        Added as default for prepare-Changelog, but not in svn-create-patch, to avoid loops in recursive calls.

I take it that it isn't straightforward to audit all callers of svn-create-patch so as to eliminate recursive/duplicate calls? That is, modify callers to pass --no-style (as applicable) such that we don't recursively call check-webkit-style or call it more than once.

> Tools/Scripts/prepare-ChangeLog:75
> +sub checkWebKitStyle();

Please add this prototype declaration in sorted order according to the UNIX sort command.

> Tools/Scripts/prepare-ChangeLog:232
> +    # Run check-webkit-style.

This comment is unnecessary as it states exactly what the code is doing below. Please remove this comment.

>>>>> Tools/Scripts/prepare-ChangeLog:603
>>>>> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";
>>>> 
>>>> Perl veterans… Is this the recommended way to run a Python script from the same directory as the currently executing Perl script?
>>> 
>>> What about
>>> 
>>> system("check-webkit-style") == 0 or die "Failed to check-webkit-style.";
>>> 
>>> like we have it in the svn-apply/svn-unapply scripts.
>> 
>> It'd have to be the script in the current directory to work for me. Sometimes I have multiple SVN checkouts, and the system path you've suggested above would be a global, potentially masking some failure in the current directory if it was not the one used in $PATH.
> 
> Fair enough. The way you did it, seems fine!

Can't we use a similar approach as in Tools/Scripts/commit-log-editor and use $FindBin::Bin?

I don't see the value in having this function given that it's referenced exactly once in this script and it's straightforward to understand. I suggest we inline its implementation at the call site.

Additional remarks:

Most scripts call webkitdirs::chdirWebKit() at the start of a script to change the working directory to the WebKit checkout directory and then hardcode the path to the program relative to the WebKit checkout directory (e.g. Tools/Scripts/check-webkit-style). Obviously this approach has the disadvantage that we need to update this hardcoded relative path should our directory structure change (though this tends to be rare).

>>> Tools/Scripts/svn-create-patch:81
>>> +my $checkWebKitStyle = 0;
>> 
>> I support make it default.
>> 
>> The change looks good to me in general.
> 
> See comment #6. I'd like to leave this a flag for now and change the default if and when the infinite recursion edge cases can be worked out.

This is OK. It's unnecessary to explicitly initialize this variable to 0. By default a variable is initialized to undef, which evaluates to false in a boolean context. And this variable is used only in a boolean context throughout this script.

> Tools/Scripts/svn-create-patch:87
> +    "ignore-changelogs"    => \$ignoreChangelogs,

Since we're modifying this line please remove the extraneous whitespace in this line such that it reads:

"ignore-changelogs" => \$ignoreChangelogs,

> Tools/Scripts/svn-create-patch:88
> +    "style!" => \$checkWebKitStyle

I suggest that we add a comma at the end of this line so that a future patch that adds a command line option can avoid modifying this line (so as to add a comma at the end of it).

> Tools/Scripts/svn-create-patch:112
> +# Run check-webkit-style.

This comment is unnecessary as it states exactly what the code is doing below. Please remove this comment.

> Tools/Scripts/svn-create-patch:113
> +unless (caller) {

Did you mean to keep this given that this patch proposes that we don't run the style checker by default and the remark about reliability that you raised in comment 6? If you choose to keep this then I suggest we add a comment to explain the purpose of this conditional.

>> Tools/Scripts/svn-create-patch:148
>> +    system dirname(File::Spec->rel2abs(__FILE__)) . "/check-webkit-style";
> 
> Ditto.

Can't we use a similar approach as in Tools/Scripts/commit-log-editor and use $FindBin::Bin?

I don't see the value in having this function given that it's referenced exactly once in this script and it's straightforward to understand. I suggest we inline its implementation at the call site.

[James Craig]

Comment on attachment 228562 [details]
patch

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

Thanks for the review. I'll get those changes in an updated patch ASAP.

>> Tools/ChangeLog:9
>> +        Added as default for prepare-Changelog, but not in svn-create-patch, to avoid loops in recursive calls.
> 
> I take it that it isn't straightforward to audit all callers of svn-create-patch so as to eliminate recursive/duplicate calls? That is, modify callers to pass --no-style (as applicable) such that we don't recursively call check-webkit-style or call it more than once.

Yes, that's the approach I think we should probably take in a future patch, and the main reason I made the --style param nullable here.

>> Tools/Scripts/svn-create-patch:113
>> +unless (caller) {
> 
> Did you mean to keep this given that this patch proposes that we don't run the style checker by default and the remark about reliability that you raised in comment 6? If you choose to keep this then I suggest we add a comment to explain the purpose of this conditional.

I'll just get rid of it. I think called with --no-style is the better long-term solution.

[James Craig]

Comment on attachment 228562 [details]
patch

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

>>> Tools/ChangeLog:9
>>> +        Added as default for prepare-Changelog, but not in svn-create-patch, to avoid loops in recursive calls.
>> 
>> I take it that it isn't straightforward to audit all callers of svn-create-patch so as to eliminate recursive/duplicate calls? That is, modify callers to pass --no-style (as applicable) such that we don't recursively call check-webkit-style or call it more than once.
> 
> Yes, that's the approach I think we should probably take in a future patch, and the main reason I made the --style param nullable here.

Actually, there is only one more choke point (Tools/Scripts/webkitpy/common/checkout/scm/svn.py) I missed in the first patch, so I'll just make --style the default and nullify it there.

[James Craig]

Created attachment 228951 [details]
patch with review feedback

[Daniel Bates]

Comment on attachment 228951 [details]
patch with review feedback

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

> Tools/Scripts/svn-create-patch:112
> +    print STDERR "  Finished svn-create-patch. Running check-webkit-style.\n  ";

How did you come to the decision to emit "Finished svn-create-patch"? I mean, you didn't emit "Finished prepare-ChangeLog" in script Tools/Scripts/prepare-ChangeLog. I don't see much value in emitting such a message. Regardless, we should either omit such a message or emit a similar message in prepare-ChangeLog for consistency.

[James Craig]

Comment on attachment 228951 [details]
patch with review feedback

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

>> Tools/Scripts/svn-create-patch:112
>> +    print STDERR "  Finished svn-create-patch. Running check-webkit-style.\n  ";
> 
> How did you come to the decision to emit "Finished svn-create-patch"? I mean, you didn't emit "Finished prepare-ChangeLog" in script Tools/Scripts/prepare-ChangeLog. I don't see much value in emitting such a message. Regardless, we should either omit such a message or emit a similar message in prepare-ChangeLog for consistency.

I'm happy to omit it if you think that's best. The reason I didn't is because svn-create-patch has no default output, so I wanted to be clear why there was new output. prepare-ChangeLog, on the other hand, has a lot of output, so I thought this distinction was less relevant.

[James Craig]

Created attachment 230019 [details]
patch with review feedback

[Daniel Bates]

(In reply to comment #18)
> (From update of attachment 228951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228951&action=review
> 
> >> Tools/Scripts/svn-create-patch:112
> >> +    print STDERR "  Finished svn-create-patch. Running check-webkit-style.\n  ";
> > 
> > How did you come to the decision to emit "Finished svn-create-patch"? I mean, you didn't emit "Finished prepare-ChangeLog" in script Tools/Scripts/prepare-ChangeLog. I don't see much value in emitting such a message. Regardless, we should either omit such a message or emit a similar message in prepare-ChangeLog for consistency.
> 
> I'm happy to omit it if you think that's best. The reason I didn't is because svn-create-patch has no default output, so I wanted to be clear why there was new output. prepare-ChangeLog, on the other hand, has a lot of output, so I thought this distinction was less relevant.

I don't have a strong opinion. I would omit the message as I tend to follow the UNIX philosophy of the Rule of Silence: "When a program has nothing surprising to say, it should say nothing." (http://www.faqs.org/docs/artu/ch01s06.html). And I don't find "Finished ..." to be a surprising message. Although prepare-ChangeLog doesn’t seem to follow this philosophy. Regardless, I suggest we be consistent in our choice of emitting the “Finished …” message. That is, if we choose to emit it in prepare-ChangeLog then we should emit it in svn-create-patch.

[WebKit Commit Bot]

Comment on attachment 230019 [details]
patch with review feedback

Rejecting attachment 230019 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[James Craig]

I'll wait 2 hours and try again. Or someone else can CQ+ for me in the meantime.

[Daniel Bates]

Comment on attachment 230019 [details]
patch with review feedback

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

> Tools/Scripts/svn-create-patch:112
> +    system "$FindBin::Bin/check-webkit-style";

How did you come to the decision to omit the "Running check-webkit-style." message?

[James Craig]

Comment on attachment 230019 [details]
patch with review feedback

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

>> Tools/Scripts/svn-create-patch:112
>> +    system "$FindBin::Bin/check-webkit-style";
> 
> How did you come to the decision to omit the "Running check-webkit-style." message?

I must've misunderstood. I thought you wanted the entire comment removed.

[James Craig]

Created attachment 230027 [details]
patch with review feedback

Adding the print statement back in, but with the "Finished" substring removed.

[WebKit Commit Bot]

Comment on attachment 230027 [details]
patch with review feedback

Rejecting attachment 230027 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 230027, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4538986685005824

[James Craig]

I thought commit-queue would change the reviewer automatically. I guess it doesn't because the r+ was on the previous version of the attachment.

[James Craig]

Created attachment 230051 [details]
patch with reviewer comment included manually

[Daniel Bates]

(In reply to comment #24)
> (From update of attachment 230019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230019&action=review
> 
> >> Tools/Scripts/svn-create-patch:112
> >> +    system "$FindBin::Bin/check-webkit-style";
> > 
> > How did you come to the decision to omit the "Running check-webkit-style." message?
> 
> I must've misunderstood. I thought you wanted the entire comment removed.

No, I was only suggesting to remove the substring "Finished svn-create-patch."

[Daniel Bates]

Comment on attachment 230051 [details]
patch with reviewer comment included manually

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

> Tools/ChangeLog:6
> +        Reviewed by Daniel Bates <[e-mail]>.

No need for an email address. My name is sufficient:

Reviewed by Daniel Bates.

[James Craig]

https://trac.webkit.org/changeset/167751

[James Craig]

Thanks Daniel.

[Tim Horton]

I'm pretty sad that this got landed without figuring out a better solution for the issue of prepare-ChangeLog whining about not having a bug number *in the text that prepare-ChangeLog just added*. Is there a bug covering that?

[Alexey Proskuryakov]

This also spews super annoying errors about files with properties:

...
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: 'Property changes on: LayoutTests/fast/files/local-file-drag-security-expected.txt'
ERROR: Unexpected diff format when parsing a chunk: '___________________________________________________________________'
ERROR: Unexpected diff format when parsing a chunk: 'Added: svn:mime-type'
ERROR: Unexpected diff format when parsing a chunk: '## -0,0 +1 ##'
ERROR: Unexpected diff format when parsing a chunk: '\\ No newline at end of property'
ERROR: Unexpected diff format when parsing a chunk: 'Added: svn:eol-style'
ERROR: Unexpected diff format when parsing a chunk: '## -0,0 +1 ##'
ERROR: Unexpected diff format when parsing a chunk: '\\ No newline at end of property'
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: 'Property changes on: LayoutTests/fast/files/local-file-drag-security.html'
ERROR: Unexpected diff format when parsing a chunk: '___________________________________________________________________'
ERROR: Unexpected diff format when parsing a chunk: 'Added: svn:mime-type'
ERROR: Unexpected diff format when parsing a chunk: '## -0,0 +1 ##'
ERROR: Unexpected diff format when parsing a chunk: '\\ No newline at end of property'
Total errors found: 0 in 6 files

[James Craig]

(In reply to comment #33)
> I'm pretty sad that this got landed without figuring out a better solution for the issue of prepare-ChangeLog whining about not having a bug number *in the text that prepare-ChangeLog just added*. Is there a bug covering that?

I think that's correct behavior. You can avoid it by either running prepare-Changelog with the "open modified changelogs" flag (-o | --open) to edit the logs before the script continues. You can also pass --no-style to avoid running the style checker.

[James Craig]

(In reply to comment #34)
> This also spews super annoying errors about files with properties

Will you file a bug referencing or including a diff that triggers it? I didn't know about this spew, but we can either fix it in the style checker script or disable that by passing another flag to the caller points in prepare-Changelog and svn-create-patch.

[James Craig]

(In reply to comment #34)
> ERROR: Unexpected diff format when parsing a chunk: 'Property changes on: 

Why isn't this just an bug filed against check-webkit-style? Do you get different behavior when it's not called from prepare-Changelog?

[Alexey Proskuryakov]

> Why isn't this just an bug filed against check-webkit-style? Do you get different behavior when it's not called from prepare-Changelog?

I never use check-webkit-style, and do not intend to. But I use svn-create-patch and prepare-ChangeLog, and this change regressed their behavior a lot. I think that the best course of action would be to roll out this change.

I also agree with Tim's comment - it makes no sense to get an error about ChangeLogs when running prepare-ChangeLog. No one should be required to pass extra arguments to prepare-ChangeLog just to get the behavior that makes sense, the default behavior should be right.

[James Craig]

(In reply to comment #38)
> > Why isn't this just an bug filed against check-webkit-style? Do you get different behavior when it's not called from prepare-Changelog?
> 
> I never use check-webkit-style, and do not intend to. But I use svn-create-patch and prepare-ChangeLog, and this change regressed their behavior a lot. I think that the best course of action would be to roll out this change.
> 
> I also agree with Tim's comment - it makes no sense to get an error about ChangeLogs when running prepare-ChangeLog. No one should be required to pass extra arguments to prepare-ChangeLog just to get the behavior that makes sense, the default behavior should be right.

I don't want to roll out the change, but I'm fine with just changing the default value of the style parameter. That was my original plan, but other reviewers requested making this the default. See the thread above and on webkit-dev.

These are trivial diffs to commit, literally changing a 1 to a 0. That said, I'd like to hear the opinions of those who suggested this be the default behavior.

[James Craig]

Here' the prepare-Changelog diff if you want to modify your local copy in the meantime.

- my $checkWebKitStyle = 1;
+ my $checkWebKitStyle;

[James Craig]

Both complaints are resolved in bug 132209.

[James Craig]

I also filed Alexey's check-webkit-style false positive as bug 132210. Thanks for the feedback.

[Alexey Proskuryakov]

This also made svn-create-patch very slow. Filed bug 132300.