Start of linting Android embedding #8023

dnfield · 2019-03-04T18:45:39Z

Tool for linting the Android embedding.

tools/android_lint/.gitignore

tools/android_lint/bin/main.dart

gspencergoog · 2019-03-04T18:53:47Z

tools/android_lint/bin/main.dart

+  argParser.addOption(
+    'in',
+    help: 'The path to `engine/src`.',
+    defaultsTo: path.join(path.current),


You could have it find the default based on the location of the script, using Platform.script:

final String flutterRoot = path.dirname(path.dirname(path.dirname(path.fromUri(Platform.script))));

This makes it possible to run it from any dir without needing to specify --in (although you should keep that as an option).

Done with a little twist to make sure it's still relative so that project.xml comes out ok on other systems

mklim

LGTM

tools/android_lint/bin/main.dart

gspencergoog

gspencergoog · 2019-03-04T19:24:37Z

tools/android_lint/bin/main.dart

@@ -0,0 +1,120 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


I'm worried the license checker won't be smart enough to figure out this doesn't impact licensing :(

FYI @gspencergoog @dnfield my understanding is that 2013 should be the year used on all licenses within the embedding.

Nevermind, this file lives somewhere else.

gspencergoog · 2019-03-04T19:27:33Z

tools/android_lint/bin/main.dart

+  print(result.stdout);
+  // TODO(dnfield): once we know what a clean lint will look like for this
+  // project, we should detect it and set the exit code based on it.
+  // Android Lint does _not_ set the exit code to non-zero if it detects lint


Whoa. Seems like quite an oversight in the lint tool to not report its results via exit code.

Ah! There's an option for it.

tools/android_lint/bin/main.dart

matthew-carroll · 2019-03-04T20:45:43Z

tools/android_lint/project.xml

@@ -0,0 +1,81 @@
+<!-- THIS FILE IS GENERATED. PLEASE USE THE INCLUDED DART PROGRAM  WHICH -->
+<!-- WILL AUTOMATICALLY FIND ALL .java FILES AND INCLUDE THEM HERE       -->


Should you find all Java files, or only the Java files listed in BUILD.gn?

We could use BUILD.gn - in which case we could probably write this as a build rule. I don't really have a strong opinion on that - would we want unlinted files in the repo that aren't part of the build though?

I guess the question is, why would a Java file exist without being a part of BUILD.gn....and if no such situation can occur, then why not just yank the Java file paths out of BUILD.gn?

matthew-carroll · 2019-03-04T20:46:33Z

tools/android_lint/bin/main.dart

+import 'package:path/path.dart' as path;
+import 'package:process/process.dart';
+
+/// Runs the Android SDK Lint tool on flutter/shell/platform/android.


When will this run? And what will be the impact on the fact that most files will not conform to our desired styles?

Right now it would be run by hand. Longer term we want to run it as part of CI.

If we determine that the linter is suggesting things that violate our desired styles we can turn them off. For now we want to just try to catch as much as we can.

But these aren't being run on PRs are they? What I want to avoid is being forced to make changes to existing files just because I had to alter a constructor argument, etc.

matthew-carroll · 2019-03-04T20:48:37Z

tools/android_lint/bin/main.dart

+/// At the time of this writing, the Android Lint tool doesn't work well with
+/// Java > 1.8.  This script will print a warning if you are not running
+/// Java 1.8.
+Future<void> main(List<String> args) async {


Can we break this function down? Something like:

request = parseArgs(args); writeProjectFile(request); result = runLint(); return result;

More or less done - I could definitely get fancier with this, but right now I just want something that works.

matthew-carroll · 2019-03-04T20:49:55Z

tools/android_lint/bin/main.dart

+/// Runs the Android SDK Lint tool on flutter/shell/platform/android.
+///
+/// This script scans the flutter/shell/platform/android directory for Java
+/// files to build a `project.xml` file.  This file is then passed to the lint


Where are the lint rules that are being applied?

Right now all of the default ones and all warnings, which are all getting elevated to errors.

But the linter includes style doesn't it? If so, which of the many flavors of java/Android style is it applying?

I'm also curious where these are actually defined. I don't know any default lint rules off the top of my head.

This is the public doc for it: https://developer.android.com/studio/write/lint

I don't know that it actually checks what we'd be calling style - or certainly not everything in the Java style guide.

matthew-carroll · 2019-03-04T21:54:44Z

LGTM - discussed offline, no PR blockages at this point.

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)

Start of linting Android embedding

ee16042

dnfield requested review from mklim and matthew-carroll March 4, 2019 18:45

googlebot added the cla: yes label Mar 4, 2019

gspencergoog requested changes Mar 4, 2019

View reviewed changes

mklim approved these changes Mar 4, 2019

View reviewed changes

tools/android_lint/bin/main.dart Show resolved Hide resolved

tools/android_lint/bin/main.dart Outdated Show resolved Hide resolved

dnfield added 3 commits March 4, 2019 11:12

review

48a9309

bump minSdk for androidmanifest.xml

65e4671

doc comment

f41eac7

gspencergoog approved these changes Mar 4, 2019

View reviewed changes

dnfield added 2 commits March 4, 2019 11:28

check java version

9df4d05

exit code

a36b8fc

matthew-carroll reviewed Mar 4, 2019

View reviewed changes

refactor script to use methods

68b5212

dnfield merged commit a2246c1 into flutter:master Mar 4, 2019

dnfield deleted the android_lint branch March 4, 2019 21:55

dnfield mentioned this pull request Mar 5, 2019

We should be linting the Android embedding flutter/flutter#28847

Closed

4 tasks

cbracken mentioned this pull request Mar 5, 2019

Roll engine 4f54a1dd9...e6a5201f0 flutter/flutter#28899

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start of linting Android embedding #8023

Start of linting Android embedding #8023

dnfield commented Mar 4, 2019 •

edited

gspencergoog Mar 4, 2019

dnfield Mar 4, 2019

mklim left a comment

gspencergoog left a comment

gspencergoog Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll Mar 4, 2019

matthew-carroll Mar 4, 2019

gspencergoog Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll Mar 4, 2019

matthew-carroll Mar 4, 2019

dnfield Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll Mar 4, 2019

matthew-carroll Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll Mar 4, 2019

matthew-carroll Mar 4, 2019

dnfield Mar 4, 2019

matthew-carroll commented Mar 4, 2019

		@@ -0,0 +1,120 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

		@@ -0,0 +1,81 @@
		<!-- THIS FILE IS GENERATED. PLEASE USE THE INCLUDED DART PROGRAM WHICH -->
		<!-- WILL AUTOMATICALLY FIND ALL .java FILES AND INCLUDE THEM HERE -->

Start of linting Android embedding #8023

Start of linting Android embedding #8023

Conversation

dnfield commented Mar 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mklim left a comment

Choose a reason for hiding this comment

gspencergoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Mar 4, 2019

dnfield commented Mar 4, 2019 •

edited