Add Lint Check for Licenses In New Files

[ekager]

In #8946 I set up a module so we can add our custom lint checks. Adding one where we check licenses could save us some review time and prevent missed licenses.

It looks like we could do something like this to check the comments in a file: https://android.googlesource.com/platform/tools/base/+/studio-master-dev/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/CommentDetector.java

┆Issue is synchronized with this Jira Task

[juan-goncalves]

Is this still needed?
I could give it a shot

[ekager]

It's not super high priority, but feel free if you're interested! Some other bugs that are ready to be worked on here if you're looking! https://github.com/mozilla-mobile/fenix/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc+label%3Aeng%3Aready+label%3A%22%F0%9F%90%9E+bug%22

[juan-goncalves]

Thanks for the link @ekager! I'll be checking out the issues there

Still, I'd like to try my hand at custom lint rules, if possible

[ekager]

Yep feel free! This PR adding the existing lint checks may help, #8946

[juan-goncalves]

A question, how should the warning look inside the file?
Should it highlight the first line of the file or everything inside it?

Also, what should the severity of the issue be? (In the screenshots above its behaving as a warning)

[ekager]

I think a warning on the first line of the file works!

[juan-goncalves]

I have a working version that covers the Kotlin (and Java) source code files, but I think it's not possible to do this with the resource (XML) files.

The problem is that after experimenting with the ResourceXmlDetector (following the CommentDetector from the platform tools) I've realized that for some reason the visitDocument(context: XmlContext, document: Document) method in actual resource files is called with the root element, ignoring its possible comment siblings (where the license comment would be located).

For example, in the following unit test the first child of the document parameter is the comment node with the license, and for that reason the detector runs successfully without reporting any warning

    @Test
    fun `valid license in resource file does not generate a warning`() {
        val code = """
            |<!-- This Source Code Form is subject to the terms of the Mozilla Public
            |   - License, v. 2.0. If a copy of the MPL was not distributed with this
            |   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
            |
            |<alpha xmlns:android="http://schemas.android.com/apk/res/android"
            |       android:interpolator="@android:interpolator/decelerate_quad"
            |       android:fromAlpha="0.0" android:toAlpha="1.0"
            |       android:duration="250" />""".trimMargin()

        lint()
            .files(TestFiles.xml("res/anim/fade_in.xml", code))
            .issues(ISSUE_MISSING_LICENSE, ISSUE_INVALID_LICENSE_FORMAT)
            .run()
            .expectClean()
    }

But if the detector runs in a real resource file (with exactly the same contents) it does report a warning (missing license). In this case I'm running it against the res/anim/fade_in.xml file.

After adding some debug output to the warning message I see that it happens because the document parameter is the alpha node (which doesn't have any children), so there is some behavior difference when running it on the real resource files.

Besides, the alpha node in the resource file does not have any siblings so we can't access the comment nodes that are outside of the root element.

If anyone has any idea on why this is happening, let me know :)

Anyways, after running the lint (for Kotlin/Java files) on the app module, it reported that there are currently 9 files without the license and 16 that have it in a different format.

@ekager

[ekager]

@juan-goncalves sorry for the very long delay! I'm fine with starting it for just the Kotlin files if you have something working feel free to put up a PR :)

[juan-goncalves]

@ekager I forgot about it! I remember that at least for the Kotlin files it was working fine, let me update the branch (and I'll see if there has been any updates on the lint tools to see if the XML one works too)

[stale]

See: #17373 This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.