Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2317023002:
Filter objc headers in cpplint presubmit check (Closed)
|
Created:
4 years, 3 months ago by kthelgason Modified:
4 years, 3 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFilter objc headers in cpplint presubmit check
The previous blacklist would filter out all headers under /webrtc/sdk
and all headers in objc dirs. This adds a filter for files that end in
objc, for example `video_capture_objc.h`.
BUG=
NOTRY=true
R=kjellander@webrtc.org
Committed: https://crrev.com/3fa35172cd1a5cb65f95e94fb053346e55b98f3a
Cr-Commit-Position: refs/heads/master@{#14153}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Filter objc headers in cpplint presubmit check #Messages
Total messages: 19 (9 generated)
kthelgason@webrtc.org changed reviewers: + kjellander@webrtc.org
The CQ bit was checked by kthelgason@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/16446)
Please use NOTRY=True for CLs like this, that doesn't affect the build or tests (we're a bit low on trybot resources normally). https://codereview.webrtc.org/2317023002/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.webrtc.org/2317023002/diff/1/PRESUBMIT.py#newcode512 PRESUBMIT.py:512: r'^PRESUBMIT\.py$', I don't see why we wouldn't enforce Pylint checks on the PRESUBMIT.py script itself.
On 2016/09/07 14:40:55, kjellander_webrtc wrote: > Please use NOTRY=True for CLs like this, that doesn't affect the build or tests > (we're a bit low on trybot resources normally). Sorry about that, I didn't know that was an option. Will keep that in mind! > PRESUBMIT.py:512: r'^PRESUBMIT\.py$', > I don't see why we wouldn't enforce Pylint checks on the PRESUBMIT.py script > itself. So this is the whole point of the CL. At least on my machine, what happens when a CL contains changes to PRESUBMIT.py, is that the presubmit step will hang indefinitely. I hypothesised that this might be due to it trying to recursively evaluate itself, and sure enough, adding this to the blacklist fixed the issue. There may very well be a better way to fix this though!
On 2016/09/08 07:29:19, kthelgason wrote: > On 2016/09/07 14:40:55, kjellander_webrtc wrote: > > Please use NOTRY=True for CLs like this, that doesn't affect the build or > tests > > (we're a bit low on trybot resources normally). > > Sorry about that, I didn't know that was an option. Will keep that in mind! > > > PRESUBMIT.py:512: r'^PRESUBMIT\.py$', > > I don't see why we wouldn't enforce Pylint checks on the PRESUBMIT.py script > > itself. > So this is the whole point of the CL. At least on my machine, what happens when > a CL contains changes to PRESUBMIT.py, is that the presubmit step will hang > indefinitely. I hypothesised that this might be due to it trying to recursively > evaluate itself, and sure enough, adding this to the blacklist fixed the issue. > There may very well be a better way to fix this though! I thought the point with the CL was to not lint check objc files? I can run the presubmit without this; when I uploaded https://codereview.webrtc.org/2322843003/ from my Mac and it happily passed the presubmit check even if the CL contained an edit to the script itself. I know the PyLint check for Python is quite heavy, uses all your cores and is a bit slow (51s on my Macbook Pro), but it should pass.
On 2016/09/08 10:01:35, kjellander_webrtc wrote: > I can run the presubmit without this; when I uploaded > https://codereview.webrtc.org/2322843003/ from my Mac and it happily passed the > presubmit check even if the CL contained an edit to the script itself. > I know the PyLint check for Python is quite heavy, uses all your cores and is a > bit slow (51s on my Macbook Pro), but it should pass. Oh, so that's what's going on! I had no idea it could possibly take this long to lint a file, I just assumed it must be stuck in an infinite loop! So sorry for wasting your time with this then. I'll remove that commit.
Description was changed from ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. Fix hang on presubmit check This commit fixes a bug that would cause `git cl presubmit` to hang if there were changes to PRESUBMIT.py in the tree. BUG= ========== to ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= ==========
Description was changed from ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= ========== to ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= NOTRY=true ==========
On 2016/09/08 10:15:50, kthelgason wrote: > On 2016/09/08 10:01:35, kjellander_webrtc wrote: > > I can run the presubmit without this; when I uploaded > > https://codereview.webrtc.org/2322843003/ from my Mac and it happily passed > the > > presubmit check even if the CL contained an edit to the script itself. > > I know the PyLint check for Python is quite heavy, uses all your cores and is > a > > bit slow (51s on my Macbook Pro), but it should pass. > > Oh, so that's what's going on! I had no idea it could possibly take this long to > lint a file, I just assumed it must be stuck in an infinite loop! > So sorry for wasting your time with this then. I'll remove that commit. Yeah, for some reason it checks _all_ Python files in our repo (except the blacklisted ones). I guess it's needed to catch Python modules including each other etc.
On 2016/09/08 11:25:36, kjellander_webrtc wrote: > Yeah, for some reason it checks _all_ Python files in our repo (except the > blacklisted ones). I guess it's needed to catch Python modules including each > other etc. Right, I guess that makes sense. Can I get a LGTM for just the objc blacklist?
On 2016/09/08 16:49:44, kthelgason wrote: > On 2016/09/08 11:25:36, kjellander_webrtc wrote: > > Yeah, for some reason it checks _all_ Python files in our repo (except the > > blacklisted ones). I guess it's needed to catch Python modules including each > > other etc. > > Right, I guess that makes sense. Can I get a LGTM for just the objc blacklist? Certainly! lgtm
On 2016/09/09 06:34:48, kjellander_webrtc wrote: > On 2016/09/08 16:49:44, kthelgason wrote: > > Right, I guess that makes sense. Can I get a LGTM for just the objc blacklist? > > Certainly! lgtm Thank you sir! I'll land it then.
Description was changed from ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= NOTRY=true ========== to ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= NOTRY=true R=kjellander@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3fa35172cd1a5cb65f95e94fb... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 3fa35172cd1a5cb65f95e94fb053346e55b98f3a (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= NOTRY=true R=kjellander@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3fa35172cd1a5cb65f95e94fb... ========== to ========== Filter objc headers in cpplint presubmit check The previous blacklist would filter out all headers under /webrtc/sdk and all headers in objc dirs. This adds a filter for files that end in objc, for example `video_capture_objc.h`. BUG= NOTRY=true R=kjellander@webrtc.org Committed: https://crrev.com/3fa35172cd1a5cb65f95e94fb053346e55b98f3a Cr-Commit-Position: refs/heads/master@{#14153} ========== |
