Skip to content

Add exclude-exts options #1128

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

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

Conversation

oraluben
Copy link

This PR adds an exclude-exts option to wheel config.

Why not use existing exclude?

  1. Previously .pyc and .pyo were hard-coded to be excluded and there was no way to configure this
  2. Pattern-based exclusion for file extensions is not always straightforward (e.g., using **/*.c* to exclude *.c, *.cpp, *.cu* would accidentally exclude compiled *.cpython*.so files)


This is additive to the SDist/Wheel exclude patterns. This applies to the final paths
in the wheel, and can exclude files from CMake output as well. Editable installs
may not respect this exclusion.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a :::{versionadded} 0.12 in here and any other sphinx relevant parts you see fit.

Comment on lines +313 to +314
default_factory=lambda: [".pyc", ".pyo"],
metadata=SettingsFieldMetadata(display_default="true"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
default_factory=lambda: [".pyc", ".pyo"],
metadata=SettingsFieldMetadata(display_default="true"),
default=[".pyc", ".pyo"],

display_default will show that literal text in the documentation, I believe you don't want it to display true there right? Also, the lambda is unnecessary.

self,
wheel_dirs: Mapping[str, Path],
exclude: Sequence[str] = (),
exclude_exts: Sequence[str] = (),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why we have defaults for these. But anyway it's an existing pattern.


:::{versionadded} 0.12

Support to exclude based on file extension.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the format looks right here, can you rearrange it? https://scikit-build-core--1128.org.readthedocs.build/en/1128/configuration/index.html#customizing-the-built-wheel

Also, this is new since the introduction of Config Reference, but can you use :confval:... role to reference the new option? There's a nice pop-up of the information about it, so you could de-duplicate some documentation with that.

@henryiii
Copy link
Collaborator

Could you explain why you need it? It's incorrect to place a .pyc file into a wheel; as an example, the magic number just changed between 3.14rc1 and 3.14rc2, and it's considered fine because .pyc's aren't part of wheels. And your example of how **/*.c* doesn't work doesn't make much sense when the replacement doesn't support globbing; so you'd have to do [".c", ".cpp"], but then ["*.c", "*.cpp"] would also have been safe.

I'm about to rework file inclusion/exclusion to support several modes, and this might be a good addition to that, but I'd like to start that work and see how it fits in before making a final decision. :)

@LecrisUT
Copy link
Collaborator

We probably would always inject the [".pyc", ".pyo"], but to have an extension-based way to filter does seem easier to manage without accidental footguns. From the example given by OP, I can see bullet shaped hole in the foot 😉

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