Bug 1956870 - Adding gradlew lint to android-{fenix|focus|ac} linters and reporting lint errors on phabricator. r=android-reviewers,nalexander

Currently CI runs gradlew lint for android projects using build-fat-aar which is not needed now that we can run lint from root project. Current setup also doesn't update mozlint.json so phabricator doesn't recognize the lint failures. This bug adds gradlew lint to mach lint through the android-{fenix|focus|ac} linters in addition of ktlint and detekt which were added in D216999

This patch changes android lint configuration to generate JSON report of errors and then reads these errors in lints.py to report in the same format as other linters.

Creating new treeherder task for each of `android-{fenix|focus|ac}` linter as android-fenix can take up to 20 minutes ot run now with the addition of gradlew lint, which is almost the same as the existing mozlint-android-lints task takes, so doing this in parallel will be nice.

Follow up work would include removing all the original detekt, ktlint and lint tasks from treeherder which will make creating these new tasks more justified.

Differential Revision: https://phabricator.services.mozilla.com/D243832

build.gradle

@@ -535,3 +535,12 @@ subprojects { project ->
         }
     }
 }
+
+tasks.register("lint-a-c") {
+    subprojects.each{
+        if (it.tasks.findByName("lint") != null && it.projectDir.absolutePath.contains("android-components")) {
+            dependsOn it.tasks.named("lint")
+            println("Subproject ${it.name} has a lint task")
+        }
+    }
+}

mobile/android/android-components/android-lint.gradle

@@ -1,7 +1,7 @@
 android {
     lint {
         warningsAsErrors = true
-        abortOnError = true
+        abortOnError = false
 
                 // With our L10N process its totally possible to have missing or (temporarily) extra translations.
         disable 'MissingTranslation',
@@ -15,5 +15,7 @@ android {
                 // "We do not impose rules on locales"
                 // https://github.com/mozilla-mobile/android-components/pull/11069
                 'TypographyDashes'
+        sarifReport true
+        sarifOutput file("../../../build/reports/lint/lint-report-${project.name}.sarif.json")
     }
 }

mobile/android/android-components/components/compose/base/src/main/java/mozilla/components/compose/base/modifier/Modifier.kt

@@ -10,6 +10,7 @@ import androidx.annotation.FloatRange
 import androidx.compose.foundation.clickable
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableLongStateOf
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.setValue
@@ -84,7 +85,7 @@ fun Modifier.debouncedClickable(
     debounceInterval: Long = 1000L,
     onClick: () -> Unit,
 ) = composed {
-    var lastClickTime: Long by remember { mutableStateOf(0) }
+    var lastClickTime: Long by remember { mutableLongStateOf(0) }
 
     this.then(
         Modifier.clickable(

mobile/android/android-components/components/feature/search/src/main/java/mozilla/components/feature/search/internal/SearchUrlBuilder.kt

@@ -79,7 +79,7 @@ private fun paramSubstitution(template: String, query: String, inputEncoding: St
 }
 
 private fun normalize(input: String): String {
-    val trimmedInput = input.trim { it <= ' ' }
+    val trimmedInput = input.trim()
     var uri = trimmedInput.toUri()
 
     if (TextUtils.isEmpty(uri.scheme)) {

mobile/android/android-components/components/support/utils/src/main/java/mozilla/components/support/ktx/util/URLStringUtils.kt

@@ -148,7 +148,7 @@ object URLStringUtils {
      * Determine whether a string is a valid search query URL.
      */
     fun isValidSearchQueryUrl(url: String): Boolean {
-        var trimmedUrl = url.trim { it <= ' ' }
+        var trimmedUrl = url.trim()
         if (!trimmedUrl.matches("^.+?://.+?".toRegex())) {
             // UI hint url doesn't have http scheme, so add it if necessary
             trimmedUrl = "http://$trimmedUrl"

mobile/android/fenix/app/build.gradle

@@ -248,6 +248,9 @@ android {
     lint {
         lintConfig = file("lint.xml")
         baseline = file("lint-baseline.xml")
+        sarifReport = true
+        sarifOutput = file("../build/reports/lint/lint-report.sarif.json")
+        abortOnError = false
     }
 
     // https://issuetracker.google.com/issues/379732901

mobile/android/fenix/app/src/main/java/org/mozilla/fenix/addons/AddonDetailsBindingDelegate.kt

@@ -30,7 +30,7 @@ import java.util.Locale
 interface AddonDetailsInteractor {
 
     /**
-     * Open the given URL in the browser.
+     * Open the given URL in the browser.a
      */
     fun openWebsite(url: Uri)
 

mobile/android/focus-android/app/build.gradle

@@ -61,6 +61,9 @@ android {
     lint {
         lintConfig = file("lint.xml")
         baseline = file("lint-baseline.xml")
+        sarifReport = true
+        sarifOutput = file("../build/reports/lint/lint-report.sarif.json")
+        abortOnError = false
     }
 
     // We have a three dimensional build configuration:

mobile/android/focus-android/app/src/main/java/org/mozilla/focus/fragment/AddToHomescreenDialogFragment.kt

@@ -93,12 +93,12 @@ class AddToHomescreenDialogFragment : DialogFragment() {
                 requireContext(),
                 IconGenerator.generateLauncherIcon(requireContext(), iconUrl),
                 iconUrl,
-                editableTitle.text.toString().trim { it <= ' ' },
+                editableTitle.text.toString().trim(),
                 blockingEnabled,
                 requestDesktop,
             )
 
-            val hasEditedTitle = initialTitle != editableTitle.text.toString().trim { it <= ' ' }
+            val hasEditedTitle = initialTitle != editableTitle.text.toString().trim()
             AddToHomeScreen.addButtonTapped.record(
                 AddToHomeScreen.AddButtonTappedExtra(
                     hasEditedTitle = hasEditedTitle,

mobile/android/focus-android/app/src/main/java/org/mozilla/focus/fragment/UrlInputFragment.kt

@@ -501,7 +501,7 @@ class UrlInputFragment :
     }
 
     internal fun onCommit(input: String) {
-        if (input.trim { it <= ' ' }.isNotEmpty()) {
+        if (input.trim().isNotEmpty()) {
             handleCrashTrigger(input)
 
             binding.browserToolbar.hideKeyboard()
@@ -612,7 +612,7 @@ class UrlInputFragment :
     internal fun onTextChange(text: String) {
         searchSuggestionsViewModel.setSearchQuery(text)
 
-        if (text.trim { it <= ' ' }.isEmpty()) {
+        if (text.trim().isEmpty()) {
             binding.searchViewContainer.isVisible = false
 
             if (!isOverlay) {

mobile/android/focus-android/app/src/main/java/org/mozilla/focus/shortcut/HomeScreen.kt

@@ -66,7 +66,7 @@ object HomeScreen {
         blockingEnabled: Boolean,
         requestDesktop: Boolean,
     ) {
-        val shortcutTitle = if (TextUtils.isEmpty(title.trim { it <= ' ' })) {
+        val shortcutTitle = if (TextUtils.isEmpty(title.trim())) {
             generateTitleFromUrl(url)
         } else {
             title

taskcluster/kinds/source-test/mozlint-android.yml

@@ -3,11 +3,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 ---
 
-# Just one TC job that runs all the lints.  There's no real advantage
-# to running them separately, and this way we perform the expensive
-# clone and somewhat expensive compilation of the Android
-# intermediates just once.
-lints:
+task-defaults:
     always-target: true
     attributes:
         build_platform: android
@@ -16,7 +12,6 @@ lints:
     description: Android lints
     platform: lint/opt
     treeherder:
-        symbol: A(lints)
         kind: test
         tier: 1
     worker-type: t-linux-docker
@@ -62,6 +57,20 @@ lints:
               type: directory
     run:
         using: run-task
+        tooltool-downloads: internal  # For internal toolchains.
+    fetches:
+        toolchain:
+            # Aliases aren't allowed for toolchains installed by fetch.
+            - linux64-android-gradle-dependencies
+            - linux64-android-sdk-linux-repack
+            - linux64-jdk-repack
+            - linux64-node
+
+
+lints:
+    treeherder:
+        symbol: A(lints)
+    run:
         command: >
             ln -s $MOZ_FETCHES_DIR/android-gradle-dependencies $GECKO_PATH &&
             ln -s $MOZ_FETCHES_DIR/android-sdk-linux $GECKO_PATH &&
@@ -75,18 +84,7 @@ lints:
             --linter android-checkstyle
             --linter android-lint
             --linter android-test
-            --linter android-fenix
-            --linter android-ac
-            --linter android-focus
             *
-        tooltool-downloads: internal  # For internal toolchains.
-    fetches:
-        toolchain:
-            # Aliases aren't allowed for toolchains installed by fetch.
-            - linux64-android-gradle-dependencies
-            - linux64-android-sdk-linux-repack
-            - linux64-jdk-repack
-            - linux64-node
     when:
         files-changed:
             # Source files.
@@ -107,3 +105,87 @@ lints:
             # Other misc lint related files.
             - 'python/mozlint/**'
             - 'tools/lint/**'
+
+fenix:
+    worker-type: t-linux-xlarge-source
+    treeherder:
+        symbol: A(fenix-lints)
+    run:
+        command: >
+            ln -s $MOZ_FETCHES_DIR/android-gradle-dependencies $GECKO_PATH &&
+            ln -s $MOZ_FETCHES_DIR/android-sdk-linux $GECKO_PATH &&
+            ln -s $MOZ_FETCHES_DIR/node $GECKO_PATH &&
+            cd $GECKO_PATH &&
+            ./mach --log-no-times build pre-export export &&
+            ./mach --log-no-times lint -f treeherder -f json:/builds/worker/mozlint.json
+            --linter android-fenix
+            *
+    when:
+        files-changed:
+            # Source files.
+            - 'mobile/android/fenix/**'
+            # Build stuff.
+            - 'mobile/android/**/Makefile.in'
+            - 'mobile/android/config/**'
+            - 'mobile/android/gradle.configure'
+            - 'mobile/android/**/moz.build'
+            - '**/*.gradle'
+            # Other misc lint related files.
+            - 'python/mozlint/**'
+            - 'tools/lint/**'
+
+focus:
+    worker-type: t-linux-xlarge-source
+    treeherder:
+        symbol: A(focus-lints)
+    run:
+        command: >
+            ln -s $MOZ_FETCHES_DIR/android-gradle-dependencies $GECKO_PATH &&
+            ln -s $MOZ_FETCHES_DIR/android-sdk-linux $GECKO_PATH &&
+            ln -s $MOZ_FETCHES_DIR/node $GECKO_PATH &&
+            cd $GECKO_PATH &&
+            ./mach --log-no-times build pre-export export &&
+            ./mach --log-no-times lint -f treeherder -f json:/builds/worker/mozlint.json
+            --linter android-focus
+            *
+    when:
+        files-changed:
+            # Source files.
+            - 'mobile/android/focus-android/**'
+            # Build stuff.
+            - 'mobile/android/**/Makefile.in'
+            - 'mobile/android/config/**'
+            - 'mobile/android/gradle.configure'
+            - 'mobile/android/**/moz.build'
+            - '**/*.gradle'
+            # Other misc lint related files.
+            - 'python/mozlint/**'
+            - 'tools/lint/**'
+
+android-components:
+    worker-type: t-linux-xlarge-source
+    treeherder:
+        symbol: A(ac-lints)
+    run:
+        command: >
+            ln -s $MOZ_FETCHES_DIR/android-gradle-dependencies $GECKO_PATH &&
+            ln -s $MOZ_FETCHES_DIR/android-sdk-linux $GECKO_PATH &&
+            ln -s $MOZ_FETCHES_DIR/node $GECKO_PATH &&
+            cd $GECKO_PATH &&
+            ./mach --log-no-times build pre-export export &&
+            ./mach --log-no-times lint -f treeherder -f json:/builds/worker/mozlint.json
+            --linter android-ac
+            *
+    when:
+        files-changed:
+            # Source files.
+            - 'mobile/android/android-components/**'
+            # Build stuff.
+            - 'mobile/android/**/Makefile.in'
+            - 'mobile/android/config/**'
+            - 'mobile/android/gradle.configure'
+            - 'mobile/android/**/moz.build'
+            - '**/*.gradle'
+            # Other misc lint related files.
+            - 'python/mozlint/**'
+            - 'tools/lint/**'

tools/lint/android/lints.py

@@ -162,6 +162,10 @@ def fenix_format(_paths, config, fix=None, **lintargs):
         config,
         fix,
         os.path.join("mobile", "android", "fenix"),
+        lint_tasks=[
+            "fenix:lint",
+            "fenix:lintFenixDebug",
+        ],
         **lintargs,
     )
 
@@ -171,6 +175,7 @@ def ac_format(_paths, config, fix=None, **lintargs):
         config,
         fix,
         os.path.join("mobile", "android", "android-components"),
+        lint_tasks=["lint-a-c"],
         **lintargs,
     )
 
@@ -180,11 +185,12 @@ def focus_format(_paths, config, fix=None, **lintargs):
         config,
         fix,
         os.path.join("mobile", "android", "focus-android"),
+        lint_tasks=["focus-android:lint"],
         **lintargs,
     )
 
 
-def report_gradlew(config, fix, subdir, **lintargs):
+def report_gradlew(config, fix, subdir, lint_tasks=[], **lintargs):
     topsrcdir = lintargs["root"]
     topobjdir = lintargs["topobjdir"]
 
@@ -195,13 +201,12 @@ def report_gradlew(config, fix, subdir, **lintargs):
 
     extra_args = lintargs.get("extra_args") or []
 
-    for task in tasks:
     gradle(
         lintargs["log"],
         topsrcdir=topsrcdir,
         topobjdir=topobjdir,
-            tasks=[task],
-            extra_args=extra_args + ["-p", os.path.join(topsrcdir, subdir)],
+        tasks=tasks,
+        extra_args=extra_args + ["-p", os.path.join(topsrcdir, subdir), "--continue"],
     )
 
     reports = os.path.join(topsrcdir, subdir, "build", "reports")
@@ -216,16 +221,21 @@ def report_gradlew(config, fix, subdir, **lintargs):
                 elif f.startswith(subdir):
                     excludes.append(f.strip())
 
+    detekt_report = None
+    if os.path.exists(os.path.join(reports, "detekt", "detekt.xml")):
+        detekt_report = os.path.join(reports, "detekt", "detekt.xml")
+    elif os.path.join(
+        topobjdir, "gradle", "build", subdir, "reports", "detekt", "detekt.xml"
+    ):
+        detekt_report = os.path.join(
+            topobjdir, "gradle", "build", subdir, "reports", "detekt", "detekt.xml"
+        )
+    else:
+        print(f"Could not read detekt report: '{detekt_report}'")
+        pass
+
     try:
-        tree = ET.parse(
-            open(
-                os.path.join(
-                    reports,
-                    "detekt",
-                    "detekt.xml",
-                ),
-            )
-        )
+        tree = ET.parse(open(detekt_report))
         root = tree.getroot()
 
         for file in root.findall("file"):
@@ -243,6 +253,7 @@ def report_gradlew(config, fix, subdir, **lintargs):
                 }
                 results.append(result.from_config(config, **err))
     except FileNotFoundError:
+        print(f"Could not read detekt report: '{detekt_report}'")
         pass
 
     ktlint_file = "ktlint.json"
@@ -274,9 +285,10 @@ def report_gradlew(config, fix, subdir, **lintargs):
                 }
                 results.append(result.from_config(config, **err))
     except FileNotFoundError:
+        print(f"Could not read ktlint report: `{ktlint_file}`")
         pass
 
-    return results
+    return results + read_lint_report(config, subdir, tasks=lint_tasks, **lintargs)
 
 
 def is_excluded_file(topsrcdir, excludes, file):
@@ -407,6 +419,98 @@ def lint(_paths, config, **lintargs):
     return results
 
 
+def read_lint_report(config, subdir, tasks=[], **lintargs):
+    topsrcdir = lintargs["root"]
+    topobjdir = lintargs["topobjdir"]
+
+    gradle(
+        lintargs["log"],
+        topsrcdir=topsrcdir,
+        topobjdir=topobjdir,
+        tasks=tasks,
+        extra_args=lintargs.get("extra_args") or [],
+    )
+
+    reports = os.path.join(topsrcdir, subdir, "build", "reports")
+
+    excludes = []
+    for path in EXCLUSION_FILES:
+        with open(os.path.join(topsrcdir, path)) as fh:
+            for f in fh.readlines():
+                if "*" in f:
+                    excludes.extend(glob.glob(f.strip()))
+                elif f.startswith(subdir):
+                    excludes.append(f.strip())
+
+    try:
+        files = os.listdir(
+            os.path.join(
+                reports,
+                "lint",
+            )
+        )
+
+        results = []
+        for file in files:
+            data = json.load(
+                open(
+                    os.path.join(reports, "lint", file),
+                )
+            ).get(
+                "runs", [{}]
+            )[0]
+
+            issues = data.get("results", [])
+            rules = data.get("tool", {}).get("driver", {}).get("rules", [])
+
+            for issue in issues:
+                dir = os.path.join(topsrcdir, subdir)
+                if subdir != os.path.join("mobile", "android", "android-components"):
+                    dir = os.path.join(topsrcdir, "mobile", "android")
+                name = os.path.join(
+                    dir,
+                    issue.get("locations", [{}])[0]
+                    .get("physicalLocation", {})
+                    .get("artifactLocation", {})
+                    .get("uri"),
+                )
+
+                if is_excluded_file(topsrcdir, excludes, name) and not "/res/" in name:
+                    continue
+
+                level = "error"
+                if "level" in issue:
+                    level = issue["level"]
+                elif "ruleIndex" in issue and len(rules) > issue["ruleIndex"]:
+                    rule_level = (
+                        rules[issue["ruleIndex"]]
+                        .get("defaultConfiguration", {})
+                        .get("level")
+                    )
+                    if rule_level:
+                        level = rule_level
+
+                err = {
+                    "rule": issue.get("ruleId"),
+                    "path": name,
+                    "lineno": issue.get("locations", [{}])[0]
+                    .get("physicalLocation", {})
+                    .get("region", {})
+                    .get("startLine"),
+                    "column": issue.get("locations", [{}])[0]
+                    .get("physicalLocation", {})
+                    .get("region", {})
+                    .get("startColumn"),
+                    "message": issue.get("message", {}).get("text"),
+                    "level": level,
+                }
+                results.append(result.from_config(config, **err))
+        return results
+    except FileNotFoundError:
+        print("Could not read lint report from ", subdir)
+        return []
+
+
 def _parse_checkstyle_output(config, topsrcdir=None, report_path=None):
     tree = ET.parse(open(report_path))
     root = tree.getroot()