1529760 - New linting checker/rule request for GeckoView CHANGELOG

Status: NEW

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Hi! Hopefully this is the right place for this.

At GeckoView we would like a rule for Differential Revisions that creates a comment with the text

"Please update the CHANGELOG file at

mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md

describing your API changes."

Whenever the following condition is true:

Affected files contains mobile/android/geckoview/api.txt
Affected files does not contain mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md

Similar to this personal rule here: https://phabricator.services.mozilla.com/H125

Thanks!

Comment 1

[:glob ✱]

sounds like you want a linting check rather than a herald rule.

if someone implements a new checker then it can be added to reviewbot for generating phabricator comments.

see https://hg.mozilla.org/mozilla-central/rev/3eefd4e1087c for an example checker.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Ah cool! I can do that.

Comment 3

[Andrew Halberstadt [:ahal]]

Actually, I'm not sure a lint check would be the best way forward here, as it isn't normally tied to version control. While it does have an --outgoing flag, the normal case (both locally and in CI) is to run over all files in mozilla-central without any concept of a "push". So what would the linter use as the inputs in this case?

It would be possible to shoe-horn something like this into mach lint, but I'm not really sure why that would be better than a herald rule which seems like it was designed specifically for this kind of thing? Ideally we could implement some kind of mechanism that would allow developers to define herald rules in-tree.

Comment 4

[:glob ✱]

as phabricator runs each herald run on submission there are real performance implications if herald becomes the way to write these sorts of automated review checks.

smacleod has a meeting with relman this week to discuss reviewbot and phabricator integration, this bug has been added to the agenda.

Comment 5

[Andrew Halberstadt [:ahal]]

This could also get implemented as a push hook (local or server side). Also worth noting, I'm not totally opposed to writing this as a lint. Maybe there's some non-hacky way to get this and other linters like it integrated.

Aside: No matter the implementation, it would be great to have this capability in a more general sense, e.g I'd love to issue warnings if we can detect source API changes that don't update relevant documentation.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

I'm looking into writing a lint for this and it looks like lints are not passed the whole list of modified files?

I was hoping I could do a check like (simplified):

def lint(paths, config, **lintargs):
    if "api.txt" in paths and not "CHANGELOG.md" in paths:
        # add error

am I missing something? Is there a different way I should do this?

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

ni for Comment #6 :)

Comment 8

[:glob ✱]

(In reply to Agi | :agi | ⏰ EST | he/him from comment #6)

I'm looking into writing a lint for this and it looks like lints are not passed the whole list of modified files?

that appears to be the crux of ahal's concerns in comment 3.

(In reply to Andrew Halberstadt [:ahal] from comment #5)

Aside: No matter the implementation, it would be great to have this capability in a more general sense, e.g I'd love to issue warnings if we can detect source API changes that don't update relevant documentation.

💯

i can think of a few other commit/push-level checks that should be caught by automation as early in the review cycle as possible (eg. pretty well all of the hg hooks); the question is how? smacleod's discussion with relman should go some ways toward answering that question.

Comment 9

[Andrew Halberstadt [:ahal]]

Here is one approach to solve this properly (i.e, not the quick and dirty solution)..

Mozlint is kind of terribly named because it implies that it is only useful for linting. But it's really more of a "library to execute arbitrary tasks against specified files". In bug 1511122 I wrote up a proposal and attached a demo patch to create a new mozlint "instance" that lives in tools/format to act as the backend of a proposed |mach format| command.

I think we could do something similar here, namely, create a new mozlint instance to handle hooks. In this instance, there would be no mach frontend at all, and instead they would be registered as a push hook setup via mach bootstrap (possibly enforcing that it's enabled via moz-phab). In this new mozlint instance, we would always use hg outgoing to seed the list of files to check (as opposed to |mach lint| where paths are user specified).

This would let us register "lints" (or other tasks) that run at push/review time. Mozlint does have some linting specific logic baked in, so this would require some minor refactoring to get rid of it.

I'm going to leave the needinfo to think about quick and dirty solutions.

Comment 10

[Marco Castelluccio [:marco]]

We'd definitely like to have checks like this (or bug 1511414) defined in-tree and in a way that we can trigger from the review bot.

This would allow us to add warnings in Phabricator just like our other linting checks, without splitting them between Herald and the bot.

Comment 11

[Steven MacLeod [:smacleod]]

(In reply to Byron Jones ‹:glob› 🎈 from comment #4)

as phabricator runs each herald run on submission there are real performance implications if herald becomes the way to write these sorts of automated review checks.

smacleod has a meeting with relman this week to discuss reviewbot and phabricator integration, this bug has been added to the agenda.

Yup. Looks like we're all in agreement that in tree is preferred over Herald now.

Comment 12

[Andrew Halberstadt [:ahal]]

Steven, Marco do you guys have any preferences/ideas on how to implement this? I'm struggling to come up with a good quick and dirty solution.

Maybe the "quick and dirty" thing is the proposal in comment 9 except instead of refactoring mozlint to be less lint specific, just shoe-horn herald rules into the existing format.

Comment 13

[Marco Castelluccio [:marco]]

As discussed yesterday, the quick and dirty solution would be bug 1541865. In the long term, we can think of making mozlint more generic.