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

Add support for python-wheel data directory #1777

Closed
BradHolmes opened this issue Feb 23, 2024 · 2 comments · Fixed by #1801
Closed

Add support for python-wheel data directory #1777

BradHolmes opened this issue Feb 23, 2024 · 2 comments · Fixed by #1801

Comments

@BradHolmes
Copy link
Contributor

🚀 feature request

Relevant Rules

//python:packaging.bzl - py_wheel

Description

The py_wheel rule should support the .data directory as specified in PEP 427, The .data directory. This is different than including data files in the source tree.

Describe the solution you'd like

It seems to me that changes are needed all the way down in wheelmaker, with arguments and logic that mirrors the support of the current flag 'extra_distinfo_file'. This would be exposed in py_wheel as perhaps data_files, and a similar ability to rename the files.

Describe alternatives you've considered

I haven't been able to think of any alternatives, but would be open to hear about them from others.


Finally, let me take a moment to say thank you for a wonderful project. It is really easy to use and adopt, especially with bazelmod.

If the solution I've described is on the right track, I can take the first pass at implementing and testing it.

@aignas
Copy link
Collaborator

aignas commented Feb 27, 2024

Thanks for the ticket. I think the need for adding data files is real and I am wondering if there is a way to make this work in a different way, for example using rules_pkg to somehow create a file mapping (I think it provides the renaming feature you mentioned about) and then consume that from rules python without adding an extra dependency on rules_pkg within the rules_python itself.

I haven't looked at the code yet, so I don't know if it is possible and how difficult it would be, but the fact that we need to do changes all the way through to the wheelmaker and introduce a code similar to the extra dist info file handling makes me suspect that we may have some legacy baggage here.

@BradHolmes
Copy link
Contributor Author

BradHolmes commented Feb 27, 2024

Thank you for the reply and support for this feature.

I agree, this is a fairly invasive change (though I believe backwards compatible). Good thought on rules_pkg, but the problematic lines in wheelmaker are here,

        self._distinfo_dir = (
            self._wheelname_fragment_distribution_name
            + "-"
            + self._version
            + ".dist-info/"
        )

I believe we need to put the usual files in that path, but data files in .... self._version + ".data/". I'm not enough of a bazel expert to know how to use rules_pkg to achieve that. Thus the straightforward (and invasive) suggestion to add a self._data_dir attribute.

BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 12, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 12, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 12, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 12, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 19, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 20, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 21, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
BradHolmes added a commit to BradHolmes/rules_python that referenced this issue Mar 21, 2024
This feature addes  attribute to the py_wheel rule and
makes a related change to the wheelmaker tool.

Fixes bazelbuild#1777
github-merge-queue bot pushed a commit that referenced this issue Mar 21, 2024
Fixes #1777

* Adds `data_files` attribute to `py_wheel` rule. 
* Minimal validation of the data-files target directories per
[specification](https://packaging.python.org/en/latest/specifications/binary-distribution-format/#installing-a-wheel-distribution-1-0-py32-none-any-whl)
* Added two tests.  
* Added example
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 a pull request may close this issue.

2 participants