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.

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

add lint script for Android with baseline #8036

Merged
merged 2 commits into from
Mar 5, 2019

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Mar 5, 2019

Fixes flutter/flutter#28847 (still need post-submits Cirrus will run this on pre and post submits)

This will start running the linter on our Java files for each commit and catch if any new violations are created.

As we fix things, we can rerun the script with --rebaseline (which removes the baseline.xml file and regenerates it).

/cc @mklim @matthew-carroll @gspencergoog

@matthew-carroll
Copy link
Contributor

But this isn't gonna block PRs, and reviewers aren't going to expect this to necessarily pass, right?

@dnfield
Copy link
Contributor Author

dnfield commented Mar 5, 2019

This will mark a PR as failed unless it explicitly has re-created the baseline.xml file.

As we fix things, we should rebaseline. If we introduce new errors, we should make sure we have good reason to and that they won't cause runtime crashes.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 5, 2019

In case it's not clear, this PR does not actually fix any of our existing lint issues - it just sets up a file to track what they are and hopefully avoid introducing more.

@matthew-carroll
Copy link
Contributor

I'm not sure if I'm misunderstanding something. We discussed failing PRs earlier today and agreed that we would not block any PRs on this linter....I don't know what rebaseline means, but it sounds like you're now saying this will block PRs. Am I understanding that correctly?

@jonahwilliams
Copy link
Member

My understanding is that baseline file contains all existing errors, and this only prevents us from adding more?

@dnfield
Copy link
Contributor Author

dnfield commented Mar 5, 2019

If you want to introduce what would cause a new linting error, you'd have to run dart tools/anroid_lint/bin/main.dart --rebaseline. That would re-generate the baseline.xml file, which would account for any new errors. A reviewer would have to LGTM allowing the new lint violation.

If we determine lints that we want to ignore, we can add those seperately. Right now we want to be sure to avoid adding NewApi violations that cause runtime crashes.

@matthew-carroll
Copy link
Contributor

If what @jonahwilliams describes is true, then we can give this a try. But if it starts creating a bottleneck for me in the refactor then I'm gonna ask that we turn it back off because that timeline is far more important than imposing these restrictions right now.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 5, 2019

Yes - and we should eventually get to a point where the "baseline" file isn't necessary anymore because we've either fixed the issues or added the rule to an ignored rule list. This is purely to avoid adding new issues while we fix the old ones - or at least only add new ones we're consciously accepting.

@gspencergoog
Copy link
Contributor

I agree that the goal should be to get rid of the baseline entirely. We should always be taking the free advice that the linter can give us about our code quality. To that end, we should set a timeline and perhaps even an OKR to resolve all of the linter warnings, depending on the extent of the debt here. That way it can be prioritized and factored in with our other priorities and tasks (like the refactor, which takes priority right now).

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

@jonahwilliams' description is correct, this will treat existing linter failures as known issues and fail presubmits only if they introduce new ones.

A lot of the errors this linter is checking for, especially the NewApi check, are actual crashes and not subjective style issues or suggested best practices. This would have caught flutter/flutter#28640 for example. I think we should definitely start preventing any further introduction of bugs like that ASAP and work on reducing our existing ones. If the linter ends up causing problems I'd rather we disable specific arguable rules rather than turn it off entirely.

@dnfield
Copy link
Contributor Author

dnfield commented Mar 5, 2019

Cirrus has actually finished all the tasks for this, the yellow one is actually green. Landing.

@dnfield dnfield merged commit 14e082f into flutter:master Mar 5, 2019
@dnfield dnfield deleted the lint_with_baseline branch March 5, 2019 17:36
cbracken added a commit to cbracken/flutter that referenced this pull request Mar 5, 2019
flutter/engine@f4951df19 (upstream/master) Minor cleanups to CONTRIBUTING.md (flutter/engine#8043)
flutter/engine@effee2f80 remove extra statement (flutter/engine#8041)
flutter/engine@14e082fa0 (master) add lint script for Android with baseline (flutter/engine#8036)
flutter/engine@5fed42197 Roll src/third_party/skia 25bc174cf682..72542816cadb (14 commits) (flutter/engine#8040)
flutter/engine@e6a5201f0 Add missing values to semantics action enums (flutter/engine#8033)
flutter/engine@75d4db31d Roll src/third_party/skia fbc887df72ec..25bc174cf682 (7 commits) (flutter/engine#8039)
flutter/engine@c46226980 Roll src/third_party/skia 0a57971f0544..fbc887df72ec (1 commits) (flutter/engine#8037)
flutter/engine@ed628da00 Add missing kHasImplicitScrolling enum value (flutter/engine#8030)
flutter/engine@052774e1c Roll src/third_party/skia e3e80b7edfd1..0a57971f0544 (16 commits) (flutter/engine#8035)
flutter/engine@2cd9b41ba Select MPL instead of LGPL (flutter/engine#7991)
flutter/engine@629c0d836 Roll src/third_party/dart 7d8ffe0daf..571ea80e11 (3 commits)
flutter/engine@8f1fdcd19 Android Embedding PR 14: Almost done with FlutterFragment. (flutter/engine#8000)
flutter/engine@fb3e35d6a Android Embedding PR15: Add Viewport Metrics to FlutterView (flutter/engine#8029)
flutter/engine@36ca5740c (flutter/engine#8026)
flutter/engine@3d53e2026 Roll src/third_party/skia 5800f2e8a920..e3e80b7edfd1 (2 commits) (flutter/engine#8028)
flutter/engine@f3d4a7f71 Roll src/third_party/dart 7c70ab1817..7d8ffe0daf (77 commits)
flutter/engine@a2246c199 Start of linting Android embedding (flutter/engine#8023)
flutter/engine@48bf4803e [fuchsia] Fix snapshot BUILD.gn file for Fuchsia roll (flutter/engine#8024)
cbracken added a commit to flutter/flutter that referenced this pull request Mar 6, 2019
flutter/engine@f4951df19 (upstream/master) Minor cleanups to CONTRIBUTING.md (flutter/engine#8043)
flutter/engine@effee2f80 remove extra statement (flutter/engine#8041)
flutter/engine@14e082fa0 (master) add lint script for Android with baseline (flutter/engine#8036)
flutter/engine@5fed42197 Roll src/third_party/skia 25bc174cf682..72542816cadb (14 commits) (flutter/engine#8040)
flutter/engine@e6a5201f0 Add missing values to semantics action enums (flutter/engine#8033)
flutter/engine@75d4db31d Roll src/third_party/skia fbc887df72ec..25bc174cf682 (7 commits) (flutter/engine#8039)
flutter/engine@c46226980 Roll src/third_party/skia 0a57971f0544..fbc887df72ec (1 commits) (flutter/engine#8037)
flutter/engine@ed628da00 Add missing kHasImplicitScrolling enum value (flutter/engine#8030)
flutter/engine@052774e1c Roll src/third_party/skia e3e80b7edfd1..0a57971f0544 (16 commits) (flutter/engine#8035)
flutter/engine@2cd9b41ba Select MPL instead of LGPL (flutter/engine#7991)
flutter/engine@629c0d836 Roll src/third_party/dart 7d8ffe0daf..571ea80e11 (3 commits)
flutter/engine@8f1fdcd19 Android Embedding PR 14: Almost done with FlutterFragment. (flutter/engine#8000)
flutter/engine@fb3e35d6a Android Embedding PR15: Add Viewport Metrics to FlutterView (flutter/engine#8029)
flutter/engine@36ca5740c (flutter/engine#8026)
flutter/engine@3d53e2026 Roll src/third_party/skia 5800f2e8a920..e3e80b7edfd1 (2 commits) (flutter/engine#8028)
flutter/engine@f3d4a7f71 Roll src/third_party/dart 7c70ab1817..7d8ffe0daf (77 commits)
flutter/engine@a2246c199 Start of linting Android embedding (flutter/engine#8023)
flutter/engine@48bf4803e [fuchsia] Fix snapshot BUILD.gn file for Fuchsia roll (flutter/engine#8024)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We should be linting the Android embedding
6 participants