Add experimental support for building wheels. #159
Conversation
Currently only pure python wheels are supported. This extension builds wheels directly, invoking a simple python script that creates the zip archive in the desired format instead of using distutils/setuptools. This will make building platform-dependent wheels easier in the future, as bazel will have full control on how extension code is built.
|
@dsully - another attempt at the same pull request. |
|
@brandjon @laurentlb - PTAL |
|
This looks really nice, great work! |
|
Erm. Sorry for the delay. I've asked @brandjon to take a look, since I'm somewhat unqualified at the moment to review it for I'd need to swap the problem domain into my brain.. |
experimental/python/BUILD
Outdated
| @@ -0,0 +1,18 @@ | |||
| # Copyright 2017 The Bazel Authors. All rights reserved. | |||
There was a problem hiding this comment.
Nit: Is the year correct for when the file was originally authored, or is it a copy/paste error? Fine to keep as-is. (Same issue for other copyright years.)
There was a problem hiding this comment.
Fixed. The code was written in 2018, this was indeed copy/paste error.
experimental/examples/wheel/BUILD
Outdated
| # Package data. We're building "example_minimal-0.0.1-py3-none-any.whl" | ||
| wheel_name="example_minimal", | ||
| version="0.0.1", | ||
| python="py3", |
There was a problem hiding this comment.
In PEP 427 this is called "python tag". Maybe we should rename the attribute to python_tag to make the connection clearer? It might also help clarify why this doesn't follow the native rules' convention of python_version = ["PY2" | "PY3"].
Along the same lines, perhaps wheel_name should be called "distribution"? I'll defer to your judgement if you think that's too obscure.
experimental/python/wheel.bzl
Outdated
| implementation = _py_wheel_impl, | ||
| attrs = { | ||
| "deps": attr.label_list( | ||
| doc = "Dependencies of this package, e.g. py_library rules", |
There was a problem hiding this comment.
I'd clarify "Dependencies of this package" by changing it to something like "Targets to be included in this distribution", and then make it precise by saying that all runfiles of these targets and their transitive dependencies will be available.
Nit: We tend to discourage "i.e." and "e.g." in user facing documentation since they're more likely to confuse non-native English speakers.
experimental/python/wheel.bzl
Outdated
|
|
||
| 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], |
There was a problem hiding this comment.
If your goal is only to get the Python source files, you can use dep[PyInfo].transitive_sources (see PyInfo). But using the runfiles may be more appropriate anyway since it will also include any miscellaneous data files.
There was a problem hiding this comment.
I do make heavy use of packaged data files.
| outfile=None): | ||
| self._name = name | ||
| self._version = version | ||
| self._build_tag = build_tag |
There was a problem hiding this comment.
FYI this appears unused by the rule.
|
Ack.
I'll try to implement such a split set of rules over the week.
wt., 19 mar 2019, 22:15 użytkownik Jon Brandvein <notifications@github.com>
napisał:
… ***@***.**** commented on this pull request.
------------------------------
In experimental/python/wheel.bzl
<#159 (comment)>
:
> @@ -107,10 +109,42 @@ def _py_wheel_impl(ctx):
py_wheel = rule(
implementation = _py_wheel_impl,
+ doc = """
+A rule for building Python Wheels.
+
+Wheels are Python distributi
F438
on format. This rule packages a set of
+targets into a single wheel.
+
+Currently only pure-python wheels are supported.
+
+Example:
+
+<code>
+py_library(name="main",
Nit: Format as buildifier would, to avoid encouraging users to use
non-canonical formatting:
py_library(
name = "main",
srcs = ["main.py"],
deps = [
"//experimental/[...]",
"//expeirmental/[...]",
],
)
(Same for the py_wheel below -- whitespace around =.)
------------------------------
In experimental/python/wheel.bzl
<#159 (comment)>
:
> + arguments = arguments,
+ executable = ctx.executable._wheelmaker,
+ progress_message = "Building wheel",
+ )
+ return [DefaultInfo(
+ files = depset([outfile]),
+ data_runfiles = ctx.runfiles(files = [outfile]),
+ )]
+
+py_wheel = rule(
+ implementation = _py_wheel_impl,
+ attrs = {
+ "deps": attr.label_list(
+ doc = "Dependencies of this package, e.g. py_library rules",
+ ),
+ "packages": attr.string_list(
On further reflection, I think it might be better to completely divorce
the policy decision of *what* files to package from the mechanism that
does this packaging. This makes it easier to change the behavior if users
have different needs -- or if everyone has the same need but it's different
from what we think is the best mechanism right now. :)
This could be done by replacing the deps attribute with a files
attribute. Instead of expanding the transitive runfiles of each item in
deps, we would just union the direct files of each item in files (that
is, take ctx.files.files). Then some other rule could be used to
aggregate the runfiles of some given deps and filter them based on package
prefix. Or not. Up to the user. (This does mean the filtering would be
moved from the Python tool into the new .bzl rule. I believe it may still
be possible to filter without flattening the depset by using Args.add_all's
map_each
<https://g3doc.corp.google.com/devtools/blaze/rules/g3doc/lib/Args.html?cl=head#add_all.map_each>
parameter.)
I was chatting with @aiuto <https://github.com/aiuto> about this but I
don't think we're 100% resolved. I'm out Wednesday but back Thursday.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AqnE8a9fYQSTv_b-_H0qEMFYxS_e4UzQks5vYVNlgaJpZM4aBkoG>
.
|
|
Question from the peanut gallery: at first glance it seems like this doesn't handle automatically the distinction between external and internal dependencies (as, e.g., pants does). I.e., ideally, any "internal" code that's not itself published to PyPI or elsewhere should be packaged into the wheel, but code that is published elsewhere shouldn't be directly included in the wheel, and should instead just be listed as a requirement of the wheel. Are there plans to handle this? |
|
Indeed, the distinction is not automatic. You can select what gets packaged
via the deps argument and declare external dependencies via requires
argument but so far it's manual.
I'm not sure we should be making that choice automatically; in particular,
one might want to create multiple wheels out of a single repository with
one of them depending on the other..
czw., 28 mar 2019 o 15:01 Alec Benzer <notifications@github.com> napisał(a):
… Question from the peanut gallery: at first glance it seems like this
doesn't handle automatically the distinction between external and internal
dependencies (as, e.g., pants does).
I.e., ideally, any "internal" code that's not itself published to PyPI or
elsewhere should be packaged into the wheel, but code that *is* published
elsewhere *shouldn't* be directly included in the wheel, and should
instead just be listed as a requirement of the wheel.
Are there plans to handle this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AqnE8Tbolk1u-pnNc_Y50wD2UAVu04VCks5vbL0dgaJpZM4aBkoG>
.
--
Paweł Stradomski
|
Yeah, that seems like a fair concern. Would there be a way to do some kind of automatic detection that one can opt in/out of? Perhaps combined with some way of marking any given target as "exported", to indicate that it should be referred to as a requirement and not packaged directly (I believe this is roughly how pants does it). |
|
With the current approach that separates collecting the files to package
and actual packaging it should be possible to write a variant that walks
the tree of dependencies and cuts off everything that belongs to a root
created by pip_import.
I don't have a good idea yet how to add entries to the "requires" list.
Perhaps entries from requirements.txt could be added though it's generally
a bad idea for a library. One way or another we're going to hit the issue
that in bazel we'd want a single version of a third-party package while the
wheel should specify a permissive range of accepted versions.
czw., 28 mar 2019, 17:21 użytkownik Alec Benzer <notifications@github.com>
napisał:
… I'm not sure we should be making that choice automatically; in particular,
one might want to create multiple wheels out of a single repository with
one of them depending on the other..
Yeah, that seems like a fair concern.
Would there be a way to do some kind of automatic detection that one can
opt in/out of? Perhaps combined with some way of marking any given target
as "exported", to indicate that it should be referred to as a requirement
and not packaged directly (I believe this is roughly how pants does it).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#159 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AqnE8XK_j6rBO4b7HpHQj5_9c37j556Aks5vbNPfgaJpZM4aBkoG>
.
|
|
OK, so I think such automatic solution should in principle be possible,
though it would require a change to how pip_import works so it could mark
py_libraries it creates as external resources (via a new provider). One
could then create a rule similar to py_package from current proposal that
boths skips files from external rules and adds proper "requires" lines.
Note that I'm unsure if this is even the right direction for bazel. It also
doesn't solve the issue of wide range of requirements vs narrow ones for
specific build. It would also be a quite large change, out of scope for
this PR
czw., 28 mar 2019 o 17:30 Paweł Stradomski <pstradomski@gmail.com>
napisał(a):
… With the current approach that separates collecting the files to package
and actual packaging it should be possible to write a variant that walks
the tree of dependencies and cuts off everything that belongs to a root
created by pip_import.
I don't have a good idea yet how to add entries to the "requires" list.
Perhaps entries from requirements.txt could be added though it's generally
a bad idea for a library. One way or another we're going to hit the issue
that in bazel we'd want a single version of a third-party package while the
wheel should specify a permissive range of accepted versions.
czw., 28 mar 2019, 17:21 użytkownik Alec Benzer ***@***.***>
napisał:
> I'm not sure we should be making that choice automatically; in particular,
> one might want to create multiple wheels out of a single repository with
> one of them depending on the other..
>
> Yeah, that seems like a fair concern.
>
> Would there be a way to do some kind of automatic detection that one can
> opt in/out of? Perhaps combined with some way of marking any given target
> as "exported", to indicate that it should be referred to as a requirement
> and not packaged directly (I believe this is roughly how pants does it).
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#159 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AqnE8XK_j6rBO4b7HpHQj5_9c37j556Aks5vbNPfgaJpZM4aBkoG>
> .
>
--
Paweł Stradomski
|
|
@brandjon - Anything else I should do for this pull request? |
|
Looks good! |
Add experimental support for building wheels. Currently only pure python wheels are supported. This extension builds wheels directly, invoking a simple python script that creates the zip archive in the desired format instead of using distutils/setuptools. This will make building platform-dependent wheels easier in the future, as bazel will have full control on how extension code is built.
Currently only pure python wheels are supported.
This extension builds wheels directly, invoking a simple python script
that creates the zip archive in the desired format instead of using
distutils/setuptools.
This will make building platform-dependent wheels easier in the future,
as bazel will have full control on how extension code is built.
This replaces previous pull request (#128) that had bad merge history.