-
Notifications
You must be signed in to change notification settings - Fork 541
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
feat(py_wheel): deps
can provide requirement specifiers for Requires-Dist
#1776
Conversation
Example "Implementation of py_package rule"
load("@rules_python//python:packaging.bzl", "PyRequrirementInfo", "PyRequrirementsInfo")
PYPI_LOCK_PREFIX = "../pypi/_lock/"
def _py_package_impl(ctx):
inputs = depset(
transitive = [dep[DefaultInfo].data_runfiles.files for dep in ctx.attr.deps] +
[dep[DefaultInfo].default_runfiles.files for dep in ctx.attr.deps],
)
files = []
requirements = []
for input_file in inputs.to_list():
filename = input_file.short_path
if filename.startswith(PYPI_LOCK_PREFIX):
filename = filename[len(PYPI_LOCK_PREFIX):]
name, version = filename.split("@")
if name and version:
requirements.append(PyRequrirementInfo(
name = name,
version = version,
specifier = "%s>=%s" % (name, version),
))
else:
files.append(input_file)
return [
DefaultInfo(
files = depset(direct = files),
),
PyRequrirementsInfo(
label = ctx.label,
requirements = requirements,
),
]
py_package = rule(
doc = """\
A rule to select all files in transitive dependencies of deps which
belong to given set of Python packages.
This rule is intended to be used as data dependency to py_wheel rule.
""",
implementation = _py_package_impl,
attrs = {
"deps": attr.label_list(
doc = "",
),
},
)
|
I like the idea but was wondering if the py_project rule should be part of the rules_python, at least as an example if we are not sure that we want to increase the API surface we are maintaining. What do you think, @pcj? |
I'm happy to include it, perhaps under a directory like cc @jvolkman |
python/private/py_wheel.bzl
Outdated
"specifier": "String: a formatted string, to be used verbatim for 'Requires-Dist' metadata specification (e.g 'protobuf>=4.25.2', see https://pip.pypa.io/en/stable/reference/requirement-specifiers/)", | ||
"version": "String: the concrete version of the pypi requirement (e.g. '4.25.2')", |
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.
Aren't specifier
and version
redundant? Do you need both?
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.
Yes it is redundant. I left version
in because I thought someone might want to know the concrete version rather than having to parse the specifier, but since I don't have a specific use for it myself, should probably be removed for the sake of simplicity (could be added later)
"name": "String: the name of the pypi dependency (e.g. 'protobuf')", | ||
"specifier": "String: a formatted string, to be used verbatim for 'Requires-Dist' metadata specification (e.g 'protobuf>=4.25.2', see https://pip.pypa.io/en/stable/reference/requirement-specifiers/)", | ||
"version": "String: the concrete version of the pypi requirement (e.g. '4.25.2')", | ||
}, |
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.
Does it make sense to also include markers
?
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.
Probably? I'm somewhat of an imposter when it comes to python, but after doing more reading it would make sense to include them (List[String]
?)
Not sure it makes sense to have this example in rules_python as written, with dependencies on Could the example be retooled to rely only on stuff in rules_python? |
Added |
@@ -316,17 +316,45 @@ tasks: | |||
working_directory: examples/multi_python_versions | |||
platform: windows | |||
|
|||
integration_test_custom_py_project_ubuntu_min_workspace: |
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.
This will deplete the number of runners we have available for running builds. I remember @rickeylev did some work to optimize this so maybe relying on the bazel integration test framework would be better here?
@@ -21,6 +21,9 @@ A brief description of the categories of changes: | |||
|
|||
[0.XX.0]: https://github.com/bazelbuild/rules_python/releases/tag/0.XX.0 | |||
|
|||
* (py_wheel) Wheel deps can supply requirement specifiers to the generated wheel |
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.
nit: please rebase and put the bullet point into an appropriate section.
- the golden test depends on the generated wheel, whose METADATA has been | ||
extracted and grepped for `Requires-Dist:`. | ||
|
||
> NOTE: this example was originally based off `examples/pip_parse_vendored`. |
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.
Any new example code should be compatible with bzlmod
, IMHO so that it is easier to to maintain the examples in the future. Are there reasons to not do this in this case?
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 just copied over the files from examples/pip_parse_resolved
, but perhaps a different example would be better as a base. Alternatively, I can just remove the WORKSPACE bits.
|
||
load("@rules_python//python:packaging.bzl", "PyRequrirementInfo", "PyRequrirementsInfo") | ||
|
||
SITE_PACKAGES = "/site-packages/" |
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.
This may break if we merge #1766, I wonder if there is a better way to do this.
a392fd5
to
5fcb3fe
Compare
5fcb3fe
to
ee837de
Compare
This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. |
This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?" |
This feature adds two new providers (
PyRequirementInfo
andPyRequirementsInfo
) topy_wheel.bzl
.Downstream users can use these Providers to supply additional requirement specifiers to the generated wheel metadata.
The use case is that we'd like wheels to contain files limited to transitive dependencies not including those which are available from pypi directly. If we use
py_package
out of the box, we get "fat wheels" that contain too many files (all ofprotobuf
, for example). Rather, we'd like to be able to have our own "thinner" implementation ofpy_package
that filters protobuf out, but also includeRequires-Dist: protobuf>=4.25.2
in the wheel metadata.This is similar but more limited in scope than #1182.