Open Bug 1697357 Opened 4 years ago Updated 4 years ago

`mach install-moz-phab` should be more consistent across platforms

Categories

(Firefox Build System :: Bootstrap Configuration, task)

Product:
Firefox Build System
Component:
Bootstrap Configuration
Type:
task

Tracking

(Not tracked)

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

In some cases, it uses --user, in others, it doesn't (and some of those other cases make assumptions that are not necessarily true, e.g. python3 coming from homebrew on macOS). I think we should be more consistent across platforms, and use --user everywhere.
OTOH, because of all the dependencies required that could very well conflict with what people might want to, themselves, pip3 install --user, arguably, it would be better to setup a virtualenv.
My overall suggestion would be to create a virtualenv under ~/.mozbuild/_virtualenvs, install there, create a ~/.mozbuild/bin directory, and create a moz-phab wrapper/link there, and make the user add ~/.mozbuild/bin to their $PATH.

Thoughts?

Flags: needinfo?(sheehan)
Flags: needinfo?(mhentges)
Flags: needinfo?(glob)

I'm for consistency as well as using a dedicated virtualenv for moz-phab for mach install-moz-phab.

My primary concern is the need for users to manually update $PATH, particularly on Windows where that is less fun. That's the core reason for the logic we currently have. It's a shame that path_helper on macOS only looks in /etc/path and /etc/paths.d; it would be sweet if it also checked ~/.paths.d.

That all said I don't think the need to manually fix the path outweighs the benefits we'd get from this, so f=glob.

As an aside, does mach have the ability to detect broken virtualenvs and repair; eg. following a python upgrade?

Flags: needinfo?(glob)
Summary: mach install-moz-phab should be more consistent across platforms → `mach install-moz-phab` should be more consistent across platforms

ISTR there are cases that are handled, but there are others that are very difficult to handle that aren't.

As for the $PATH issue, we could get around it if we moved to having moz-phab being a mach command, which I know you have objections about, but I think we can make it work.

BTW, it's worth noting that the path under which --user ends up installing things contains the python version, and after a python upgrade, I'm not sure what happens.

BTW, it's worth noting that the path under which --user ends up installing things contains the python version, and after a python upgrade, I'm not sure what happens.

Blocks: 1696094

(In reply to Mike Hommey [:glandium] from comment #2)

As for the $PATH issue, we could get around it if we moved to having moz-phab being a mach command, which I know you have objections about, but I think we can make it work.

I don't have objections to it, in fact it's my preferred option.

Specifically what I want is:

  • a command such as mach review which invokes moz-phab in some way (easy)
  • updating moz-phab should not also require updating mozilla-central in any way (easy)
  • mach review --help should show moz-phab's help; I could only make this work with mach review -- --help which is horrible (hard)

I played around a lot with integrating moz-phab's help into mach's command decorators, which IIRC required generating classes and functions at runtime to satisfy the decorators. It was pretty ugly and after discussions with Chris we settled on the install command (see https://phabricator.services.mozilla.com/D59139).

after a python upgrade, I'm not sure what happens.

Pretty sure it depends on how the venv was created (probably virtualenv vs. python3 -m venv).

I see virtualenvs on my macOS/brew system with symlinks to the exact python version in the Cellar, resulting in dyld: Library not loaded: @executable_path/../.Python errors following even a minor version bump.

(In reply to :glob 🎈 from comment #5)

after a python upgrade, I'm not sure what happens.

Pretty sure it depends on how the venv was created (probably virtualenv vs. python3 -m venv).

Note that this specific context was about the non-virtualenv case.

If all Mozilla developers were only submitting patches for mozilla-central, then I'd agree that ./mach review would be the best way to manage patch submission.

However, we have a repository diaspora (multiple hg repositories, some Github repos, etc). By having ./mach review, we've complicated the user interface: as a developer, when should you use moz-phab, and when should you use ./mach review?

Additionally, the concerns about causing issues in --user still apply if the developer isn't just working on mozilla-central - we've just kicked the bucket down the road.

I'm for consistency as well as using a dedicated virtualenv ...

I agree that having a dedicated moz-phab virtualenv will alleviate concerns of mutating an existing environment (such as --user), and is (IMHO) a necessary feature of whatever solution we adopt.

We've already taken a couple steps down the path of acting as if moz-phab is a standalone tool for the ecosystem, since we've packaged it and are distributing it via pip. I think that we should further embrace the ecosystem and recommend the use of the pipx.

pipx's killer feature is that it creates a separate virtualenv for each CLI that you install, but exposes them all as single commands that you can invoke. For example, I have hg, moz-phab and docker-compose installed, and they're all in their own independent virtualenvs. However, I just run $ hg ... in my shell, and the correct virtualenv is activated and the command is run within it.

My only concern with pipx is that it is relatively new software, and that the "ecosystem favourite" could shift (pipsi was the previous incumbent).
However, it does it's job really, really well. Last month I moved my main user-wide packages to be installed with pipx, and it's worked incredibly smoothly. Additionally, I've had the helpful side-effect of hg being significantly faster, which I'm chalking up to less unrelated packages being in the PYTHONPATH.

Flags: needinfo?(mhentges)

For what it's worth, another option for avoiding pip environment issues is to bundle all of our dependencies into a single "fat binary".
Unfortunately, since dependencies are python-version-specific, this would require that we bundle Python as well.

This allows us to ship a single self-updating binary for each platform, and all we ask of users is that it's placed in a directory on the PATH (./mach install-moz-phab could do this intelligently). We gain enormous control over distribution of moz-phab.
The downside is that we increase download size and the work that happens in moz-phab CI.

pipx is certainly interesting (and technically, it's roughly the same thing I was suggesting, except it uses ~/.local/bin instead of ~/.mozbuild/bin). One big caveat, though, is that it relies on python3's venv, and for some reason, the one in MozillaBuild is busted.

Flags: needinfo?(sheehan)

pipx ... [is] roughly the same thing I was suggesting, except it uses ~/.local/bin instead of ~/.mozbuild/bin.

Exactly. The benefit that it has over the ~/.mozbuild/bin suggestion is that pipx is able to create each virtual environment and entrypoint script, even if the developer isn't working in-tree.

One big caveat, though, is that it relies on python3's venv, and for some reason, the one in MozillaBuild is busted.

Ouch.

Chatted with glob a little bit more. I think we're between a rock and a hard place. To summarize all the features we'd prefer to have:

  • The user shouldn't need to sudo.
  • moz-phab should have its own virtualenv so its dependencies don't get stomped by pip.
  • The user shouldn't need to manually manage their $PATH.
  • There should be a single way of invoking moz-phab, regardless of which project is being worked on.
  • The solution should work on all supported platforms.
  • The solution shouldn't require 3rd-party software (e.g.: pipx).

Of course, there doesn't appear to be a solution that resolves all of these problems.
Glob raised a good point that there aren't many users who need moz-phab that aren't working in-tree. So, perhaps we can concede on that "single way of invoking moz-phab" point. The solution we landed on today looked like this:

  1. When working out-of-tree, consult the moz-phab docs - which currently recommend pip install MozPhab.
  2. When working in-tree, let mach manage moz-phab (we install it into a custom virtualenv, expose it via a mach command), which allows us to check every other box in the above list.

Related: I'm not sold on ./mach review being the name of the command, though - the user running the command isn't doing the review, they're just submitting for review. Perhaps it should be ./mach publish or ./mach phabricator or ./mach moz-phab instead?

(In reply to Mitchell Hentges [:mhentges] 🦀 from comment #11)

  • The user shouldn't need to manually manage their $PATH.

I don't think this is an issue - in our discussion I pointed out that updating the PATH shouldn't be a complicated task for software engineers, especially if we point to documentation on the wiki.

Glob raised a good point that there aren't many users who need moz-phab that aren't working in-tree.

My point was more that there are very few people who are using moz-phab both for mozilla-central and other Phabricator hosted projects. This is likely to be a smaller number than those who are working outside of m-c completely.

So, perhaps we can concede on that "single way of invoking moz-phab" point.

100%. Having a single way to invoke moz-phab across projects isn't important and isn't a feature that we need.

The solution we landed on today looked like this:

  1. When working out-of-tree, consult the moz-phab docs - which currently recommend pip install MozPhab.

Nit: on Linux we recommend pip install --user MozPhab. Non-user installs are recommended on other platforms to avoid path editing.

  1. When working in-tree, let mach manage moz-phab (we install it into a custom virtualenv, expose it via a mach command), which allows us to check every other box in the above list.

FWIW a mach managed virtualenv would still require users to edit their PATH, but in the environments m-c cares about that isn't painful (mozillabuild on Windows saves us from Windows environment variable management).

Related: I'm not sold on ./mach review being the name of the command, though - the user running the command isn't doing the review, they're just submitting for review. Perhaps it should be ./mach publish or ./mach phabricator or ./mach moz-phab instead?

If we're to switch off mach review (which is still my preferred name), mach moz-phab is the only one in your list that I like.

publish is a generic term that isn't constrained to "submitting a patch for review", and moz-phab does more than just submitting a patch for review.

phabricator wouldn't work due as there are moz-phab commands that don't touch Phabricator at all; and it feels less descriptive than review.

FWIW a mach managed virtualenv would still require users to edit their PATH, but in the environments m-c cares about that isn't painful (mozillabuild on Windows saves us from Windows environment variable management).

Can you clarify? If we only exposed mach's moz-phab via a ./mach <moz-phab> command, then the user doesn't need to modify their $PATH.

Related, but these two points seem to conflict:

I don't think [$PATH management] is an issue - in our discussion I pointed out that updating the PATH shouldn't be a complicated task for software engineers, especially if we point to documentation on the wiki.

and

Nit: on Linux we recommend pip install --user MozPhab. Non-user installs are recommended on other platforms to avoid path editing.

Either way, we seem aligned on the direction forward.

(In reply to Mitchell Hentges [:mhentges] 🦀 from comment #13)

FWIW a mach managed virtualenv would still require users to edit their PATH, but in the environments m-c cares about that isn't painful (mozillabuild on Windows saves us from Windows environment variable management).

Can you clarify? If we only exposed mach's moz-phab via a ./mach <moz-phab> command, then the user doesn't need to modify their $PATH.

While ./mach review is my preferred option (see comment 5), I don't think it's realistically achievable with how mach's command decorators operate.

As such it sounds like the plan outlined in comment 0 of making the user add ~/.mozbuild/bin to their $PATH is the way forward.
Utilising a ~/.mozbuild/bin directory opens they way for other non-mach commands to be managed by mach's bootstrap.

Related, but these two points seem to conflict:

I don't think [$PATH management] is an issue - in our discussion I pointed out that updating the PATH shouldn't be a complicated task for software engineers, especially if we point to documentation on the wiki.

and

Nit: on Linux we recommend pip install --user MozPhab. Non-user installs are recommended on other platforms to avoid path editing.

This is mostly about how hard Windows makes it to extend your path.

(In reply to :glob 🎈 from comment #14)

While ./mach review is my preferred option (see comment 5), I don't think it's realistically achievable with how mach's command decorators operate.

That's fixable (and desirable because it affects mach configure too)

See Also: → 1704895

After reading this again, just wanted to dump my 👍 for either solution, though I'd lightly prefer to have ./mach review/./mach moz-phab rather than exposing a mach-managed virtualenv to the $PATH.

You need to log in before you can comment on or make changes to this bug.