Fix clang-tidy path filtering #60225

driazati · 2021-06-17T23:33:25Z

PR #60048 neglected to include the --paths option for file filtering, so it ended up passing every changed file in the diff to clang-tidy (cpp files outside torch/csrc/, yaml/sh files, etc.). This adds that back in to make the filtering work properly again.

Tested it manually by printing out the files to lint and running

curl -L https://github.com/pytorch/pytorch/pull/60018.diff > diff
python tools/clang_tidy.py --diff-file diff --paths torch/csrc/

curl -L https://github.com/pytorch/pytorch/pull/60222.diff > diff
python tools/clang_tidy.py --diff-file diff --paths torch/csrc/

Should fix #60192 and fix #60193, the files tripping errors there shouldn't have been passed to clang-tidy in the first place (supporting aten/ for clang-tidy is a separate task)

facebook-github-bot · 2021-06-17T23:33:32Z

💊 CI failures summary and remediations

As of commit 3e9a227 (more details on the Dr. CI page and at hud.pytorch.org/pr/60225):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

facebook-github-bot · 2021-06-17T23:39:27Z

@driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zhouzhuojie · 2021-06-17T23:44:09Z

need to enable clang-tidy back

facebook-github-bot · 2021-06-17T23:48:14Z

@driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zhouzhuojie · 2021-06-17T23:54:29Z

clang_tidy.py: error: argument -g/--glob: expected one argument

hmm, seemingly that --glob "-torch..." was interpreted as --glob -t ..., so might need to revert to the previous format

driazati · 2021-06-17T23:56:28Z

need to enable clang-tidy back

actually we might want to wait on enabling it until we fix https://github.com/pytorch/pytorch/runs/2850264818

(header is missing in the docker image we're using now, it's probably there somewhere but clang-tidy doesn't know where)

PR pytorch#60048 neglected to include the `--paths` option for file filtering, so it ended up passing every changed file in the diff to clang-tidy (cpp files outside `torch/csrc/`, yaml/sh files, etc.). This adds that back in to make the filtering work properly again. Tested it manually by printing out the files to lint and running ```bash curl -L https://github.com/pytorch/pytorch/pull/60018.diff > diff python tools/clang_tidy.py --diff-file diff --paths torch/csrc/ curl -L https://github.com/pytorch/pytorch/pull/60222.diff > diff python tools/clang_tidy.py --diff-file diff --paths torch/csrc/ ```

PyTorch's distributed library imports `omp.h` a bunch so we need that header around for clang-tidy. `libomp-dev` provides it, so this adds that to the `apt install` of the Docker image. Once we merge this and land pytorch/pytorch#60225, we can update the image tag in `lint.yml` and re-enable clang-tidy on master since all the errors we saw should be addressed. Tested by running the clang command in the Docker image, seeing a failure, installing `libomp-dev`, running again and seeing no failure. ```bash docker run -it test /bin/bash clang-tidy -p build -config '{"InheritParentConfig": true, "Checks": " bugprone-*, -bugprone-forward-declaration-namespace, -bugprone-macro-parentheses, -bugprone-lambda-function-name, -bugprone-reserved-identifier, cppcoreguidelines-*, -cppcoreguidelines-avoid-magic-numbers, -cppcoreguidelines-interfaces-global-init, -cppcoreguidelines-macro-usage, -cppcoreguidelines-owning-memory, -cppcoreguidelines-pro-bounds-array-to-pointer-decay, -cppcoreguidelines-pro-bounds-constant-array-index, -cppcoreguidelines-pro-bounds-pointer-arithmetic, -cppcoreguidelines-pro-type-cstyle-cast, -cppcoreguidelines-pro-type-reinterpret-cast, -cppcoreguidelines-pro-type-static-cast-downcast, -cppcoreguidelines-pro-type-union-access, -cppcoreguidelines-pro-type-vararg, -cppcoreguidelines-special-member-functions, -facebook-hte-RelativeInclude, hicpp-exception-baseclass, hicpp-avoid-goto, modernize-*, -modernize-concat-nested-namespaces, -modernize-return-braced-init-list, -modernize-use-auto, -modernize-use-default-member-init, -modernize-use-using, -modernize-use-trailing-return-type, performance-*, -performance-noexcept-move-constructor, -performance-unnecessary-value-param, ", "HeaderFilterRegex": "torch/csrc/.*", "AnalyzeTemporaryDtors": false, "CheckOptions": null}' -line-filter '[{"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp", "lines": [["1", "1"]]}, {"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.h", "lines": [["39", "39"]]}, {"name": "torch/csrc/distributed/rpc/testing/init.cpp", "lines": [["1", "1"]]}]' torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp torch/csrc/distributed/rpc/testing/init.cpp apt install -y libomp-dev clang-tidy -p build -config '{"InheritParentConfig": true, "Checks": " bugprone-*, -bugprone-forward-declaration-namespace, -bugprone-macro-parentheses, -bugprone-lambda-function-name, -bugprone-reserved-identifier, cppcoreguidelines-*, -cppcoreguidelines-avoid-magic-numbers, -cppcoreguidelines-interfaces-global-init, -cppcoreguidelines-macro-usage, -cppcoreguidelines-owning-memory, -cppcoreguidelines-pro-bounds-array-to-pointer-decay, -cppcoreguidelines-pro-bounds-constant-array-index, -cppcoreguidelines-pro-bounds-pointer-arithmetic, -cppcoreguidelines-pro-type-cstyle-cast, -cppcoreguidelines-pro-type-reinterpret-cast, -cppcoreguidelines-pro-type-static-cast-downcast, -cppcoreguidelines-pro-type-union-access, -cppcoreguidelines-pro-type-vararg, -cppcoreguidelines-special-member-functions, -facebook-hte-RelativeInclude, hicpp-exception-baseclass, hicpp-avoid-goto, modernize-*, -modernize-concat-nested-namespaces, -modernize-return-braced-init-list, -modernize-use-auto, -modernize-use-default-member-init, -modernize-use-using, -modernize-use-trailing-return-type, performance-*, -performance-noexcept-move-constructor, -performance-unnecessary-value-param, ", "HeaderFilterRegex": "torch/csrc/.*", "AnalyzeTemporaryDtors": false, "CheckOptions": null}' -line-filter '[{"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp", "lines": [["1", "1"]]}, {"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.h", "lines": [["39", "39"]]}, {"name": "torch/csrc/distributed/rpc/testing/init.cpp", "lines": [["1", "1"]]}]' torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp torch/csrc/distributed/rpc/testing/init.cpp ```

facebook-github-bot · 2021-06-18T01:20:26Z

@driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-06-18T06:05:42Z

@driazati merged this pull request in ecc3718.

PyTorch's distributed library imports `omp.h` a bunch so we need that header around for clang-tidy. `libomp-dev` provides it, so this adds that to the `apt install` of the Docker image. Once we merge this and land pytorch/pytorch#60225, we can update the image tag in `lint.yml` and re-enable clang-tidy on master since all the errors we saw should be addressed. Tested by running the clang command in the Docker image, seeing a failure, installing `libomp-dev`, running again and seeing no failure. ```bash docker run -it test /bin/bash clang-tidy -p build -config '{"InheritParentConfig": true, "Checks": " bugprone-*, -bugprone-forward-declaration-namespace, -bugprone-macro-parentheses, -bugprone-lambda-function-name, -bugprone-reserved-identifier, cppcoreguidelines-*, -cppcoreguidelines-avoid-magic-numbers, -cppcoreguidelines-interfaces-global-init, -cppcoreguidelines-macro-usage, -cppcoreguidelines-owning-memory, -cppcoreguidelines-pro-bounds-array-to-pointer-decay, -cppcoreguidelines-pro-bounds-constant-array-index, -cppcoreguidelines-pro-bounds-pointer-arithmetic, -cppcoreguidelines-pro-type-cstyle-cast, -cppcoreguidelines-pro-type-reinterpret-cast, -cppcoreguidelines-pro-type-static-cast-downcast, -cppcoreguidelines-pro-type-union-access, -cppcoreguidelines-pro-type-vararg, -cppcoreguidelines-special-member-functions, -facebook-hte-RelativeInclude, hicpp-exception-baseclass, hicpp-avoid-goto, modernize-*, -modernize-concat-nested-namespaces, -modernize-return-braced-init-list, -modernize-use-auto, -modernize-use-default-member-init, -modernize-use-using, -modernize-use-trailing-return-type, performance-*, -performance-noexcept-move-constructor, -performance-unnecessary-value-param, ", "HeaderFilterRegex": "torch/csrc/.*", "AnalyzeTemporaryDtors": false, "CheckOptions": null}' -line-filter '[{"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp", "lines": [["1", "1"]]}, {"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.h", "lines": [["39", "39"]]}, {"name": "torch/csrc/distributed/rpc/testing/init.cpp", "lines": [["1", "1"]]}]' torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp torch/csrc/distributed/rpc/testing/init.cpp apt install -y libomp-dev clang-tidy -p build -config '{"InheritParentConfig": true, "Checks": " bugprone-*, -bugprone-forward-declaration-namespace, -bugprone-macro-parentheses, -bugprone-lambda-function-name, -bugprone-reserved-identifier, cppcoreguidelines-*, -cppcoreguidelines-avoid-magic-numbers, -cppcoreguidelines-interfaces-global-init, -cppcoreguidelines-macro-usage, -cppcoreguidelines-owning-memory, -cppcoreguidelines-pro-bounds-array-to-pointer-decay, -cppcoreguidelines-pro-bounds-constant-array-index, -cppcoreguidelines-pro-bounds-pointer-arithmetic, -cppcoreguidelines-pro-type-cstyle-cast, -cppcoreguidelines-pro-type-reinterpret-cast, -cppcoreguidelines-pro-type-static-cast-downcast, -cppcoreguidelines-pro-type-union-access, -cppcoreguidelines-pro-type-vararg, -cppcoreguidelines-special-member-functions, -facebook-hte-RelativeInclude, hicpp-exception-baseclass, hicpp-avoid-goto, modernize-*, -modernize-concat-nested-namespaces, -modernize-return-braced-init-list, -modernize-use-auto, -modernize-use-default-member-init, -modernize-use-using, -modernize-use-trailing-return-type, performance-*, -performance-noexcept-move-constructor, -performance-unnecessary-value-param, ", "HeaderFilterRegex": "torch/csrc/.*", "AnalyzeTemporaryDtors": false, "CheckOptions": null}' -line-filter '[{"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp", "lines": [["1", "1"]]}, {"name": "torch/csrc/distributed/rpc/testing/faulty_process_group_agent.h", "lines": [["39", "39"]]}, {"name": "torch/csrc/distributed/rpc/testing/init.cpp", "lines": [["1", "1"]]}]' torch/csrc/distributed/rpc/testing/faulty_process_group_agent.cpp torch/csrc/distributed/rpc/testing/init.cpp ```

This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with pytorch#60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings.

This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with #60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings. [ghstack-poisoned]

This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with #60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings. Differential Revision: [D29239783](https://our.internmc.facebook.com/intern/diff/D29239783) [ghstack-poisoned]

This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with #60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings. The diff parsing was also broken on diffs that change an entire file (git uses -1 -> +1 as the line range in those diffs for some reason). Rather than deal with these intricacies myself I copied from https://stackoverflow.com/a/39909397 which uses the `unidiff` library, so we need to add that to the image first: pytorch/test-infra#43 Differential Revision: [D29239783](https://our.internmc.facebook.com/intern/diff/D29239783) [ghstack-poisoned]

Summary: Pull Request resolved: #60297 This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with #60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings. Test Plan: Imported from OSS Reviewed By: seemethere, 1ntEgr8 Differential Revision: D29239783 Pulled By: driazati fbshipit-source-id: b1893256fdb27436af03d6c5279e81f64b47fe6b

Summary: PR pytorch#60048 neglected to include the `--paths` option for file filtering, so it ended up passing every changed file in the diff to clang-tidy (cpp files outside `torch/csrc/`, yaml/sh files, etc.). This adds that back in to make the filtering work properly again. Tested it manually by printing out the files to lint and running ```bash curl -L https://github.com/pytorch/pytorch/pull/60018.diff > diff python tools/clang_tidy.py --diff-file diff --paths torch/csrc/ curl -L https://github.com/pytorch/pytorch/pull/60222.diff > diff python tools/clang_tidy.py --diff-file diff --paths torch/csrc/ ``` Should fix pytorch#60192 and fix pytorch#60193, the files tripping errors there shouldn't have been passed to clang-tidy in the first place (supporting aten/ for clang-tidy is a separate task) Pull Request resolved: pytorch#60225 Reviewed By: zhouzhuojie Differential Revision: D29216251 Pulled By: driazati fbshipit-source-id: b5d7fb7161d33eb7958a6f1ccc25809942045209

Summary: Pull Request resolved: pytorch#60297 This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with pytorch#60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings. Test Plan: Imported from OSS Reviewed By: seemethere, 1ntEgr8 Differential Revision: D29239783 Pulled By: driazati fbshipit-source-id: b1893256fdb27436af03d6c5279e81f64b47fe6b

* Fix default DEFAULT_FILE_PATTERN in clang-tidy (pytorch#60212) Summary: Without the change, clang-tidy also checks folders like `.circleci/...` Example of the clang-tidy that looked into `.circleci` changes https://github.com/pytorch/pytorch/runs/2844682644?check_suite_focus=true [skip ci] Pull Request resolved: pytorch#60212 Reviewed By: seemethere Differential Revision: D29214728 Pulled By: zhouzhuojie fbshipit-source-id: fd53f7b2f7d88936264db1effdc06cc4fc271ca4 * Fix clang-tidy path filtering (pytorch#60225) Summary: PR pytorch#60048 neglected to include the `--paths` option for file filtering, so it ended up passing every changed file in the diff to clang-tidy (cpp files outside `torch/csrc/`, yaml/sh files, etc.). This adds that back in to make the filtering work properly again. Tested it manually by printing out the files to lint and running ```bash curl -L https://github.com/pytorch/pytorch/pull/60018.diff > diff python tools/clang_tidy.py --diff-file diff --paths torch/csrc/ curl -L https://github.com/pytorch/pytorch/pull/60222.diff > diff python tools/clang_tidy.py --diff-file diff --paths torch/csrc/ ``` Should fix pytorch#60192 and fix pytorch#60193, the files tripping errors there shouldn't have been passed to clang-tidy in the first place (supporting aten/ for clang-tidy is a separate task) Pull Request resolved: pytorch#60225 Reviewed By: zhouzhuojie Differential Revision: D29216251 Pulled By: driazati fbshipit-source-id: b5d7fb7161d33eb7958a6f1ccc25809942045209 * Re-enable clang-tidy on PRs (pytorch#60297) Summary: Pull Request resolved: pytorch#60297 This switches clang-tidy to the fresh tag from https://github.com/pytorch/test-infra/runs/2860763986 which has a fix for the missing OMP headers we were seeing. Along with pytorch#60225 this should restore clang-tidy to normal functionality and we shouldn't see any spurious warnings. Test Plan: Imported from OSS Reviewed By: seemethere, 1ntEgr8 Differential Revision: D29239783 Pulled By: driazati fbshipit-source-id: b1893256fdb27436af03d6c5279e81f64b47fe6b Co-authored-by: Zhuojie Zhou <zhouzhuojie@users.noreply.github.com> Co-authored-by: driazati <driazati@users.noreply.github.com>

facebook-github-bot added the cla signed label Jun 17, 2021

driazati force-pushed the wwlvf branch from 651935e to db1e453 Compare June 17, 2021 23:36

driazati requested a review from a team June 17, 2021 23:39

driazati force-pushed the wwlvf branch from db1e453 to 18d368b Compare June 17, 2021 23:45

zhouzhuojie approved these changes Jun 17, 2021

View reviewed changes

driazati force-pushed the wwlvf branch from 18d368b to 3e9a227 Compare June 17, 2021 23:59

driazati mentioned this pull request Jun 18, 2021

Install omp in clang-tidy docker image pytorch/test-infra#41

Merged

facebook-github-bot closed this in ecc3718 Jun 18, 2021

facebook-github-bot added the Merged label Jun 18, 2021

driazati mentioned this pull request Jun 18, 2021

Re-enable clang-tidy on PRs #60296

Closed

driazati mentioned this pull request Jun 18, 2021

Re-enable clang-tidy on PRs #60297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix clang-tidy path filtering #60225

Fix clang-tidy path filtering #60225

Uh oh!

driazati commented Jun 17, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Jun 17, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Jun 17, 2021

Uh oh!

zhouzhuojie commented Jun 17, 2021

Uh oh!

facebook-github-bot commented Jun 17, 2021

Uh oh!

zhouzhuojie commented Jun 17, 2021

Uh oh!

driazati commented Jun 17, 2021

Uh oh!

facebook-github-bot commented Jun 18, 2021

Uh oh!

facebook-github-bot commented Jun 18, 2021

Uh oh!

Uh oh!

Fix clang-tidy path filtering #60225

Fix clang-tidy path filtering #60225

Uh oh!

Conversation

driazati commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

facebook-github-bot commented Jun 17, 2021

Uh oh!

zhouzhuojie commented Jun 17, 2021

Uh oh!

facebook-github-bot commented Jun 17, 2021

Uh oh!

zhouzhuojie commented Jun 17, 2021

Uh oh!

driazati commented Jun 17, 2021

Uh oh!

facebook-github-bot commented Jun 18, 2021

Uh oh!

facebook-github-bot commented Jun 18, 2021

Uh oh!

Uh oh!

driazati commented Jun 17, 2021 •

edited

Loading

facebook-github-bot commented Jun 17, 2021 •

edited

Loading