Allow engine variants other than "host_debug" to be clang-tidy linted #29668

jmagman · 2021-11-11T03:50:42Z

Add a new --target-variant and --src-dir option to the clang-tidy linter script.
Update lint.sh to pass along the src-dir instead of the compile-commands path. The dart script will interpret this as src-dir/out/host_debug/compile_commands.json by default. Alternative --target-variant can be passed into the shell script.

$ ci/lint.sh --target-variant ios_debug

This will allow variants (is that the right word here?) other than host_debug to be run by the linter.

The recipe can then run the script for different targets
https://flutter.googlesource.com/recipes/+/refs/heads/main/recipes/engine_unopt.py#99
will become something like:

Lint(api, 'ios_debug_unopt')

def Lint(api, variant):
  checkout = GetCheckoutPath(api)
  with api.context(cwd=checkout):
    lint_cmd = checkout.join('flutter', 'ci', 'lint.sh', '--lint-all', '--target-variant', variant)
    api.step(api.test_utils.test_step_name('lint test'), [lint_cmd])

Note: in the future when the recipe is updated to use --target-variant, any outstanding PRs older than this script change will fail until merged in/rebased. I don't think it will impact future releases since they are versioned (will a beta release without this change run with that recipe? @christopherfujino )

Git pre-push hook continues to pass in --compile-commands and still works.

Other improvements:

--format-style=file to use .clang-format file for fixes.
Show all linter errors, not just the first one found.
Remove the --repo option, calculate that directory relative to the script path.

flutter/flutter#61661

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.

tools/clang_tidy/lib/src/options.dart

zanderso · 2021-11-11T06:04:45Z

tools/clang_tidy/lib/src/options.dart

+import 'package:path/path.dart' as path;
+
+// Path to root of the flutter/engine repository containing this script.
+final String _engineRoot = path.dirname(path.dirname(path.dirname(path.dirname(path.fromUri(io.Platform.script)))));


If you're computing this here, maybe we don't need the --repo command line argument anymore. Or the other way around, if you have the --repo argument, this is just the parent directory of that, right?

Was there any case where --repo needs to handle some other repo directory? I wasn't sure why it was passed in in the first place.

This pattern is probably safe here since we're always running the script from source, but it would break if the script were compiled to a snapshot and then the snapshot moved somewhere else before being run.

We can keep this and get rid of the --repo argument if that is simpler.

it would break if the script were compiled to a snapshot and then the snapshot moved somewhere else before being run.

If you do that and it doesn't work: Give The Script A Break 🙂

Removed --repo.

jmagman · 2021-11-11T08:25:56Z

tools/clang_tidy/lib/clang_tidy.dart

@@ -229,16 +229,8 @@ class ClangTidy {
      if (job.result.exitCode == 0) {
        continue;
      }
-      if (job.exception != null) {


Updated to remove this block. It was only showing the first file linter errors because a linter failure makes the process fail, which populates the job.exception, and breaks so doesn't show the other job failures.

zanderso · 2021-11-11T13:56:24Z

tools/clang_tidy/lib/src/options.dart

+import 'package:path/path.dart' as path;
+
+// Path to root of the flutter/engine repository containing this script.
+final String _engineRoot = path.dirname(path.dirname(path.dirname(path.dirname(path.fromUri(io.Platform.script)))));


This pattern is probably safe here since we're always running the script from source, but it would break if the script were compiled to a snapshot and then the snapshot moved somewhere else before being run.

We can keep this and get rid of the --repo argument if that is simpler.

christopherfujino

this shouldn't be an issue for beta branches, provided the engine builds were passing at the time the branch was made.

…y linted (flutter/engine#29668)

jmagman self-assigned this Nov 11, 2021

google-cla bot added the cla: yes label Nov 11, 2021

Allow engine variant to be passed into linter script

15a6f9e

jmagman force-pushed the lint-variant branch from e667d37 to 15a6f9e Compare November 11, 2021 03:57

jmagman requested a review from zanderso November 11, 2021 05:14

zanderso reviewed Nov 11, 2021

View reviewed changes

jmagman added 3 commits November 10, 2021 22:25

check options

78845fd

Fix help message

1bb1424

Show all lint failures, not just the first

b04ab6b

jmagman commented Nov 11, 2021

View reviewed changes

Use .clang-format when fixing files

6e33389

jmagman force-pushed the lint-variant branch from 8d4bec4 to 6e33389 Compare November 11, 2021 08:40

zanderso approved these changes Nov 11, 2021

View reviewed changes

christopherfujino reviewed Nov 11, 2021

View reviewed changes

Remove --repo

23d6a03

jmagman added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 11, 2021

fluttergithubbot merged commit 18fc3b4 into flutter:master Nov 11, 2021

jmagman deleted the lint-variant branch November 11, 2021 22:00

engine-flutter-autoroll mentioned this pull request Nov 11, 2021

Roll Engine from aaacb670af16 to e022c9283c40 (4 revisions) flutter/flutter#93510

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 11, 2021

18fc3b4 Allow engine variants other than "host_debug" to be clang-tid…

23241f1

…y linted (flutter/engine#29668)

This was referenced Nov 19, 2021

iOS Background Platform Channels #29665

Merged

Cherrypick #29783 to flutter-2.8-candidate.6 #29812

Merged

Embedder Asset Provider #29802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow engine variants other than "host_debug" to be clang-tidy linted #29668

Allow engine variants other than "host_debug" to be clang-tidy linted #29668

Uh oh!

jmagman commented Nov 11, 2021 •

edited

Loading

Uh oh!

Uh oh!

zanderso Nov 11, 2021

Uh oh!

jmagman Nov 11, 2021

Uh oh!

zanderso Nov 11, 2021

Uh oh!

jmagman Nov 11, 2021

Uh oh!

jmagman Nov 11, 2021

Uh oh!

jmagman Nov 11, 2021

Uh oh!

zanderso Nov 11, 2021

Uh oh!

zanderso Nov 11, 2021

Uh oh!

christopherfujino left a comment

Uh oh!

Uh oh!

Allow engine variants other than "host_debug" to be clang-tidy linted #29668

Allow engine variants other than "host_debug" to be clang-tidy linted #29668

Uh oh!

Conversation

jmagman commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmagman commented Nov 11, 2021 •

edited

Loading