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

tweak(whl_library): capture arbitrary files as data #1730

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

arrdem
Copy link
Contributor

@arrdem arrdem commented Jan 30, 2024

This fixes wheels like ruff which want to emplace bin/ and other dirs used as library data.

@aignas
Copy link
Collaborator

aignas commented Jan 31, 2024

Thanks for the PR @arrdem, could you please fix the tests and add a CHANGELOG item so that we can merge it? This looks like a really useful feature.

@arrdem
Copy link
Contributor Author

arrdem commented Jan 31, 2024

Wilco. Work's a bit crazy atm just wanted to get this patch in the upstream pipe before I forgot about it since it was trivial.

@arrdem
Copy link
Contributor Author

arrdem commented Feb 14, 2024

There we go. Ready @aignas.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-read this again and LGTM

@aignas
Copy link
Collaborator

aignas commented Apr 6, 2024

Could you add a Changelog note so that I can merge?

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, could you please add a line in the Changed section in the Changelod.md file please? It is not automatically generated from the commit messages, so each contributions needs to have an entry there. Thanks for the PR!

@arrdem arrdem force-pushed the arrdem/arbitrary-whl-data-files branch from 8e4ebc0 to deab84d Compare April 19, 2024 03:51
@arrdem
Copy link
Contributor Author

arrdem commented Apr 19, 2024

Hrm. Reading through https://peps.python.org/pep-0491/#file-contents, I'm wondering if this actually makes sense. It feels like while it may well make sense to have a :data filegroup, we probably want to handle at lest data, scripts and include separately. While for my purposes it's good enough to just have a build rule that encloses the ruff binary it would be better if we could identify scripts as part of unpacking the wheel and make them usable as entrypoint rules rather than just files.

On the same note whether the data should be a dependency of the library seems arguable.

@aignas
Copy link
Collaborator

aignas commented Jul 26, 2024

Looking at https://peps.python.org/pep-0491/#install-paths I am wondering if the right way to fix this would be to add the extra filegroup targets for each item listed in the UNIX install scheme.

That said, we may want to include all of those filegroups into the py_library so that the library can work correctly - Python can do whatever and assume that the files are installed in site-packages so I am wondering if adding the filegroups on the other hand would be not effective at all.

That said having the filegroups exposed would make the numpy header inclusion easier and users would not need to use whl_filegroup in their build graph to just extract a few files.

@arrdem arrdem closed this Jul 26, 2024
@arrdem arrdem reopened this Jul 26, 2024
@arrdem
Copy link
Contributor Author

arrdem commented Jul 26, 2024

Missclick.

That makes sense to me. The main other thing I want to do is to teach the wheel installer to identify scripts/binaries already in the bin/ tree and add entrypoint metdata for them automagically. Will try to revisit this.

@aignas
Copy link
Collaborator

aignas commented Jul 27, 2024

Thanks! Is it so that you could later use the py_console_script_binary to use those scripts?

@groodt
Copy link
Collaborator

groodt commented Sep 21, 2024

I was thinking about this recently, and I'm wondering if the neatest way of solving this is to mirror what was done for py_console_script_binary aka console_scripts aka entry_points?

The main motivator for accessing scripts are static binaries such as ruff, uv etc. However, scripts are actually a non-standardized feature with distutils/setuptools origins. They're not actually recommended, but they can't be removed because they're load bearing...

See:

Maybe we should consider the following:

  • Provide access to the .data from the wheel
  • Provide a py_script_binary so that users can fish out any named binaries (via ctx.run?)

There are some caveats that old school scripts can also be Python code. I see them periodically, but most of have been migrated to console_scripts so I think we can ignore these.

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.

3 participants