Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Init: pypa/installer #700

Merged
merged 7 commits into from
May 23, 2022
Merged

Init: pypa/installer #700

merged 7 commits into from
May 23, 2022

Conversation

groodt
Copy link
Collaborator

@groodt groodt commented May 14, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

The Wheel related parsing / unpacking / installation is using custom written code and the pkginfo package to parse metadata. This logic is complex and fragile because of the many Python Packaging PEPs it relates to.

Issue Number: #671

What is the new behavior?

This PR introduces a modern, PEP standards-compliant library called installer. This package is maintained in the Python Packaging Authority and is written by a few of the current maintainers of pip. It is therefore more likely to be processing Wheel files correctly.

This PR is only using installer for metadata parsing, but if this PR is accepted, I hope to replace all of the custom Wheel processing code.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

There are modern Python packaging libraries and PRs emerging that these Bazel rules should adopt where appropriate.

Modern packaging libraries:

installer from this PR, is able to correctly "install" (or rather unpack) Wheels into the desired location.
build is able to replace the elements of pip that produces Wheels. Suitable for converting an sdist into a Wheel.
hatch is a modern, extensible Python project manager that is a good example of how best to use the modern and emerging Python Packaging standards.


Promising PRs:

pip metadata-only resolve with pip download --dry-run --report when this PR lands and becomes standardised, the new internal resolver of pip can be used to produce an approximation of a lockfile. Note: this is not a formal specification of a lockfile (which will require a PEP), but is a promising step in that direction. This would eventually remove the need for pip-tools.


All of these things together means that there will eventually be the following options for consuming third-party dependencies with rules_python.

Option A - Install pre-built Wheels only in WORKSPACE (similar to rules_jvm_external where Bazel only needs to download and unpack)

  1. Produce Wheels either using build or pip wheel
  2. Store the Wheels somewhere (local filesystem, git LFS, private PyPI artifact repository
  3. Resolve urls for the dependencies against 2 using pip download --dry-run --report and create .bzl file
  4. Bazel repository rule to consume the .bzl and unpack using installer

Option B - Build and install in WORKSPACE (approach in this PR where Bazel does a combined build and unpack of third-party code)

  1. Bazel repository rule uses pip wheel and the fragile "build on demand" nature of pip to resolve and download existing Wheels OR convert sdist to Wheels
  2. Use installer to parse and unpack Wheels from 1.

Option C - Something like rules_pycross (which attempts to fuse python package builds into Bazel as closely as possible)
See: https://github.com/jvolkman/rules_pycross

Option D - Something like dbx_py_pypi_piplib (where you build cpython and libraries form pypi in Bazel)
See: https://github.com/dropbox/dbx_build_tools

@groodt groodt force-pushed the groodt-init-installer branch from d7cb603 to b4acf79 Compare May 14, 2022 03:36
@groodt groodt marked this pull request as ready for review May 14, 2022 03:59
@groodt groodt requested review from brandjon and lberki as code owners May 14, 2022 03:59
@thundergolfer thundergolfer requested review from thundergolfer and removed request for brandjon and lberki May 15, 2022 07:29
Copy link

@thundergolfer thundergolfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Appreciate the "other information" section.

python/pip_install/extract_wheels/lib/wheel.py Outdated Show resolved Hide resolved
python/pip_install/extract_wheels/lib/wheel.py Outdated Show resolved Hide resolved
@groodt
Copy link
Collaborator Author

groodt commented May 20, 2022

What’s the process for merging @thundergolfer ?

@f0rmiga
Copy link
Collaborator

f0rmiga commented May 23, 2022

@groodt I can merge it. I guess @thundergolfer was waiting for the review comments to be addressed. I checked they are fine. Thanks for addressing them.

@f0rmiga f0rmiga merged commit 9b8ab1e into bazelbuild:main May 23, 2022
ianoc pushed a commit to ianoc/rules_python that referenced this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants