Closed Bug 1366401 Opened 2 years ago Closed 9 months ago

Unified frontend for submitting code reviews

Categories

(Developer Services :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1471678

People

(Reporter: gps, Unassigned)

References

Details

Attachments

(1 file)

Right now, we have multiple command line tools for submitting code reviews to Firefox. The exact command depends on:

* Whether you are using Mercurial or Git
* Whether you are using MozReview or Splinter

There is redundancy in those commands and we often lack feature parity. It would be *much* nicer if there were a single frontend that performed common operations (such as collecting changesets/commits and other metadata) and dispatched to an appropriate backend for actually doing the submitting. That way, we implement something once and use it everywhere. There is justification for creating a unified review submission frontend *today*.

In the future, we'll be submitting to Phabricator. That does kind of blow up all the existing assumptions I just typed. But, it brings with it new problems, such as Arcanist (Phabricator's CLI).

I'm not keen on exposing Arcanist (`arc`) to end users for various reasons:

* `arc` is essentially `mach` and I would prefer to not introduce another interface to be learned by developers, especially since we'll only use a limited subset of Phabricator.
* Similarly, I'm worried about people wanting `arc unit` or `arc lint` to work. I don't want to implement these things on the Firefox repo because they are redundant with `mach`. Even if we proxy `arc` commands via `mach`, there will inevitably be things that don't work "just right" or require effort to maintain. I don't want "Arcanist support" to be a time sink for build system maintainers.
* Arcanist introduces a new run-time dependency on PHP, which means something else we need to install and manage on machines. This includes checking if it is out of date and updating it, if needed.
* The terminology for `arc` commands conflicts with terminology used by both Git and Mercurial and not in a good way. e.g. `arc diff` submits a review. Huh? This creates cognitive dissonance and confusion.
* My understanding is the default behavior of `arc diff` with regards to submitting multiple commits is not compatible with the common workflow at Mozilla (it squashes commits).
* I've been told to expect scaling pain with `arc` and large repositories.

Even if we expose `arc` to end users or use it behind the scenes for submitting reviews to Phabricator, there is still justification for having an abstraction layer in front of it.

For example, vendored projects like WPT, Servo, Rust creates, etc are developed outside of mozilla-central. As such, they have their own review workflow. I think it would be really rad if at review submission time we were like "oh, you are touching files that belong to an external project: I'll just submit a GitHub pull request for you." We /could/ do this server-side on Phabricator. But I'm guessing mcote doesn't want to do that :)

Another example is multiple commits. Phabricator doesn't handle multiple commits on the same review. Instead, it essentially creates a squashed diff then uploads metadata about the commits it came from. This completely undermines the widely practiced "micro commits" and "commits series" model at Mozilla. The workaround is to iterate through every commit and create separate review for each. e.g. https://medium.com/@kurtisnusbaum/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6. This is horrible UX and there will be anarchy at Mozilla by those who practice microcommit workflows. Fortunately, the solution of scripting `arc diff` is trivial. Another point in favor of abstracting `arc` away from the end user.

Here's what I'd like to do.

1) Add a Python function somewhere in version-control-tools that can be called from a mach command that will submit code review to Phabricator (or MozReview and Splinter if we implement this before their demise).
2) Add a `mach` command in mozilla-central that ensures version-control-tools is up to date then loads that Python module and calls that function

The reason I want the code doing the heavy lifting in version-control-tools is because:

* We already have infrastructure for testing Mercurial and Git
* We already have code for doing lots of VCS things
* We will want to use this code outside of mozilla-central anyway (e.g. for version-control-tools)
* We can keep it up to date from *any* Firefox repo checkout relatively easily. If the code lives in the Firefox repo, you have problems where if you are submitting a patch from an ESR checkout you may be running buggy code because it is months out of date. Or you have to backport tons of commits. It is a giant hassle.

FWIW, I would still like to merge v-c-t into mozilla-central (once mozilla-central's access policy is loosened). And the "ensure latest version available" problem is tractable once you can do partial clones and/or checkouts, which Mercurial and Git both have different problems supporting today. So VCT is the logical choice.
The Mercurial project is standing up a Phabricator instance. There is work in Mercurial to create a custom CLI that handles a series of commits.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-July/100778.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-July/100780.html
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-July/100779.html
https://phabricator.services.mozilla.com/D345 has an initial sketch of how to get bug and reviewer information into phabricator using the extension from https://www.mercurial-scm.org/wiki/Phabricator
If you are interested in working with tomprince's PoC, here is the rough set of steps you need to follow:

1. Download https://www.mercurial-scm.org/repo/hg/raw-file/4.5/contrib/phabricator.py
2. Download https://d3kxowhw4s8amj.cloudfront.net/file/data/w227zzsf3ts3zzkrxp3a/PHID-FILE-hjtijxtfkqpmgpum57ho/hgext_mozphab_init_.py as mozphab.py
3. Go to https://phabricator.services.mozilla.com/settings/user/gps/page/apitokens/ to generate an API token
4. Update your ~/.hgrc so it has the following content:

  [extensions]
  phabricator = /path/to/phabricator.py
  mozphab = /path/to/mozphab.py

  [phabricator]
  url = https://phabricator.services.mozilla.com/
  callsign = MOZILLACENTRAL
  token = <API token that you generated>

After this is done, you should be able to `hg phabsend .`, `hg phabsend ebb683::`, etc to submit changesets to Phabricator for code review. It will automatically extract any bug number and reviewer annotation from the commit message and set the appropriate fields in Phabricator. Run `hg help -e phabricator` and `hg help phabsend` etc to see what options are available.

There are a few rough edges. For example, it is difficult to point the client extension at multiple Phabricator instances. And having to configure the repository callsign via hgrc for different repos is a bit annoying. But these will be addressed over time.
Another issue came up in https://mail.mozilla.org/pipermail/firefox-dev/2018-June/006553.html. With MozReview, we used a proper VCS repository and linting tasks could query it to see the chain of changesets leading up to a specific patch. Our Phabricator isn't using this model (you upload a diff + metadata to Phabricator) and downstream tools have to reconstruct the changeset to `hg import` it.

Linting tasks need a way to identify the "relevant" parent changeset for a submission. Without a repository backing reviews, the *only* way to capture the "relevant parent changeset" is for the client to capture this metadata at submission time and post it to Phabricator. Arcanist could presumably do this. But we'd need its definition of "relevant" to agree with Mozilla's. So if this were implemented upstream, we'd probably need it to be configurable (e.g. a Mercurial revset).
For `arc` submitted revisions, that information does appear to be available (at least for revisions based directly off of a commit in m-c):

curl https://phabricator.services.mozilla.com/api/differential.querydiffs -d api.token=<token> -d 'revisionIDs[0]=1582' | jq '[.result[]][0].sourceControlBaseRevision'
(In reply to Tom Prince [:tomprince] from comment #6)
> For `arc` submitted revisions, that information does appear to be available
> (at least for revisions based directly off of a commit in m-c):
> 
> curl https://phabricator.services.mozilla.com/api/differential.querydiffs -d
> api.token=<token> -d 'revisionIDs[0]=1582' | jq
> '[.result[]][0].sourceControlBaseRevision'

If I'm reading the Arcanist source code properly, it is only doing this for the parent revision of the individual changeset/commit. We specifically need something that skips over non-public ancestors so we know of *all* changesets relevant to a given revision. Although even if we posted this metadata to Phabricator, there's no guarantee a tool would have access to all the intermediate changesets [like it would with a repo-based review tool].

We may want to take this to a separate bug report, since it feels out of scope given the bug summary.
As a temporary workaround, we are going to retrieve the parents iteratively until we find one which is in mozilla-central.
See Also: → 1489509
:tomprince :gps, and :glob. is this wontfix-able/dupe-able?
Flags: needinfo?(mozilla)
Flags: needinfo?(gps)
Flags: needinfo?(glob)
Yeah, moz-phab is the officially supported client, so I'll dupe this over to the initial-release bug.
Status: NEW → RESOLVED
Closed: 9 months ago
Flags: needinfo?(mozilla)
Flags: needinfo?(gps)
Flags: needinfo?(glob)
Resolution: --- → DUPLICATE
Duplicate of bug: 1471678
You need to log in before you can comment on or make changes to this bug.