Open Bug 1936158 Opened 2 months ago Updated 2 months ago

Install linux_native-messaging-portal_requirements.txt only when needed

Categories

(Testing :: XPCShell Harness, task)

Product:
Testing
Component:
XPCShell Harness
Type:
task

Tracking

(Not tracked)

People

(Reporter: robwu, Unassigned)

References

Details

To support a xpcshell test, bug 1661935 introduced new Python package dependencies. The dependency is hardcoded and installed from the top-level mach_commands.py file. Differences between mach test and mach xpcshell-test have already revealed one regression (bug 1936114).

After taking a closer look, I think that the implementation should be changed: instead of installing from there, it should be after the test manifests have all been evaluated, and skip-ifs and run-ifs have been processed. Maybe around here: https://searchfox.org/mozilla-central/rev/e6733c6143dc6a1bd32c9c65d685528deb1ba895/testing/xpcshell/runxpcshelltests.py#1891-1909

To support this, we could consider a new per-test manifest key, e.g. by modifying https://searchfox.org/mozilla-central/rev/e6733c6143dc6a1bd32c9c65d685528deb1ba895/toolkit/components/extensions/test/xpcshell/native_messaging.toml#19-21 to:

["test_ext_native_messaging_portal.js"]
run-if = ["os == 'linux' && toolkit == 'gtk' && dbus_enabled"]
tags = "portal"
python_requirements = "linux_native-messaging-portal_requirements.txt"

Not sure if this completely makes sense, but it sounds conceptually appealing.

To be honest, unless per-test dependencies is going to become a common requirement, I think I'd prefer to just install the requirements globally for every mach xpcshell-test and mach test invocation. But instead of handling this in the xpcshell-test code, we would define the requirements in a mach virtualenv (defined under python/sites). This way the requirement will be installed once and cached until it changes.

I do get the desire to avoid unnecessarily installing packages.. but in this case I feel like keeping things simple is going to be better long term. For context, there's no one working on test harnesses so a large-ish feature like this is unlikely to get picked up.

The biggest hurdle to the proposal in comment 0 is going to be compatibility and/or isolation. Either we'd need to install these requirements in isolated virtualenvs, which would require a refactor of how the xpcshell harness launches tests. Or we'd need to have logic that ensures all these requirements files are compatible with one another in the event we need multiple of them. But this would be re-doing a lot of the work that mach already does for us with its virtualenv handling, and would add a decent amount of complexity to an unmaintained code base.

The original version of the NativeMessaging-for-snap patches did introduce a new python/sites/xpcshell-test.toml, but a later revision removed that in favor of installing manually from run_xpcshell_test. The following interdiffs show:

:gerard-majax - could you offer some context on why you moved away from python/sites/xpcshell-test.txt to installing from run_xpcshell_test?

The current implementation is imperfect, and between installing too often vs only when needed, the current approach is to NOT install when in doubt (with the fix to bug 1936114).

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

The biggest hurdle to the proposal in comment 0 is going to be compatibility and/or isolation. Either we'd need to install these requirements in isolated virtualenvs, which would require a refactor of how the xpcshell harness launches tests. Or we'd need to have logic that ensures all these requirements files are compatible with one another in the event we need multiple of them.

I was thinking of invoking the management of virtualenv near https://searchfox.org/mozilla-central/rev/e6733c6143dc6a1bd32c9c65d685528deb1ba895/testing/xpcshell/runxpcshelltests.py#1891-1909, so that it only runs when needed. The only reason for putting the logic there (or in the build system at all) is because the logic to manage dependencies in a virtualenv already exists there.

One concern I have with the current approach, of it being disconnected from python/sites/ is that it may be disconnected from existing tooling. E.g. if there is automation that scans for security vulnerabilities in dependencies, how does it know how to find the odd requirements.txt file? I don't know if there is any existing tooling that depends on the "standard" structure.

Another approach, if considered acceptable, is to install the dependencies from the test itself, i.e. as part of add_setup() in https://searchfox.org/mozilla-central/rev/5b93d2870ab36ba9cfa144fa7518d54e5439caa9/toolkit/components/extensions/test/xpcshell/test_ext_native_messaging_portal.js#91

The test does not really care from where it is run, and there is a lot of flexibility in the choice of location for the Python program and dependencies.

Flags: needinfo?(lissyx+mozillians)

(In reply to Rob Wu [:robwu] from comment #3)

The original version of the NativeMessaging-for-snap patches did introduce a new python/sites/xpcshell-test.toml, but a later revision removed that in favor of installing manually from run_xpcshell_test. The following interdiffs show:

:gerard-majax - could you offer some context on why you moved away from python/sites/xpcshell-test.txt to installing from run_xpcshell_test?

it was buggy and installing the wheels always, whenever running any test. which is painful especially given the deps it requires (and building)

Flags: needinfo?(lissyx+mozillians)

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

To be honest, unless per-test dependencies is going to become a common requirement, I think I'd prefer to just install the requirements globally for every mach xpcshell-test and mach test invocation.

The build dependencies for the package are non-trivial (comment 5) and the packages themselves are only relevant for one unit test on Linux. For that reason it is preferable to not force these package requirements on everyone else.

The current implementation in the tree that landed a few days ago installs the dependencies in the "default" virtualenv when the one test is among the matched tests (source of run_xpcshell_test, does not include the fix from bug 1936114 yet). Is this an acceptable final state of the implementation? If yes, then we can close this bug.

But instead of handling this in the xpcshell-test code, we would define the requirements in a mach virtualenv (defined under python/sites). This way the requirement will be installed once and cached until it changes.

"Installing once and cached" is a desired characteristic. Is that only available to requirements listed in python/sites/?
The current run_xpcshell_test implementation calls command_context.virtualenv_manager.install_pip_requirements(...), and it looks like the virtualenv is stored between executions (source of virtualenv_manager property). Since this is already in the tree, I assume that it would be okay to continue this practice, but then called when we have more context on which test to run (i.e. deeper in the runxpcshelltests.py implementation), instead of the current top-level run_xpcshell_test command handler.

The only requirement is for the packages to be installed. The build system does not have to do more than that. If needed, we can add custom logic in the test file itself (test_ext_native_messaging_portal) to set up the environment variables and paths to locate the dependencies.

Another step further, if acceptable, could be to drop the dependency on support from the build system, and set up the virtual env from the test itself. E.g. with a new mach command (./mach setup-dbus-test) called from the test file, reusing all existing mach virtualenv_name logic without affecting ./mach xpcshell-test or ./mach test). Or even independently of mach by basically calling virtualenv and pip install as needed to ensure the availability of the dbusmock program in the test.

In short, I see the following approaches here:

  1. Do nothing - keep status quo of run_xpcshell_test installing the dependencies on Linux.
  2. Move the logic deeper in runxpcshelltests.py, so that we only invoke the logic when really needed.
  3. Move the dependency installation to the test_ext_native_messaging_portal test itself.

:ahal - out of these options, which has your preference? Feel free to suggest other approaches, since this is not my area of expertise.

Flags: needinfo?(ahal)

Ah ok, yeah I remember now.

Another thing to consider here is that the test harness gets invoked differently when it's run locally (uses mach) vs CI (uses mozharness, no srcdir). So if we push the logic deeper into the test harness / test we would need to come up with a solution that doesn't rely on mach's virtualenvs at all. So I'm inclined to just keep the status quo.

If someone is motivated enough to work on 2 or 3, I wouldn't be opposed. But it would be good to have a rough plan drafted before starting.

Flags: needinfo?(ahal)

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

Another thing to consider here is that the test harness gets invoked differently when it's run locally (uses mach) vs CI (uses mozharness, no srcdir).

Thanks for this extra bit of context. I'm not too familiar with how CI sets everything up, but with your mention of mozharness I guess that the responsibility of setting up the dependencies in CI is linux_dbus-python.py, loaded by https://searchfox.org/mozilla-central/rev/d6ba5401121104ae242ca18efa6a5672af9cae0f/taskcluster/kinds/test/xpcshell.yml#29
Given that, we only need to support non-CI cases here, right? E.g. skipping the logic when MOZ_AUTOMATION is set.

So if we push the logic deeper into the test harness / test we would need to come up with a solution that doesn't rely on mach's virtualenvs at all.

If my understanding is correct, then CI does not depend on mach to set up dependencies, and it would be fine to rely on mach's virtualenv after all. Is that correct?

Technically yes. But I like how we currently leave the setup of the test environment outside of the test harness. E.g, either it's invoked by mach or invoked by mozharness and the test harness doesn't really need to care.

If we start making the test harness responsible for its own setup, then I'd like to avoid logic that special cases based on how the harness was invoked. I'd like it to just check whether dbus-python is installed, and if not, install it regardless of whether it was mach or mozharness that called it. Maybe it's enough if mach ensures its venv is activated, and then the test harness just installs the package into whatever site is active? In other words, I don't want to pass in virtualenv_manager objects down into the test harness.

This is mostly just an attempt at keeping a last vestige of sanity in the test harnesses ><

By "test harness", are you referring to runxpcshelltests.py? I assume yes, so I read your comment as the desire to minimize the amount of unnecessary complexity in there.

To make sure that we have a common understanding - are we converging towards option 3 from comment 6?

And then concretely:

  • Let test_ext_native_messaging_portal.js detect the availability of the necessary dependency.
  • Since we know that the dependencies should be present in automation (comment 8), fail fast if the dependency is missing if MOZ_AUTOMATION is set. (This can be dropped if we need to, but I am suggesting this because CI is supposed to already have set up all dependencies)
  • If not present, set up the virtual environment and install the dependencies. To keep the test harness completely out of it, I would choose the directory name in objdir/temp or simply /tmp/.
You need to log in before you can comment on or make changes to this bug.