Bug 131115

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

Created attachment 228439 [details] patch

<rdar://problem/16506103>

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.

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.

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.

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

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.

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.

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.

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.

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.

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.

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.

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.

Created attachment 228951 [details] patch with review feedback

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.

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.

Created attachment 230019 [details] patch with review feedback

(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.

Comment on attachment 230019 [details] patch with review feedback Rejecting attachment 230019 [details] from commit-queue. jcraig@apple.com 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.

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

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?

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.

Created attachment 230027 [details] patch with review feedback Adding the print statement back in, but with the "Finished" substring removed.

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

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.

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

(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."

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 <dbates@webkit.org>. No need for an email address. My name is sufficient: Reviewed by Daniel Bates.

Thanks Daniel.

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?

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

(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.

(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.

(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?

> 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.

(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.

Here' the prepare-Changelog diff if you want to modify your local copy in the meantime. - my $checkWebKitStyle = 1; + my $checkWebKitStyle;

Both complaints are resolved in bug 132209.

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

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