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

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 20, 2022

Only run clang-tidy builders in presubmit when C, C++, ObjC, or ObjC++ files change. Also run if the DEPS or CI files change. Run on Linux Android clang-tidy if any python files change, since that's the shortest-running builder. All builds will always run in post-submit.

To test the globs, the first commit 586b9e1 added a comment to a .mm file, and Mac Host clang-tidy did not include .ci.yaml. It was correctly scheduled just because of the extension:
https://ci.chromium.org/p/flutter/builders/try/Mac%20Host%20clang-tidy/760

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@jmagman jmagman self-assigned this Oct 20, 2022
@jmagman jmagman changed the title Only run clang-tidy when C, C++, ObjC, or ObjC++ files change Only run clang-tidy in presubmit when C, C++, ObjC, or ObjC++ files change Oct 20, 2022
@jmagman
Copy link
Member Author

jmagman commented Oct 20, 2022

Am I missing any extensions?

@jmagman jmagman requested a review from zanderso October 20, 2022 05:41
@jmagman jmagman marked this pull request as ready for review October 20, 2022 05:41
- tools/**
- ci/**
- "**.c"
- "**.cc"
Copy link
Member

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

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

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

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

Choose a reason for hiding this comment

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

Copy link
Member

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

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

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

@chinmaygarde
Copy link
Member

Ping @zanderso @jmagman. Not sure if there is progress being made here. Is this ready for review?

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2022
@auto-submit auto-submit bot merged commit 4aadfaa into flutter:main Oct 28, 2022
@jmagman jmagman deleted the clang-tidy-run-if branch October 28, 2022 18:25
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants