Closed Bug 1732946 Opened 3 years ago Closed 3 years ago

Guard against "pip install"-ing package conflicting with system/vendored packages

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement

Tracking

(firefox97 fixed)

RESOLVED FIXED
97 Branch
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.
Blocks: 1712131
Depends on: 1730712

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 blindly pip install without --constraint, which will improve performance.

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 :)

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

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

Assignee: nobody → mhentges
Status: NEW → ASSIGNED
Attachment #9243461 - Attachment description: Bug 1732946: Guard against venv conflicts after "pip install" → Bug 1732946: Guard against "pip install" venv conflicts

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

Depends on D127163

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.

Attachment #9243802 - Attachment description: Bug 1732946: [WILL-SQUASH] Vendor `click==8.0.1 → Bug 1732946: [WILL-SQUASH] Vendor `click==7.1.2`
Blocks: 1728646
Keywords: leave-open
Pushed by mhentges@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6057cadd8e54 Vendor pip/setuptools/wheel instead of installing into venv r=ahal
No longer blocks: 1728646
Regressions: 1739124
Attachment #9243800 - Attachment description: Bug 1732946: Update vendored "click" library → Bug 1732946: Update vendored libraries for `black` compatibility
Attachment #9243802 - Attachment description: Bug 1732946: [WILL-SQUASH] Vendor `click==7.1.2` → Bug 1732946: [WILL-SQUASH] Vendor `click`, `pathspec`, etc
Pushed by mhentges@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6a2e62e9cc99 Update vendored libraries for `black` compatibility r=ahal
Pushed by mhentges@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c05af873a20b Update vendored libraries for `black` compatibility r=ahal
Flags: needinfo?(mhentges)
Attachment #9243460 - Attachment description: Bug 1732946: Adjust sdist-vendored detection to be more aligned with pip → WIP: Bug 1732946: Adjust sdist-vendored detection to be more aligned with pip
Attachment #9243461 - Attachment description: Bug 1732946: Guard against "pip install" venv conflicts → WIP: Bug 1732946: Guard against "pip install" venv conflicts
Attachment #9243460 - Attachment description: WIP: Bug 1732946: Adjust sdist-vendored detection to be more aligned with pip → Bug 1732946: Adjust sdist-vendored detection to be more aligned with pip
Pushed by mhentges@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/de9e103ffea4 Adjust sdist-vendored detection to be more aligned with pip r=ahal

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?

Flags: needinfo?(mhentges)

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.

Flags: needinfo?(mhentges)
Attachment #9243460 - Attachment description: Bug 1732946: Adjust sdist-vendored detection to be more aligned with pip → WIP: Bug 1732946: Adjust "use 'vendored:'?" warn to be more aligned with pip
Attachment #9243460 - Attachment description: WIP: Bug 1732946: Adjust "use 'vendored:'?" warn to be more aligned with pip → Bug 1732946: Adjust "use 'vendored:'?" warn to be more aligned with pip
Pushed by mhentges@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/26d3952e5b49 Adjust "use 'vendored:'?" warn to be more aligned with pip r=ahal
Attachment #9243461 - Attachment description: WIP: Bug 1732946: Guard against "pip install" venv conflicts → Bug 1732946: Guard against "pip install" venv conflicts
No longer regressions: 1739124
Pushed by mhentges@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4529fe739a24 Guard against "pip install" venv conflicts r=ahal
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: