Skip to content

ci/tools: use gen comp db python script directly to provide more flexibility#35926

Merged
wbpcode merged 5 commits intoenvoyproxy:mainfrom
wbpcode:dev-minor-imporve-tool
Sep 2, 2024
Merged

ci/tools: use gen comp db python script directly to provide more flexibility#35926
wbpcode merged 5 commits intoenvoyproxy:mainfrom
wbpcode:dev-minor-imporve-tool

Conversation

@wbpcode
Copy link
Member

@wbpcode wbpcode commented Aug 31, 2024

Commit Message: ci/tools: use gen comp db python script directly to provide more flexibility

Additional Description:

$ ci/do_ci.sh refresh_compdb --vscode --include_all --exclude_contrib

This is much convenient now to specify parameters of .py script.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

…ibility

Signed-off-by: wangbaiping <wbphub@gmail.com>
ci/do_ci.sh Outdated

CI_TARGET=$1
shift
CI_PARAMS=("$@")
Copy link
Member

@phlax phlax Aug 31, 2024

Choose a reason for hiding this comment

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

this would ignore the first argument - not sure if that is what we want - you would need to do something like ...

./ci/do_ci.sh IGNOREME arg1 arg2 ...

Copy link
Member

Choose a reason for hiding this comment

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

actually - ignore me - CI_TARGET is refresh_compdb in this case so above comment is wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

I am sure. First arg is the target name and I was going to ignore it.

Copy link
Member

Choose a reason for hiding this comment

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

i guess im a little uncomfortable more generally with making do_ci.sh into an arg accepting command

atm its more for generating standard args to pass to tooling - which is why it doesnt have ${@} passthrough already

Copy link
Member Author

Choose a reason for hiding this comment

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

I am open to that. What's your suggestion I I want to do similar things? Create a new script under the ci/ or tools/

Copy link
Member

Choose a reason for hiding this comment

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

one option is to do something like what do_ci.sh does for bazel args - ie define them as env var and then parse those into an array

Copy link
Member Author

@wbpcode wbpcode Aug 31, 2024

Choose a reason for hiding this comment

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

It is okay for me for this PR for now.

But I have another point. I hope we can provide a CLI tools that could help developers to develop, build, test, check format check, check clang tidy, etc., as supplements to poor C++ tool chains. Parameters could be used to control different functions. The developers needn't to find different helper scripts at every where of the repo.

Now, the do_ci.sh is something that similar to my expected role. Although it's not designed to do this.

Copy link
Member

Choose a reason for hiding this comment

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

yeah - its not designed to do this

the role of this file is to hold the canonical ci targets - as called in actual ci

this role has become even more important as the workflow files are not editable in a pr - so the workflows hand over to these targets

Copy link
Member

Choose a reason for hiding this comment

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

tbh - the role you are describing is more bazel's role

Copy link
Member Author

@wbpcode wbpcode Aug 31, 2024

Choose a reason for hiding this comment

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

tbh - the role you are describing is more bazel's role

Yeah, but it doesn't meet my requirement now. I personally want to pay some more time to learn and optimize this tools for Envoy. But there are always some things has higher priority, in both life and working. What I need to do are not what I want to do in most time. :(

@vikaschoudhary16
Copy link
Contributor

Please update bazel/Readme.md as well

Signed-off-by: wangbaiping <wbphub@gmail.com>
Signed-off-by: wangbaiping <wbphub@gmail.com>
phlax
phlax previously approved these changes Sep 2, 2024
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @wbpcode

@wbpcode
Copy link
Member Author

wbpcode commented Sep 2, 2024

Please update bazel/Readme.md as well

I have added a description to the new target the ci/README.md. Basically, this new CI target won't effect the use of the python script. So, I am inclined not to update bazel/README.md.

Signed-off-by: wangbaiping <wbphub@gmail.com>
Signed-off-by: wangbaiping <wbphub@gmail.com>
@wbpcode wbpcode enabled auto-merge (squash) September 2, 2024 14:41
@wbpcode wbpcode merged commit 19358ac into envoyproxy:main Sep 2, 2024
mum4k pushed a commit to envoyproxy/nighthawk that referenced this pull request Sep 10, 2024
- Update the ENVOY_COMMIT and ENVOY_SHA in bazel/repositories.bzl to the latest Envoy's commit.
- Update .bazelrc to envoyproxy/envoy#35978
- Update ci/run_envoy_docker.sh to envoyproxy/envoy#35926 and envoyproxy/envoy#35660
- Update tools/gen_compilation_database.py to envoyproxy/envoy#36018 and envoyproxy/envoy#35811
- Update source/client/process_impl.cc to match interface change in envoyproxy/envoy#35912

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants