Added stamp attribute to py_wheel

[UebelAndre]

PR Checklist

Please check if your PR fulfills the following requirements:

• Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature (please, look at the "Scope of the project" section in the README.md file)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

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 stamp attribute to the py_wheel rule 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?

• Yes
• No

Other information

[UebelAndre]

This PR is blocked by #555

[UebelAndre]

This PR is very similar to and inspired by #545 (Thanks for putting that together!). The only notable difference is that the stamp attribute is added and follows the same behavior as py_binary. I'd very much like to get this functionality in. Hopefully the review goes smoothly 😄

[alexeagle]

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]

really nice, seems like the right shape to me

[alexeagle]

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
})

[alexeagle]

especially because stamp and _stamp_flag should always come as a pair

[UebelAndre]

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.

[alexeagle]

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.

[UebelAndre]

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"

[alexeagle]

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

[UebelAndre]

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?

I do not think the _stamp_flag target should be public API. As I mentioned at the top of the file, I think the need for the target should go away once bazelbuild/bazel#11164 is implemented. Unless there's a strong reason for it to be part of the public API, I think it should go in a "canonically private" package. I don't care necessarily how that looks but based on my understanding of bzl-visibility, this is the correct place for this target. Happy to move to any specific place you can suggest.

[UebelAndre]

@alexeagle Responded to your reviews. Ready for another pass.

[alexeagle]

moving the package boundary just moves the colon, right? why does it matter if it's //:private/thing or //private:thing ?

[UebelAndre]

moving the package boundary just moves the colon, right? why does it matter if it's //:private/thing or //private:thing ?

Because I believe bzl-visibility applies to the package. And that colon implies "package". I do not think this change is that impactful but to be safe made a separate PR for it to raise viability. The thing I care most about this particular target is that it's not part of any public API. I'm happy to make any change that satisfies that.

[UebelAndre]

@alexeagle forgot to push earlier, this PR is now ready for another review. Sorry for the confusion!

[alexeagle]

since you made two PRs, lets keep their comment threads distinct...

[tkoeppe]

(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.)