Skip to content
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

feature(py_wheel) Resolve wheel props from user defined properties file #1807

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgosk
Copy link

@mgosk mgosk commented Mar 15, 2024

Resolves #1804

Adds new custom_properties param to py_wheel. This feature allow to inject version generated by other target.

Tested with integration tests:
bazel build examples/wheel:version_from_other_target.dist

@mgosk mgosk requested a review from rickeylev as a code owner March 15, 2024 19:48
@mgosk mgosk force-pushed the version-file2 branch 2 times, most recently from dfb729e to e79fea6 Compare March 15, 2024 20:03
@aignas
Copy link
Collaborator

aignas commented Mar 17, 2024

I personally liked the suggested version_file attribute in the rule. I am wondering what other problems would this more generic templating rule solve?

Since expand_template rules already exist in bazel-skylib or aspect's bazel lib, I am not sure if rules_python should be doing templating here.

What do you think?

@mgosk
Copy link
Author

mgosk commented Mar 18, 2024

ad1.

My initial idea also was to use simpler approach with :version_file with version inside. Unfortunately name of output wheel file is dynamically generated (contains version ) so I couldn't do it in build phase. In this situation I moved implementation to run phase next to stamping.

Right now I don't see any other use cases for custom_properties than passing version. I decided to implement feature this way to be consistent with stamping implementation.

ad2.

skylib expand_template works only on files in build phase. Current templating is done during run phase in wheelmaker python script.

Right now I see three potential solutions, please advise which is most suitable for you :

  1. Keep implementation from this PR
  2. Pass just :version_file param instead of custom_properites and use dedicated templating for this variable
  3. Refactor py_wheel rule and use static wheelfile name. It breaks the py_wheel API but looks like more "bazelish" solution

@mgosk
Copy link
Author

mgosk commented Mar 19, 2024

Hi @aignas any advices how I can improve this PR to get it merged

@aignas
Copy link
Collaborator

aignas commented Mar 25, 2024

My initial thoughts are that having a bigger API surface may add extra maintenance to the rule set. Having the version taken from a file sounds like something that has precedent, e.g. rules_oci is doing this - it can only support a file for remote tags when pushing an artifact.

The bazel-skylib expand_template does not support templating based on the stamp value, but aspect's library does, similarly to how it is documented in rules_oci documentation here. A similar thing can be done with a simple genrule (although that if you don't want to depend on aspect's bazel lib. This is how one might do that with the genrule - it is one of the first search results, so please ignore the py_library code in the example.

Does this change your thoughts about how this should be implemented?

Copy link

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.
Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

@github-actions github-actions bot added the Can Close? Will close in 30 days if there is no new activity label Sep 21, 2024
@aignas aignas removed the Can Close? Will close in 30 days if there is no new activity label Sep 27, 2024
@aignas
Copy link
Collaborator

aignas commented Sep 27, 2024

I got hit by the dynamic output filename (depends on the version) as well and I think the best way would be to use a dir artifact. I think that would be the cleanest approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define wheel version via file/target
2 participants