Skip to content

clang/tools: Use python distributed versions#29455

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:py-clang-tools
Sep 11, 2023
Merged

clang/tools: Use python distributed versions#29455
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:py-clang-tools

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Sep 6, 2023

This massively reduces the overhead for running the clang tooling

In this PR only clang-format is hooked up to the format precheck, but we can use the clang-tidy install when bringing it back

Ideally we would be able to do the same with lcov and friends which would reduce the cost of running coverage, but i dont see a package for them

We may decide in the future to fetch these tools from a different channel - the one in this PR is not ideal although it meets our current needs

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Sep 6, 2023
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #29455 was opened by phlax.

see: more, trace.

keith
keith previously approved these changes Sep 6, 2023
Copy link
Copy Markdown
Member

@keith keith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@phlax phlax changed the title clang/tools: Use python distributed versions [WIP] clang/tools: Use python distributed versions Sep 7, 2023
@phlax phlax marked this pull request as draft September 7, 2023 10:36
@phlax phlax force-pushed the py-clang-tools branch 4 times, most recently from 4465523 to 5a2a6da Compare September 7, 2023 13:04
Comment thread tools/code_format/check_format.py Outdated
Comment thread tools/code_format/check_format.py Outdated
@phlax phlax force-pushed the py-clang-tools branch 6 times, most recently from 391e288 to 7a4030c Compare September 11, 2023 10:10
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 11, 2023

not sure why but this has terrible performance - possibly using the entry_point - ill debug further

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 11, 2023

hmm - yep the python wrapper seems to add 200ms per call and theres a lot of calls

$ time bazel run //tools/code_format:py-clang-format "${PWD}/source/extensions/http/cache/simple_http_cache/simple_http_cache.cc"
real    0m0.360s
user    0m0.239s
sys     0m0.035s
$ time bazel run //tools/code_format:clang-format "${PWD}/source/extensions/http/cache/simple_http_cache/simple_http_cache.cc"
real    0m0.091s
user    0m0.006s
sys     0m0.018s

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 11, 2023

committing the binary from the python package directly to the repo and using that and ci runs at usual speed

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 11, 2023

... as does adding a bazel rule to fish the binary out of the python package

@phlax phlax force-pushed the py-clang-tools branch 4 times, most recently from eb13fc4 to c3d54c2 Compare September 11, 2023 13:16
@phlax phlax force-pushed the py-clang-tools branch 5 times, most recently from 21a0d83 to a4ea72f Compare September 11, 2023 18:09
@phlax phlax changed the title [WIP] clang/tools: Use python distributed versions clang/tools: Use python distributed versions Sep 11, 2023
@phlax phlax marked this pull request as ready for review September 11, 2023 18:10
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax merged commit 230e16f into envoyproxy:main Sep 11, 2023
phlax added a commit to phlax/envoy that referenced this pull request Sep 19, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added a commit to phlax/envoy that referenced this pull request Sep 19, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
phlax added a commit that referenced this pull request Sep 20, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added a commit that referenced this pull request Sep 21, 2023
Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: phlax <phlax@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants