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.

Sign up for GitHub

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

Jump to bottom

feat(pip): provide pypi -> whl target mapping in requirements.bzl #1532

Merged
merged 4 commits into from Nov 2, 2023
Merged

feat(pip): provide pypi -> whl target mapping in requirements.bzl #1532

merged 4 commits into from Nov 2, 2023

Conversation

alexeagle
Copy link
Collaborator

@alexeagle alexeagle commented Nov 1, 2023 •
edited

Currently a BUILD file can load all_whl_requirements but then can't determine which one is associated with a given package installed by the user.
This makes it impossible to build rules where the user can choose a given package and then override the wheel used, for example. @mattem and I are working at a client where we need this ability.

This PR makes a small, non-breaking refactoring to the generated requirements.bzl file so that this information is available in a new all_whl_requirements_by_package symbol.

Users can then do something like this:

load("@pip//:requirements.bzl", "all_whl_requirements_by_package")

some_rule(
  wheels = dict(all_whl_requirements_by_package, **{
    "charset-normalizer": "@charset_1_2_3//:file"
  }),
)

@alexeagle
refactor: requirements.bzl allows wheel files to be traced from the p…
8a23cbc 
…ackage that installed them

This is needed in custom rules that want to locate the wheel file for a given package name.

Demo:

```
$ head -15 $(bazel info output_base)/external/pypi/requirements.bzl
"""Starlark representation of locked requirements.

@generated by rules_python pip_parse repository rule
from @//:requirements_lock.txt
"""

load("@rules_python//python/pip_install:pip_repository.bzl", "whl_library")

all_requirements = ["@pypi//certifi:pkg", "@pypi//chardet:pkg", "@pypi//idna:pkg", "@pypi//pathspec:pkg", "@pypi//python_dateutil:pkg", "@pypi//python_magic:pkg", "@pypi//pyyaml:pkg", "@pypi//requests:pkg", "@pypi//s3cmd:pkg", "@pypi//setuptools:pkg", "@pypi//six:pkg", "@pypi//urllib3:pkg", "@pypi//yamllint:pkg"]

all_whl_requirements_by_package = {"certifi": "@pypi//certifi:whl", "chardet": "@pypi//chardet:whl", "idna": "@pypi//idna:whl", "pathspec": "@pypi//pathspec:whl", "python_dateutil": "@pypi//python_dateutil:whl", "python_magic": "@pypi//python_magic:whl", "pyyaml": "@pypi//pyyaml:whl", "requests": "@pypi//requests:whl", "s3cmd": "@pypi//s3cmd:whl", "setuptools": "@pypi//setuptools:whl", "six": "@pypi//six:whl", "urllib3": "@pypi//urllib3:whl", "yamllint": "@pypi//yamllint:whl"}

all_whl_requirements = all_whl_requirements_by_package.values()

all_data_requirements = ["@pypi//certifi:data", "@pypi//chardet:data", "@pypi//idna:data", "@pypi//pathspec:data", "@pypi//python_dateutil:data", "@pypi//python_magic:data", "@pypi//pyyaml:data", "@pypi//requests:data", "@pypi//s3cmd:data", "@pypi//setuptools:data", "@pypi//six:data", "@pypi//urllib3:data", "@pypi//yamllint:data"]
```
@alexeagle alexeagle requested a review from aignas November 1, 2023 20:05
@alexeagle alexeagle changed the title refactor: requirements.bzl allows wheel files to be traced from the p… refactor: allow wheel targets to be traced from the package Nov 1, 2023
@alexeagle alexeagle requested a review from mattem November 1, 2023 20:16
@alexeagle alexeagle marked this pull request as ready for review November 1, 2023 20:31
rickeylev
rickeylev requested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Please also update the bzlmod implementation:

  • python/private/bzlmod/pip_repository.bzl
  • python/private/bzlmod/requirements.bzl.tmpl

@rickeylev rickeylev changed the title refactor: allow wheel targets to be traced from the package feat(pip): provide pypi -> whl target mapping in requirements.bzl Nov 2, 2023
@rickeylev
Copy link
Contributor

Can you also update the changelog, please?

@alexeagle
Copy link
Collaborator Author

@rickeylev attempting to update the bzlmod implementation, but I'm not seeing the code used. Even in examples/pip_parse with --enable_bzlmod, I still get the requirements.bzl produced by the implementation I modified. Is the new bzlmod implementation not wired up yet?

@rickeylev
Copy link
Contributor

Yes, it's wired up.

The dependency goes:

python/extension/pip.bzl -> python/private/bzlmod/pip.bzl -> python/private/bzlmod/pip_repository.bzl -> python/private/bzlmod/requirements.bzl.tmpl

@alexeagle
Copy link
Collaborator Author

Thanks, updated.

I found one issue that I don't think I want to try to solve here: in this code
https://github.com/bazelbuild/rules_python/blob/main/python/private/bzlmod/pip.bzl#L124-L128
we overwrite whl_name with the normalized name, but only after a comment about how users shouldn't need to guess our sanitization scheme. I agree with that comment - but FWICT we then use the normalized whl_name in places that ARE user-affordances like the whl_overrides.

As a consequence, under the bzlmod implementation, my feature has the normalized names appearing as the keys in all_whl_requirements_by_package. I think that's not desirable, but it seems like a substantial refactoring to instead take more care about the normalized whl_name vs. the original distribution name.

@rickeylev rickeylev added this pull request to the merge queue Nov 2, 2023
Merged via the queue into main with commit 78fc4de Nov 2, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aignas aignas aignas approved these changes

@rickeylev rickeylev rickeylev approved these changes

@mattem mattem Awaiting requested review from mattem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@alexeagle @rickeylev @aignas