refactor(pypi): split out more utils and start passing 'abi_os_arch' around#2069
Conversation
|
currently stacked on #2068 |
…around This is extra preparation needed for bazel-contrib#2059. Summary: - Create `pypi_repo_utils` for more ergonomic handling of Python in repo context. - Split the resolution of requirements files to platforms into a separate function to make the testing easier. This also allows more validation that was realized that there is a need for in the WIP feature PR. - Make the code more robust about the assumption of the target platform label. Work towards bazel-contrib#260, bazel-contrib#1105, bazel-contrib#1868.
aignas
force-pushed
the
refactor/env-marker-prep-2
branch
from
42b8510 to
f2601eb
Compare
aignas
left a comment
aignas
left a comment
There was a problem hiding this comment.
Adding some comments to hopefully make the review easier.
| requirements_by_platform = pip_attr.requirements_by_platform, | ||
| requirements_linux = pip_attr.requirements_linux, | ||
| requirements_lock = pip_attr.requirements_lock, | ||
| requirements_osx = pip_attr.requirements_darwin, | ||
| requirements_windows = pip_attr.requirements_windows, | ||
| extra_pip_args = pip_attr.extra_pip_args, | ||
| requirements_by_platform = requirements_files_by_platform( | ||
| requirements_by_platform = pip_attr.requirements_by_platform, | ||
| requirements_linux = pip_attr.requirements_linux, | ||
| requirements_lock = pip_attr.requirements_lock, | ||
| requirements_osx = pip_attr.requirements_darwin, | ||
| requirements_windows = pip_attr.requirements_windows, | ||
| extra_pip_args = pip_attr.extra_pip_args, | ||
| python_version = major_minor, | ||
| logger = logger, | ||
| ), |
There was a problem hiding this comment.
The main idea is to normalize the pip_attr.requirements* attributes to a dict[Label, list[str]]. This makes the downstream code much simpler.
| requirement = select_requirement( | ||
| requirements, | ||
| platform = repository_platform, | ||
| platform = None if pip_attr.download_only else repository_platform, |
There was a problem hiding this comment.
If pip_attr.download_only == True then we want to just select the first requirement out of requirements array. Previously the code was burried and we were carrying to much data around. The simplification here means that select_requirement does not need to care about whe download_only value anywhere.
| for p in plats: | ||
| configured_platforms[p] = file | ||
|
|
||
| for file, plats in requirements_by_platform.items(): |
There was a problem hiding this comment.
The code above just got moved to a different file. The tests got also moved.
| requirement_line = requirement_line, | ||
| target_platforms = [], | ||
| extra_pip_args = extra_pip_args, | ||
| download = len(platforms) > 0, |
There was a problem hiding this comment.
This attribute got removed as it is no longer needed due to the changes in the select_requirement function.
| if target_platforms: | ||
| for p in target_platforms: | ||
| if not p.startswith("cp"): | ||
| fail("target_platform should start with 'cp' denoting the python version, got: " + p) |
There was a problem hiding this comment.
Just an extra validation of the internal model invariants.
| } | ||
| return list(platforms.keys()) | ||
|
|
||
| def _platform(platform_string, python_version = None): |
There was a problem hiding this comment.
This code has been lifted from parse_requirements.bzl without any changes except for addition of _platform function, which normalizes the output of the main function to always return cp311_os_arch type of values instead of the previous os_arch. That gets consumed properly in the render_pkg_aliases.bzl. If this is something that is still hard to review, I can split it out.
|
|
||
| _tests = [] | ||
|
|
||
| def _test_fail_no_requirements(env): |
There was a problem hiding this comment.
The tests got simplified in this function because I could split the functions.
| ]: | ||
| env.expect.that_dict(got).contains_exactly({ | ||
| "requirements_lock": [ | ||
| "cp311_linux_aarch64", |
There was a problem hiding this comment.
Note, that only when python_version is passed we are adding the cp311_ to the beginning of the platforms.
|
Rebased on |
It seems that a few things broke in recent commits:
- We are not using the `MODULE.bazel.lock` file and it seems that it is
easy to
miss when the components in the PyPI extension stop integrating well
together. This happened during the switch to `{abi}_{os}_{plat}` target
platform passing within the code.
- The logger code stopped working in the extension after the recent
additions
to add the `rule_name`.
- `repo_utils.getenv` was always getting `PATH` env var on bazel `6.x`.
This PR fixes both cases and updates docs to serve as a better reminder.
By
fixing the `select_whls` code and we can just rely on target platform
triples
(ABI, OS, CPU). This gets one step closer to maybe supporting optional
`python_version` which would address #1708. Whilst at it we are also
adding
different status messages for building the wheel from `sdist` vs just
extracting or downloading the wheel.
Tests:
- Added more unit tests and brought them in line with the rest of the
code.
- Checked manually for differences between the `MODULE.bazel.lock` files
in our
`rules_python` extension before #2069 and after this PR and there are no
differences except in the `experimental_target_platforms` attribute in
`whl_library`. Before this PR you would see that we do not select any
wheels
for e.g. `MarkupSafe` and we are always building from `sdist`.
Work towards #260.
2 participants
This is extra preparation needed for #2059.
Summary:
pypi_repo_utilsfor more ergonomic handling of Python in repo context.to make the testing easier. This also allows more validation that was realized
that there is a need for in the WIP feature PR.
Work towards #260, #1105, #1868.