Closed Bug 1263201 Opened 6 years ago Closed 6 years ago

Run checkstyle before push in reviewboard extension

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The turn-around process is currently: push your change to fx-team, someone notices the failing checkstyle task, files a bug, someone gets assigned, someone fixes it, pushes to review, gets reviewed, gets pushed, cycle repeats.

Awful. Let's run it at a better time. Some options:
 1) fail the build
 2) on push to reviewboard (should fail?)
 3) on push to fx-team (should fail?)
 4) At commit time (blocks commit?)
 5) As part of reviewboard [1]

I like 2 and 3 or just 2 – they strike the balance between getting things done (e.g. if we checked on every build, you can't write stylishly-hacky code just to test something out) and catching these errors. 2) also prevents the user from wasting their reviewers' time w/ nits. Doing all of these style fixes at once is also better for context switching capabilities (as opposed to 4).

5 sounds great, but it's not really on us and it may not be worth waiting for.

fwiw, I once heard it's not-encouraged to add hg hooks for everyone but I'm not sure why that is. But then again, we already have a linter for python code on commit.

[1]: https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-March/001914.html
On my machine, Checkstyle takes:
  * 1.2 seconds for an iterative build
  * 4.17 seconds for a clean build (perhaps longer if the deps need to be retrieved)

My machine is quite fast and I wouldn't want to add another second to hg's already slow commits so I say we run this on push and, imo, let's block the push if checkstyle fails – it'll save reviewer time. It could be a pain in the ass when putting out fires... but I don't think that happens often enough so I'm okay with that. We can fix it later if it's a problem.

Chris, is there anyone on the build team who has the cycles to build this quick push-hook for hg? If you're not the person to ask, feel free to punt this to another team member – you're just my go-to on the build team. :P
Flags: needinfo?(cmanchester)
I don't think hg hooks fall exactly in build team territory (I wouldn't know where to go to implement one), but gps or smacleod (from the mozreview side) might be a good point of contact.

A step in the right direction might be to make a checkstyle failure a Tier 1 failure (cause for backout if not fixed in place).
Flags: needinfo?(cmanchester)
Greg, comment 1 – would anybody you know be able to build a quick push-hook for me, or point me in the right direction?
Flags: needinfo?(gps)
A push hook on the server is out of the question: we don't want pushes to take longer than they already are because of an expensive process on the server.

Writing a local hook to run at commit time is a good idea, assuming it doesn't take forever to run. If it does, you are just going to annoy people whenever they commit (this includes amending commits). Also, you don't want to prevent the commit if linting fails, because that is just annoying.

The mozext extension contains a Python linter that runs at commit time. See https://dxr.mozilla.org/mozilla-central/source/tools/mercurial/eslintvalidate.py for an eslint extension that does something similar.
Flags: needinfo?(gps)
Sounds like I'll need to implement this myself.

I'd like to implement a client-side hook on push because I think it runs a little longer than I'd like for commit hooks.
Assignee: nobody → michael.l.comella
Things I learned:
  * You can see existing hooks with `hg showconfig hooks`
  * The reason you don't want to install commit hooks for people is because you're executing arbitrary code on other users' machines with their privileges. For this reason, hg doesn't let you ship hooks
  * ...but you can get around this by shipping a separate repository with hooks (i.e. version-control-tools)
  * The python critic hook is defined and set at [1]
  * But there are other hooks in the repository [2] – unclear which one I should mimic
  * I can easily guinea pig locally by adding `preoutgoing = ./gradlew app:checkstyle` under the [hooks] section of my .hgrc. It appears to be run from the root of the repository even if the command is not run from the root (yay!).

I think the script's behavior should be: if files in mobile/android/ are changed, and run `./gradlew app:checkstyle` from the root. Note that we don't run `./mach gradle app:checkstyle` because we can save a few tenths of a second by not waiting for mach's python interpreter to start-up. I'm not opposed to blocking push if the checkstyle fails but I think others could find this annoying so we should consider what to do there – the "outgoing" hook can be used for not blocking the push while "preoutgoing" will block the push.

[1]: https://github.com/mozilla/version-control-tools/search?utf8=%E2%9C%93&q=critic_hook
[2]: https://github.com/mozilla/version-control-tools/tree/master/hghooks
Attached patch shell script hook (obsolete) — Splinter Review
It seems you might be able to right "external hooks" in shell so I did that and will guinea pig it in my local .hg/hgrc.

I'm not sure how we might incorporate the shell script into the version-control-tools source though.
To add the hook directly to your .hg/hgrc, you can make it a one-liner:

preoutgoing = hg status --added --modified | grep -q mobile/android/; if [ $? -eq 0 ]; then ./gradlew app:checkstyle; fi
Note to self: add a useful "HELP ME!" message. If necessary, link to some online documentation.
Another note to self: test what happens if a dev isn't developing for Android, doesn't have gradle set up, etc.
It looks like the hooks from comment 6 [2] have to be manually added so we'll have to follow the pattern of critic_hook (comment 6 [1]), which sets itself up in the mozext extension. It calls out to [3]:

  ui.setconfig('hooks', 'commit.critic', critic_hook)

where critic_hook is a python function. I think that means our options:
  * Put the shell script as a one-line string (ew)
  * Call out via subprocess to the shell script (also ew)
  * Create a python function which acts like the shell script does. Eventually, it will use subprocess to call out to `./gradlew app:checkstyle`

[3]: https://github.com/mozilla/version-control-tools/blob/5ab08819c7876701bd24d042278eba48ff464f6d/hgext/mozext/__init__.py#L1557
Attachment #8743574 - Attachment is obsolete: true
Comment on attachment 8744137 [details]
MozReview Request: reviewboardext: Add android checkstyle hook on push (bug 1263201); r=gps

Is this the right idea, Greg?

I plan to figure out the TODOs but if you have suggestions, I'm all ears!

fwiw, I know it'd be better to run checkstyle on the specific files that changed but unfortunately, I don't know how to do that at the moment (and I'd rather not spend time on it right now). Blocking checkstyle on review push should mostly prevent broken changes from getting into the tree for our small team.

Self note: some links to help dev:
 * https://selenic.com/repo/hg/file/tip/mercurial/context.py#l317
 * https://github.com/mozilla/version-control-tools/blob/master/hgext/mozext/__init__.py#L473
Attachment #8744137 - Flags: review?(gps) → feedback?(gps)
https://reviewboard.mozilla.org/r/48305/#review45725

::: hgext/reviewboard/client.py:400
(Diff revision 1)
>          ctx = repo[node]
>          if not ctx.files():
>              raise util.Abort(
>                      _('cannot review empty changeset %s') % ctx.hex()[:12],
>                      hint=_('add files to or remove changeset'))
>  

I would insert the function call to do outgoing linting here. `nodes` is a list of what will be submitted for review. This runs pretty early - before we've sent anything to the server.

::: hgext/reviewboard/client.py:1073
(Diff revision 1)
> +        base_chg, head_chg = map(
> +                lambda revset: local[local.revs(revset).first()].node(),
> +                ['::. & draft()', '.'])

You shouldn't need to use a revset: consume the list of nodes I pointed out above.

::: hgext/reviewboard/client.py:1078
(Diff revision 1)
> +        changed = []
> +        for attr_type in ['modified', 'added']:
> +            changed.extend(getattr(pushed_status, attr_type))

You'll want to use a set here.

changed = set()
for attr_type in ('modified', 'added'):
     changed |= set(getattr(pushed_status, attr_type))

::: hgext/reviewboard/client.py:1081
(Diff revision 1)
> +        mobile_changed = filter(
> +                lambda f: f.startswith('mobile/android/'), changed)

This makes the assumption that we're operating on a Firefox repository. This Mercurial extension could be running on *any* repo. So this code is not appropriate for this extension :/

A proper solution for this is somewhat difficult. But I'd be OK with a hack. You can test if the repo is a Firefox repo by doing something like:

`if len(repo) and repo[0].hex() == '8ba995b74e18334ab3707f27e9eb8f4e37ba3d29'`

::: hgext/reviewboard/client.py:1082
(Diff revision 1)
> +
> +        changed = []
> +        for attr_type in ['modified', 'added']:
> +            changed.extend(getattr(pushed_status, attr_type))
> +        mobile_changed = filter(
> +                lambda f: f.startswith('mobile/android/'), changed)

List comprehensions are typically used for this in Python:

mobile_changes = [f for f in changed if f.startswith('mobile/android/')]

::: hgext/reviewboard/client.py:1084
(Diff revision 1)
> +        for attr_type in ['modified', 'added']:
> +            changed.extend(getattr(pushed_status, attr_type))
> +        mobile_changed = filter(
> +                lambda f: f.startswith('mobile/android/'), changed)
> +
> +        if len(mobile_changed) > 0:

if mobile_changed:

::: hgext/reviewboard/client.py:1085
(Diff revision 1)
> +            changed.extend(getattr(pushed_status, attr_type))
> +        mobile_changed = filter(
> +                lambda f: f.startswith('mobile/android/'), changed)
> +
> +        if len(mobile_changed) > 0:
> +            print 'Running checkstyle for mobile/android/ changes...'

local.ui.write('running checkstyle...\n')

::: hgext/reviewboard/client.py:1089
(Diff revision 1)
> +        if len(mobile_changed) > 0:
> +            print 'Running checkstyle for mobile/android/ changes...'
> +            # We run gradle directly (rather than the mach wrapper) to save
> +            # (granted, very little) time on the python interpreter start-up.
> +            import subprocess
> +            if subprocess.call(['./gradlew', 'app:checkstyle']) == 1:

This assumes you're at the root of the repository. You can use `repo.root` to access the path of the working directory of a repo.

Also, you can drop the `== 1` since all non-0 exit codes should result in error.

::: hgext/reviewboard/client.py:1090
(Diff revision 1)
> +            print 'Running checkstyle for mobile/android/ changes...'
> +            # We run gradle directly (rather than the mach wrapper) to save
> +            # (granted, very little) time on the python interpreter start-up.
> +            import subprocess
> +            if subprocess.call(['./gradlew', 'app:checkstyle']) == 1:
> +                raise util.Abort(_('Android style violation. Please fix it '

raise error.Abort(...)
Attachment #8744137 - Flags: feedback?(gps)
Comment on attachment 8744137 [details]
MozReview Request: reviewboardext: Add android checkstyle hook on push (bug 1263201); r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48305/diff/1-2/
Attachment #8744137 - Attachment description: MozReview Request: reviewboardext: (WIP) Add android checkstyle preoutgoing hook (bug 1263201); r=gps → MozReview Request: reviewboardext: Add android checkstyle hook on push (bug 1263201); r=gps
Attachment #8744137 - Flags: review?(gps)
Comment on attachment 8744137 [details]
MozReview Request: reviewboardext: Add android checkstyle hook on push (bug 1263201); r=gps

https://reviewboard.mozilla.org/r/48305/#review46865

This isn't perfect in terms of architecture, but it's good enough for a first implementation.

I'm slightly concerned about aborting the push if there are style violations. For example, people may wish to post things for review to get feedback before they put in the effort to fix style. So don't be surprised when someone files a bug to change this behavior.

In a month or so, we might have a better review submission workflow in place. At that time, there will be a better location to perform pre-submit checks and adding a prompt/override for style violations will be much simpler then.

::: hgext/reviewboard/client.py:1103
(Diff revision 2)
> +            # set up, etc. so we can't force all changes to be run through
> +            # checkstyle. For correctness, we could whitelist all the errors we
> +            # can ignore, but that would require us to chase a variety of
> +            # changing error messages. For simplicity, we only stop the push if
> +            # we detect a checkstyle failure.
> +            if 'Checkstyle rule violations were found' in e.output:

This may not work on all locales. But that's follow-up territory.
Attachment #8744137 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/48305/#review46865

> I'm slightly concerned about aborting the push if there are style violations. For example, people may wish to post things for review to get feedback before they put in the effort to fix style.

That's a good point. Thinking about this some more, since we don't block violations from landing, we could get into an issue where multiple devs pull the changes but can't push to reviewboard, end up fixing them locally, and then have to deal with the numerous merge conflicts, changeset stripping, etc. Yeah, this sounds like a bad idea – I'll just recommend they fix the errors before submitting before review instead.

> In a month or so, we might have a better review submission workflow in place. At that time, there will be a better location to perform pre-submit checks and adding a prompt/override for style violations will be much simpler then.

And then I can add an "Are you sure you want to push?" prompt when ^ happens.
FYI, I also added the `--quiet` flag to the gradle invocation, which should only print errors (making it easier to see what's going on when checkstyle fails).
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Run checkstyle at a more productive time than treeherder → Run checkstyle before push in reviewboard extension
Blocks: 1304887
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.