-
-
Notifications
You must be signed in to change notification settings - Fork 685
feat(pypi): restrict pip.parse hub visibility #3755
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -510,6 +510,7 @@ def _create_whl_repos( | |
| 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, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we create a separate function for this? That way we make the code more maintainable and easier to test. |
||
| logger = logger, | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,7 @@ def parse_requirements( | |||||
| platforms = {}, | ||||||
| get_index_urls = None, | ||||||
| evaluate_markers = None, | ||||||
| exposed_requirements = [], | ||||||
| extract_url_srcs = True, | ||||||
| logger): | ||||||
| """Get the requirements with platforms that the requirements apply to. | ||||||
|
|
@@ -62,6 +63,9 @@ def parse_requirements( | |||||
| 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 | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we key this by_platform as well? |
||||||
| packages listed in these files should be exposed via the hub | ||||||
| repository. | ||||||
| extract_url_srcs: A boolean to enable extracting URLs from requirement | ||||||
| lines to enable using bazel downloader. | ||||||
| logger: repo_utils.logger, a simple struct to log diagnostic messages. | ||||||
|
|
@@ -92,6 +96,7 @@ def parse_requirements( | |||||
| reqs_with_env_markers = {} | ||||||
| index_url = None | ||||||
| extra_index_urls = [] | ||||||
| exposed_package_names = _exposed_package_names(ctx, exposed_requirements) | ||||||
| for file, plats in requirements_by_platform.items(): | ||||||
| logger.trace(lambda: "Using {} for {}".format(file, plats)) | ||||||
| contents = ctx.read(file) | ||||||
|
|
@@ -231,17 +236,34 @@ def parse_requirements( | |||||
| # for p in dist.target_platforms | ||||||
| # ] | ||||||
|
|
||||||
| 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: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the double negatives, maybe the following is easier to read?
Suggested change
|
||||||
| is_exposed = False | ||||||
|
Comment on lines
+241
to
+242
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like this logic to be somewhere outside the |
||||||
|
|
||||||
| item = struct( | ||||||
| # Return normalized names | ||||||
| name = normalize_name(name), | ||||||
| is_exposed = len(requirement_target_platforms) == len(requirements), | ||||||
| name = normalized_name, | ||||||
| is_exposed = is_exposed, | ||||||
| is_multiple_versions = len(reqs.values()) > 1, | ||||||
| index_url = pkg_sources.index_url if pkg_sources else "", | ||||||
| srcs = package_srcs, | ||||||
| ) | ||||||
| ret.append(item) | ||||||
| if not item.is_exposed and logger: | ||||||
| logger.trace(lambda: "Package '{}' will not be exposed because it is only present on a subset of platforms: {} out of {}".format( | ||||||
| if ( | ||||||
| exposed_package_names != None and | ||||||
| normalized_name not in exposed_package_names and | ||||||
| logger | ||||||
| ): | ||||||
| logger.trace(lambda: ( | ||||||
| "Package '{}' will not be exposed because it is not present " + | ||||||
| "in restrict_visibility_to" | ||||||
| ).format(name)) | ||||||
| if len(requirement_target_platforms) != len(requirements) and logger: | ||||||
| logger.trace(lambda: ( | ||||||
| "Package '{}' will not be exposed because it is only present " + | ||||||
| "on a subset of platforms: {} out of {}" | ||||||
| ).format( | ||||||
| name, | ||||||
| sorted(requirement_target_platforms), | ||||||
| sorted(requirements), | ||||||
|
|
@@ -251,6 +273,19 @@ def parse_requirements( | |||||
|
|
||||||
| return ret | ||||||
|
|
||||||
| def _exposed_package_names(ctx, exposed_requirements): | ||||||
| """Parse the requirement files that define hub-exposed package names.""" | ||||||
| if not exposed_requirements: | ||||||
| return None | ||||||
|
|
||||||
| exposed = {} | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| for file in exposed_requirements: | ||||||
| parse_result = parse_requirements_txt(ctx.read(file)) | ||||||
| for distribution, _ in parse_result.requirements: | ||||||
| exposed[normalize_name(distribution)] = None | ||||||
|
|
||||||
| return exposed | ||||||
|
|
||||||
| def _package_srcs( | ||||||
| *, | ||||||
| name, | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like adding this, but I imaging that we could repurpose this set of files for more labels.
@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.tomlfiles/file via the same interface in the future.