Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2375713002:
blink: Remove redundant OWNER etc check during presubmit.
Descriptionblink: Remove redundant OWNER etc check during presubmit.
Now that blink is in src, it uses the toplevel PRESUBMIT.py file, which
already adds OWNER checks etc. Don't add these checks a second time.
BUG=649970
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (6 generated)
The CQ bit was checked by thakis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... File third_party/WebKit/PRESUBMIT.py (left): https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... third_party/WebKit/PRESUBMIT.py:96: maxlen=800, license_header=license_header)) Won't this complain if someone modifies a file that doesn't have the standard Chromium boilerplate?
https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... File third_party/WebKit/PRESUBMIT.py (left): https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... third_party/WebKit/PRESUBMIT.py:96: maxlen=800, license_header=license_header)) On 2016/09/27 19:08:47, dcheng wrote: > Won't this complain if someone modifies a file that doesn't have the standard > Chromium boilerplate? I was wondering that. But I think we currently run owners tests twice, once with and once without that license_header line, so removing one of the two shouldn't make things worse than they are today, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis@chromium.org changed reviewers: + dpranke@chromium.org
dpranke, do you know about presubmits? Does this make sense?
https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... File third_party/WebKit/PRESUBMIT.py (left): https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... third_party/WebKit/PRESUBMIT.py:96: maxlen=800, license_header=license_header)) On 2016/09/27 19:10:55, Nico (mostly away until Oct 3) wrote: > On 2016/09/27 19:08:47, dcheng wrote: > > Won't this complain if someone modifies a file that doesn't have the standard > > Chromium boilerplate? > > I was wondering that. But I think we currently run owners tests twice, once with > and once without that license_header line, so removing one of the two shouldn't > make things worse than they are today, right? In the top-level //PRESUBMIT.py, we exclude //third_party/WebKit (correctly, because Blink files will have the wrong header). If you also remove things here, we would default to using the regular Chromium license header, and the checks would fail. But, since you're also deleting the check, we'll skip the check completely. Really, the comment on line 90 still applies: we should figure out which license checks to do, and do those. Maybe re-add the comment, link it to a bug to track this, assign to qyearsley@ and cc me?
On 2016/09/27 21:27:25, Dirk Pranke wrote: > https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... > File third_party/WebKit/PRESUBMIT.py (left): > > https://codereview.chromium.org/2375713002/diff/1/third_party/WebKit/PRESUBMI... > third_party/WebKit/PRESUBMIT.py:96: maxlen=800, license_header=license_header)) > On 2016/09/27 19:10:55, Nico (mostly away until Oct 3) wrote: > > On 2016/09/27 19:08:47, dcheng wrote: > > > Won't this complain if someone modifies a file that doesn't have the > standard > > > Chromium boilerplate? > > > > I was wondering that. But I think we currently run owners tests twice, once > with > > and once without that license_header line, so removing one of the two > shouldn't > > make things worse than they are today, right? > > In the top-level //PRESUBMIT.py, we exclude //third_party/WebKit (correctly, > because Blink files will have the wrong header). If you also remove things here, > we would default to using the regular Chromium license header, and the checks > would fail. But, since you're also deleting the check, we'll skip the check > completely. > > Really, the comment on line 90 still applies: we should figure out which license > checks to do, and do those. > > Maybe re-add the comment, link it to a bug to track this, assign to qyearsley@ > and cc me? Confusing! I was looking at DEFAULT_BLACK_LIST in presubmit_support.py, which seemed to exclude third_party but include third_party/WebKit. But then we use our own blacklist at the top level PRESUBMIT.py to make this all work... Well, anyway, this seems reasonable, but perhaps let's remove the CheckForVersionControlConflicts bit while we're in here too.
Since third_party/WebKit is excluded in the top-level presubmit, it seems like removing the call to PanProjectChecks here would mean that many pan-project checks that are currently done for third_party/WebKit wouldn't be done after this CL as it is now -- It seems like (due to line length and header differences) we want to continue to exclude third_party/WebKit for PanProjectChecks at the top-level, but we probably do still want to do most of those checks in PanProjectChecks for third_party/WebKit/PRESUBMIT.py because most of them are not redundant, right?
@qyearsley: correct.
Rietveld CL cleanup time ... @thakis - what do you want to do with this CL? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
