Guard against "pip install"-ing package conflicting with system/vendored packages
Categories
(Firefox Build System :: Mach Core, enhancement)
Tracking
(firefox97 fixed)
Tracking | Status | |
---|---|---|
firefox97 | --- | fixed |
People
(Reporter: mhentges, Assigned: mhentges)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
As of work like improving venv consistency, pip
has better visibility of vendored and inherited packages. This work is also allowing pip
to avoid installing packages that will already be in-scope.
However, there's a flaw that I detected last night: if a package to pip install
has dependencies that conflict with vendored/inherited packages, pip
will try to uninstall the existing incompatible package, fail (because it's outside of its venv), then install the package it wants (which will be deprioritized compared to the existing one).
Say, for example, that we have attrs==19.1.0
as a vendored package.
Then, we pip install pytest==6.2.5
:
$ pip install pytest==6.2.5
...
Installing collected packages: toml, py, pluggy, iniconfig, attrs, pytest
Attempting uninstall: attrs
Found existing installation: attrs 19.1.0
Not uninstalling attrs at /home/mitch/dev/firefox/third_party/python/attrs, outside environment /home/mitch/dev/firefox/obj-x86_64-pc-linux-gnu/_virtualenvs/python-test
Can't uninstall 'attrs'. No files were found to uninstall.
Successfully installed attrs-21.2.0 iniconfig-1.1.1 pluggy-1.0.0 py-1.10.0 pytest-6.2.5 toml-0.10.2
<snip>
$ pip list -v
Package Version Location Installer
------------------- ----------- ---------------------------------------------------------------------------------------------------- ---------
<snip>
attrs 19.1.0 /home/mitch/dev/firefox/third_party/python/attrs
<snip>
pytest 6.2.5 /home/mitch/dev/firefox/obj-x86_64-pc-linux-gnu/_virtualenvs/python-test/lib/python3.6/site-packages pip
<snip>
Despite pip install
saying "Successfully installed attrs-21.2.0", it's still using attrs==19.1.0
. This is because we are (correctly) prioritizing vendored packages over pip-installed packages.
So, what's the solution here? Well, the main problem is that pytest==6.2.5
requires attrs>=19.2.0
. We know that allowing pip install
to ignore the existing attrs==19.1.0
is not the solution, because our replacement attrs
may have incompatibilities with other vendored packages. Instead, what should happen is the pip install
should fail because there's a conflict: "I can't install pytest==6.2.5
because that conflicts with the existing attrs==19.1.0
package".
pip install
isn't quite in a position where it can defend against conflicts in its environment - it can only detect them after-the-fact (more details here).
However, it allows a --constraints
option, which we can use to make pip
do more validation up-front.
Detailed solution:
(this is in my own "note" form here, so it's a little terse :)
- Non-system python
- Mach venv, reqd: constraint on vendored deps.
- Mach venv, optional: constraint on vendored deps, reqd installed pkgs.
- Command venv, reqd: constraint on Mach vendored deps, Mach "pip list", command vendored deps.
- Note: what if Mach vendored dep != command vendored dep? Constraint file itself will conflict, will error be obvious enough to diagnose? Look into this as part of this solution.
- Command venv, optional: constraint on Mach vendored deps, Mach "pip list", command vendored deps, command reqd installed pkgs.
- System python
- mach venv: "pip check" system (prioritize vendored pkgs over system), check that reqd pkgs are installed, check that optional pkgs don't have unexpected version.
- Command venv, reqd: constraint on Mach vendored deps, system "pip list", command vendored deps.
- Commadn venv, optional: constraint on Mach vendored deps, system "pip list", command vendored deps, command reqd installed pkgs.
- CI work
- For each venv, check that it's possible for all required + optional dependencies to be installed in a clean venv without conflicts.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Update:
For each venv, check that it's possible for all required + optional dependencies to be installed in a clean venv without conflicts.
This is already done by the upcoming test_virtualenv_compatibility.py
test, which verifies that all required + optional requirements can be installed at the same time.
constraint on ...
I've found a more succinct solution here: though simply add all existing pip list
packages to a constraints file before running pip install
.
Notes:
- This resolves the "system python + pip-install-able command venv" inconsistency tradeoff described in this comment. At
pip install
-time, the system python packages are part of the generated constraints file, so conflicts will be detected up-front. - Tracking the state of "vendored deps, Mach vendored deps, system python/mach venv site-packages" is already done in bug 1730712 in a pip-aware way.
pip list
leverages this without us having to duplicate the logic. - Using the constraints file slows things down, but it only happens at
pip install
-time. Additionally, once we have lockfiles, we'll be able to blindlypip install
without--constraint
, which will improve performance.
Assignee | ||
Comment 2•3 years ago
|
||
Note: there's a few use-cases in-tree where both:
- We have a lockfile that's been generated that isn't influenced by vendored (or pypi-installed) packages
- We're using said lockfile in an active Mach process, so the lockfile isn't fully being honoured (already-imported packages are being used, even if they are different from the locked versions).
This will require some shaking-down of the tree to conform :)
Assignee | ||
Comment 3•3 years ago
|
||
Pip is able to detect unpacked sdists because they have a .egg-info
directory, not because they have a top-level PKG-INFO
file.
This is confirmed by the MarkupSafe
case, where pip can see the
package in third_party/python/MarkupSafe/src
, even though there's no
PKG-INFO
at that depth.
Depends on D126288
Assignee | ||
Comment 4•3 years ago
|
||
Use the --constraint
feature to have pip
consider already-installed
packages up-front before installing new ones.
Note that, in cases where a --requirement
is used (and is potentially
a lockfile), we allow it to continue quietly failing. This is because
such lockfiles aren't prepared with knowledge of Mach's vendored
packages, so they'll likely fail the conflict-check. However, since
they've worked so far, we should allow them to keep limping along until
orted to the centralized dependency system.
Depends on D126924
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
click
isn't used directly by us, but rather transitively through
glean-parser
and pip-tools
. They're both compatible with a newer
version, so we can use that.
This is needed because black==20.8b1
requires click>=7.1.2
.
Depends on D126924
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D127163
Assignee | ||
Comment 7•3 years ago
|
||
Now that are prioritizing system over virtualenv site-packages, the
system pip
is sometimes being used instead.
This is causing issues when the system pip is set up in a
distro-specific way, such as when "debundled":
https://github.com/pypa/pip/blob/9.0.1/pip/_vendor/__init__.py#L53-L61
However, if we vendor pip
, setuptools
and wheel
, and ensure that
they're prioritized in the sys.path
before anything is imported from
the system, then we can ensure that we're using a modern pip
and
sidestep system-specific pip weirdness.
Note that pip-compile
's --allow-unsafe
flag is not as dangerous as
it sounds.
There's confusion among maintainers about its origin:
https://github.com/jazzband/pip-tools/issues/522
Additionally, it's going to be enabled by default in a future
pip-tools
release. So, it's not scary for us to embrace here.
Assignee | ||
Comment 8•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 10•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Updated•3 years ago
|
Comment 11•3 years ago
|
||
Comment 12•3 years ago
|
||
Backed out for causing failures at test_vendor.py.
Backout link: https://hg.mozilla.org/integration/autoland/rev/6cb77e9e9cf5e82d1083f751dc5255a634359e9c
Failure log: https://treeherder.mozilla.org/logviewer?job_id=359061742&repo=autoland&lineNumber=1360
Comment 13•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 14•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Comment 15•3 years ago
|
||
Comment 16•3 years ago
|
||
bugherder |
Comment 17•3 years ago
|
||
Since https://hg.mozilla.org/mozilla-central/rev/de9e103ffea4 landed I can no longer run mach
:
Traceback (most recent call last):
File "/Users/jdescottes/Development/hg/fx-team-artifact/./mach", line 126, in <module>
main(sys.argv[1:])
File "/Users/jdescottes/Development/hg/fx-team-artifact/./mach", line 118, in main
mach = check_and_get_mach(os.path.dirname(os.path.realpath(__file__)))
File "/Users/jdescottes/Development/hg/fx-team-artifact/./mach", line 105, in check_and_get_mach
return load_mach(dir_path, mach_path)
File "/Users/jdescottes/Development/hg/fx-team-artifact/./mach", line 91, in load_mach
return mach_initialize.initialize(dir_path)
File "/Users/jdescottes/Development/hg/fx-team-artifact/build/mach_initialize.py", line 219, in initialize
_activate_python_environment(
File "/Users/jdescottes/Development/hg/fx-team-artifact/build/mach_initialize.py", line 172, in _activate_python_environment
mach_environment = MachSiteManager.from_environment(
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/site.py", line 235, in from_environment
requirements = resolve_requirements(topsrcdir, "mach")
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/site.py", line 840, in resolve_requirements
return MachEnvRequirements.from_requirements_definition(
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/requirements.py", line 80, in from_requirements_definition
_parse_mach_env_requirements(
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/requirements.py", line 182, in _parse_mach_env_requirements
_parse_requirements_definition_file(root_requirements_path, False)
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/requirements.py", line 178, in _parse_requirements_definition_file
_parse_requirements_line(
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/requirements.py", line 129, in _parse_requirements_line
_parse_requirements_definition_file(
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/requirements.py", line 178, in _parse_requirements_definition_file
_parse_requirements_line(
File "/Users/jdescottes/Development/hg/fx-team-artifact/python/mach/mach/requirements.py", line 119, in _parse_requirements_line
raise Exception(
Exception: The "pth:" pointing to "/Users/jdescottes/Development/hg/fx-team-artifact/python/mozterm" has a ".egg-info" file.
Perhaps "/Users/jdescottes/Development/hg/fx-team-artifact/build/common_virtualenv_packages.txt:14" should change to start with "vendored:" instead of "pth:".
Is there something to fix on my end?
Comment 18•3 years ago
|
||
This happened to me too. And obviously, running mach clobber python
doesn't work either. The egg-info directories are not in VCS, but they were created, in my case, 11 days ago. However that happened, we should handle the situation more gracefully. For now, I'll have this last changeset backed out.
Comment 19•3 years ago
|
||
Backed out for causing mach issues
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 21•3 years ago
|
||
Comment 22•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Comment 23•3 years ago
|
||
Comment 24•3 years ago
|
||
bugherder |
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Description
•