Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update run_clang_tidy.sh to support bazel targets with '@repository' prefixes #15604

Merged
merged 3 commits into from
Mar 23, 2021
Merged

Update run_clang_tidy.sh to support bazel targets with '@repository' prefixes #15604

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c691d7
Update run_clang_tidy.sh to support repository
murray-stripe Mar 22, 2021
31214f0
remove array expansion
murray-stripe Mar 22, 2021
778c01f
PR Feedbac - use read + array expansion
murray-stripe Mar 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/run_clang_tidy.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ echo "Generating compilation database..."

# bazel build need to be run to setup virtual includes, generating files which are consumed
# by clang-tidy
"${ENVOY_SRCDIR}/tools/gen_compilation_database.py" --include_headers
"${ENVOY_SRCDIR}/tools/gen_compilation_database.py" --include_headers ${COMP_DB_TARGETS:+${COMP_DB_TARGETS[@]}}
Copy link
Member

Choose a reason for hiding this comment

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

im not sure but i think the array expansion wont work here - ill check further

Copy link
Member

@phlax phlax Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

arrays arent carried in the env vars so there isnt a way that "${COMP_DB_TARGETS[@]}" would ever be evaluated as one (unless either this script was sourced, or sourced another script that set this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set this in an env-var with 4 targets and tested it (as above in PR description) and it worked. Are you saying that if I set and exported the value, you would not expect it to work?

Copy link
Member

Choose a reason for hiding this comment

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

i created a file with the following:

#!/bin/bash

FOO=${COMP_DB_TARGETS:+${COMP_DB_TARGETS[@]}}

echo "$FOO"

and then did the following:

$ export COMP_DB_TARGETS="xyz"
$ ./runfoo
xyz
$ export COMP_DB_TARGETS=("abc" "xyz")
$ ./runfoo

Copy link
Member

Choose a reason for hiding this comment

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

if COMP_DB_TARGETS is an array it wont work

i think you need to do something like

read -ra COMP_DB_TARGETS <<< "$COMP_DB_TARGETS"

if you want to use an array

Copy link
Member

Choose a reason for hiding this comment

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

and then you would also need to set it to a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this currently working in our internal CI system with the following:

COMP_DB_TARGETS="@envoy//source/... @envoy//test/... @envoy//tools/... //filters/..."
export COMP_DB_TARGETS

bash -ex ./envoy/ci/do_ci.sh 'bazel.clang_tidy' '//filters/...'

And then with -x on run_clang_tidy.sh I see:

+ /src/envoy/tools/gen_compilation_database.py --include_headers @envoy//source/... @envoy//test/... @envoy//tools/... //filters/...

Copy link
Member

@phlax phlax Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

its set to a string - my point is $COMP_DB_TARGETS[@] is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, gotcha. I'll update to not do any array expansion.

Copy link
Member

Choose a reason for hiding this comment

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

tbh i think you do want array expansion
i added a py script (similar to what is happening in this file)

export COMP_DB_TARGETS="abc xyz"
$ ./runfoo
abc xyz
['./runfoo.py', 'abc xyz']

if you want the args to be separate at least


# Do not run clang-tidy against win32 impl
# TODO(scw00): We should run clang-tidy against win32 impl once we have clang-cl support for Windows
Expand Down