Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Only run clang-tidy in presubmit when C, C++, ObjC, or ObjC++ files change #36887

Merged
auto-submit merged 6 commits into from
Oct 28, 2022
Merged

Only run clang-tidy in presubmit when C, C++, ObjC, or ObjC++ files change #36887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e47b500
Only run clang-tidy when C, C++, ObjC, or Obc++ files change
jmagman Oct 20, 2022
ec45a34
Remove testing comment
jmagman Oct 20, 2022
cdc41f1
python
jmagman Oct 20, 2022
f730f37
Add timestamps to bash script
jmagman Oct 20, 2022
3ef4276
Headers
jmagman Oct 20, 2022
37a67f6
flat buffers and shader bindings
jmagman Oct 27, 2022
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
49 changes: 49 additions & 0 deletions .ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,17 @@ targets:
lint_android: "false"
lint_host: "true"
timeout: 60
runIf:
- DEPS
- .ci.yaml
- tools/**
- ci/**
- "**.h"
- "**.c"
- "**.cc"
Copy link
Member

@zanderso zanderso Oct 20, 2022

Choose a reason for hiding this comment

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

I think this also needs to run on changes to header files right? Here and below.

Suggested change
- "**.cc"
- "**.cc"
- "**.h"

Copy link
Member Author

@jmagman jmagman Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You're right. Should I filter out third_party from these checks? (I mean in the script, not in the ci.yaml)

Copy link
Member Author

@jmagman jmagman Oct 20, 2022

Choose a reason for hiding this comment

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

Hm, you explicitly added a test for third_party so I guess not #26722

Copy link
Member

@zanderso zanderso Oct 21, 2022

Choose a reason for hiding this comment

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

Yeah, the script already skips source files in third_party.

Morning brain is also remembering that we generate and analyze source for flat buffers and shader bindings, so this should also run on changes to *.fbs, *.frag, and *.vert.

Copy link
Member Author

@jmagman jmagman Oct 21, 2022

Choose a reason for hiding this comment

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

Sorry for my shader ignorance, so changes to those files generate .cc files that are being linted?

The list of files it's linting:
https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8799745439669169009/+/u/test:_lint_ios_debug_sim/stdout

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8799745440095255937/+/u/test:_lint_host_debug/stdout

Copy link
Member

@zanderso zanderso Oct 21, 2022

Choose a reason for hiding this comment

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

I think the generated source might be in header files, which is why they might not show up in the list of .cc files being analyzed. It's why we have to do builds before linting: https://flutter.googlesource.com/recipes/+/refs/heads/main/recipes/engine/engine_lint.py#91.

Copy link
Member Author

@jmagman jmagman Oct 27, 2022

Choose a reason for hiding this comment

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

Updated. That's a lot of files, but at least it will be skipped on dart and java -only commits 🙂

Copy link
Member Author

@jmagman jmagman Nov 1, 2022

Choose a reason for hiding this comment

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

It is being skipped on some PRs, so that's something.
#37187
#37212

- "**.fbs"
- "**.frag"
- "**.vert"

- name: Linux Android clang-tidy
recipe: engine/engine_lint
Expand All @@ -203,6 +214,18 @@ targets:
lint_android: "true"
lint_host: "false"
timeout: 60
runIf:
- DEPS
- .ci.yaml
- tools/**
- ci/**
- "**.h"
- "**.c"
- "**.cc"
- "**.fbs"
- "**.frag"
- "**.vert"
- "**.py" # Run pylint on the fastest clang-tidy builder.

- name: Linux Arm Host Engine
recipe: engine/engine_arm
Expand Down Expand Up @@ -336,6 +359,19 @@ targets:
lint_host: "true"
lint_ios: "false"
timeout: 75
runIf:
- DEPS
- .ci.yaml
- tools/**
- ci/**
- "**.h"
- "**.c"
- "**.cc"
- "**.fbs"
- "**.frag"
- "**.vert"
- "**.m"
- "**.mm"

- name: Mac iOS clang-tidy
recipe: engine/engine_lint
Expand All @@ -345,6 +381,19 @@ targets:
lint_host: "false"
lint_ios: "true"
timeout: 75
runIf:
- DEPS
- .ci.yaml
- tools/**
- ci/**
- "**.h"
- "**.c"
- "**.cc"
- "**.fbs"
- "**.frag"
- "**.vert"
- "**.m"
- "**.mm"

- name: Mac iOS Engine
recipe: engine/engine
Expand Down
6 changes: 6 additions & 0 deletions ci/lint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ if [ ! -f "$COMPILE_COMMANDS" ]; then
(cd "$SRC_DIR"; ./flutter/tools/gn)
fi

echo "$(date +%T) Running clang_tidy"

cd "$SCRIPT_DIR"
"$DART" \
--disable-dart-dev \
Expand All @@ -49,9 +51,13 @@ cd "$SCRIPT_DIR"
--mac-host-warnings-as-errors="$MAC_HOST_WARNINGS_AS_ERRORS" \
"$@"

echo "$(date +%T) Running pylint"

cd "$FLUTTER_DIR"
pylint-2.7 --rcfile=.pylintrc \
"build/" \
"ci/" \
"impeller/" \
"tools/gn"

echo "$(date +%T) Linting complete"