Added stamp attribute to py_wheel - #554
Conversation
|
This PR is blocked by #555 |
|
This PR is very similar to and inspired by #545 (Thanks for putting that together!). The only notable difference is that the |
|
I don't understand why it's blocked on #555 - aren't these orthogonal? why do you need to reorganize packages in order to add these files? |
alexeagle
left a comment
There was a problem hiding this comment.
really nice, seems like the right shape to me
| default = "py3", | ||
| doc = "Supported Python version(s), eg `py3`, `cp35.cp36`, etc", | ||
| ), | ||
| "stamp": attr.int( |
There was a problem hiding this comment.
this is a good docstring, and I think it's enough copy-paste that we should consider sharing this attribute in the stamp.bzl file
load("//path/to:stamp.bzl", "STAMP_ATTR")
attrs = dict(STAMP_ATTR, **{
more attrs
})
There was a problem hiding this comment.
especially because stamp and _stamp_flag should always come as a pair
There was a problem hiding this comment.
I would like to minimize the use of stamp.bzl, if you read the comment at the top of the file, I think it should ideally be deleted so it should ideally be self-contained and as isolated as possible.
There was a problem hiding this comment.
no one is working on bazelbuild/bazel#11164 and all other rulesets have already gone ahead with some local solution. pretty sure what we're doing here will be as permanent as rules_python.
There was a problem hiding this comment.
I don't think it should be but I can unfortunately see that happening. Regardless, I don't think rules_python should be loading attributes more from stamp.bzl. If the desire is to not have duplicate docs, I'll delete them from stamp.bzl in favor of something like "see py_wheel.stamp"
There was a problem hiding this comment.
py_wheel isn't the only rule that needs stamping, I'm sure there are others or there will be. maybe it can be refactored when we add the second one
I do not think the |
|
@alexeagle Responded to your reviews. Ready for another pass. |
|
moving the package boundary just moves the colon, right? why does it matter if it's |
Because I believe |
|
@alexeagle forgot to push earlier, this PR is now ready for another review. Sorry for the confusion! |
|
since you made two PRs, lets keep their comment threads distinct... |
|
(This did cause a minor break in our use of wheelmaker, fixed by google-deepmind/lab2d@b0921a5, though I suppose what we're doing isn't exactly relying on any public guarantees.) |
PR Checklist
Please check if your PR fulfills the following requirements:
.parfiles. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, there is now way to get workspace status information into a wheel. This is particularly useful when trying to embed things like git hashes or build numbers into a wheel's patch version, which is normally easy outside of Bazel.
Issue Number: N/A
What is the new behavior?
This change introduces the
stampattribute to thepy_wheelrule which allows users to specify how their targets would interact with the --stamp flag.The changes here follow the conventions of py_binary::stamp.
Does this PR introduce a breaking change?
Other information