feat(pypi): restrict pip.parse hub visibility#3755
feat(pypi): restrict pip.parse hub visibility#3755Harshal96 wants to merge 2 commits intobazel-contrib:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the restrict_visibility_to attribute to pip.parse, allowing users to limit public hub aliases to direct dependencies specified in requirement files. Transitive dependencies not included in these files are kept internal to the generated wheel repositories, preventing their use as direct dependencies. The changes include updates to documentation, the pip.parse extension, requirement parsing logic, and visibility rendering in the hub repository, along with new unit tests. I have no feedback to provide as there were no review comments.
Harshal96
force-pushed
the
feat/pip-parse-restricted-visibility
branch
from
23b1141 to
a43c2f2
Compare
Add a restrict_visibility_to attribute for bzlmod pip.parse so hub aliases are public only for packages listed in direct requirement files. Keep all lockfile packages available to generated wheel repositories so transitive dependencies continue to resolve internally. Fixes bazel-contrib#3413
aignas
left a comment
aignas
left a comment
There was a problem hiding this comment.
Thank you for the contribution.
| the platforms stored as values in the input dict. Returns the same | ||
| dict, but with values being platforms that are compatible with the | ||
| requirements line. | ||
| exposed_requirements: List of requirements files. When present, only |
There was a problem hiding this comment.
Should we key this by_platform as well?
| if not exposed_requirements: | ||
| return None | ||
|
|
||
| exposed = {} |
There was a problem hiding this comment.
This acts like a union. Is this the right thing to do here?
From the modeling perspective, the requirements may be different based on the target platform. That said, I think this approach is OK in the parse_requirements internals. Since we are asking the user to provide the source lock files.
| a corresponding `python.toolchain()` configured. | ||
| """, | ||
| ), | ||
| "restrict_visibility_to": attr.label_list( |
There was a problem hiding this comment.
I like adding this, but I imaging that we could repurpose this set of files for more labels.
- Restrict visibility.
- Create a
@pypi//:lock.updatetarget automatically that includes thesrcspassed here assrcspassed to the lock file. This plumbing would be similar to whatrules_jvmhave with theirrepin.
We should also assume that we will need to support pyproject.toml files/file via the same interface in the future.
| extra_pip_args = pip_attr.extra_pip_args, | ||
| get_index_urls = self._get_index_urls.get(pip_attr.python_version), | ||
| evaluate_markers = _evaluate_markers(self, pip_attr), | ||
| exposed_requirements = pip_attr.restrict_visibility_to, |
There was a problem hiding this comment.
Should we create a separate function for this?
exposed_requirements = parse_dep_srcs(module_ctx, pip_attr.restrict_visibility_to)
That way we make the code more maintainable and easier to test.
| if exposed_package_names != None and normalized_name not in exposed_package_names: | ||
| is_exposed = False |
There was a problem hiding this comment.
I would like this logic to be somewhere outside the parse_requirements.bzl file. Would it be possible to move it to hub_builder.bzl? The output of parse_requirements have this is_exposed flag (that is an internal implementation detail) and if we make the lock sources mandatory, then we would be able to drop that implementation detail, which actually makes the code a little bit more complex than it needs to be.
|
|
||
| normalized_name = normalize_name(name) | ||
| is_exposed = len(requirement_target_platforms) == len(requirements) | ||
| if exposed_package_names != None and normalized_name not in exposed_package_names: |
There was a problem hiding this comment.
I don't like the double negatives, maybe the following is easier to read?
| if exposed_package_names != None and normalized_name not in exposed_package_names: | |
| if not (exposed_package_names and normalized_name in exposed_package_names): |
2 participants
Add a restrict_visibility_to attribute for bzlmod pip.parse so hub aliases are public only for packages listed in direct requirement files.
Keep all lockfile packages available to generated wheel repositories so transitive dependencies continue to resolve internally.
Fixes #3413
PR Instructions/requirements
type: descriptionformat. See CONTRIBUTING.md for types.CHANGELOG.mdas applicablesection at the bottom.
See CONTRIBUTING.md for our breaking changes process.