Skip to content

ci: fix clang-tidy#9140

Merged
lizan merged 5 commits intoenvoyproxy:masterfrom
derekargueta:dereka/fix-clang-tidy
Nov 27, 2019
Merged

ci: fix clang-tidy#9140
lizan merged 5 commits intoenvoyproxy:masterfrom
derekargueta:dereka/fix-clang-tidy

Conversation

@derekargueta
Copy link
Member

clang-tidy is currently yielding:

...
Running clang-tidy-diff against master branch...
From https://github.com/envoyproxy/envoy
 * branch            master     -> FETCH_HEAD
fatal: ambiguous argument 'master..HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
No relevant changes found.

on all PRs. This reverts a small part of #8965 to go back to using the base commit (the commit from which the PR was split off from) for git diff.

Signed-off-by: Derek Argueta dereka@pinterest.com

Signed-off-by: Derek Argueta <dereka@pinterest.com>
@SaveTheRbtz
Copy link
Contributor

How about set -o pipefail at the top, so issues like that could be caught earlier?

@derekargueta
Copy link
Member Author

@SaveTheRbtz nice catch, I was just trying to figure out why the git fatal error passed CI. Sadly not the first time I've been bitten by a piped failure >.< will add

Signed-off-by: Derek Argueta <dereka@pinterest.com>
Signed-off-by: Derek Argueta <dereka@pinterest.com>
@derekargueta
Copy link
Member Author

looks better

Running clang-tidy-diff against master branch...
From https://github.com/envoyproxy/envoy
 * branch            master     -> FETCH_HEAD
No relevant changes found.
##[section]Finishing: Run CI script

@zuercher
Copy link
Member

(Filed #9145 for the macOS test flake.)

echo "Running clang-tidy-diff against master branch..."
git fetch https://github.com/envoyproxy/envoy.git master
git diff "${SYSTEM_PULLREQUEST_TARGETBRANCH:-refs/heads/master}..HEAD" | filter_excludes | \
git diff "$(git merge-base HEAD FETCH_HEAD)..HEAD" | filter_excludes | \
Copy link
Member

Choose a reason for hiding this comment

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

The reason to use SYSTEM_PULLREQUEST_TARGETBRANCH is it will generate large diff for PR agains release branch and take forever.

Also, AZP run's on HEAD at GH pull request merge commit (i.e. refs/pull/9140/merge), so the merge-base will just be HEAD, so this generates no diff.

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 see, so the issue isn't with SYSTEM_PULLREQUEST_TARGETBRANCH but HEAD. Sounds like the correct fix here would be to replace HEAD with SYSTEM_PULLREQUEST_SOURCEBRANCH ?

Copy link
Member

Choose a reason for hiding this comment

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

actually, I think just git diff "${SYSTEM_PULLREQUEST_TARGETBRANCH}" will work, without ..HEAD?

Signed-off-by: Derek Argueta <dereka@pinterest.com>
echo "Running clang-tidy-diff against master branch..."
git fetch https://github.com/envoyproxy/envoy.git master
git diff "${SYSTEM_PULLREQUEST_TARGETBRANCH:-refs/heads/master}..HEAD" | filter_excludes | \
git diff "${SYSTEM_PULLREQUEST_TARGETBRANCH}" | filter_excludes | \
Copy link
Member

@lizan lizan Nov 27, 2019

Choose a reason for hiding this comment

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

Suggested change
git diff "${SYSTEM_PULLREQUEST_TARGETBRANCH}" | filter_excludes | \
git diff "remotes/origin/${SYSTEM_PULLREQUEST_TARGETBRANCH}" | filter_excludes | \

seems Azp checkout is slightly different and this should work.

@@ -70,7 +70,7 @@ elif [[ "${BUILD_REASON}" != "PullRequest" ]]; then
else
echo "Running clang-tidy-diff against master branch..."
git fetch https://github.com/envoyproxy/envoy.git master
Copy link
Member

Choose a reason for hiding this comment

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

delete this line?

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

I think this will work, thanks.

Signed-off-by: Derek Argueta <dereka@pinterest.com>
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks!

@lizan lizan merged commit 1f83353 into envoyproxy:master Nov 27, 2019
@derekargueta derekargueta deleted the dereka/fix-clang-tidy branch February 20, 2020 07:21
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.

4 participants