feat: stricter selector parsing, refactor to platforms module
#2291
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joerick
force-pushed
the
selector-strict
branch
from
88ac468 to
fe78ba6
Compare
- Use a different method to build nothing - Make the check aware of enable groups
Unit test time: 26.2s -> 13.1s
joerick
force-pushed
the
selector-strict
branch
2 times, most recently
from
f7c0412 to
6942c47
Compare
|
This change made the unit tests quite a bit slower (26s), so I broke out the profiler. In doing so I spotted a few places things could be sped up. I'll split out the unrelated changes to a separate PR. |
|
Do you know why this change made it slower, though? |
|
Yeah, it was these lines, it ended up doing a lot of TOML loading and running of the BuildSelector. |
joerick
force-pushed
the
selector-strict
branch
from
6942c47 to
7b94bcd
Compare
|
I've kept only the optimisation relevant to this change, the rest are in #2293. |
mayeut
reviewed
Mar 2, 2025
Comment on lines
+26
to
+35
| def get_platform_module(platform: PlatformName) -> PlatformModule: | ||
| if platform == "linux": | ||
| return linux | ||
| if platform == "windows": | ||
| return windows | ||
| if platform == "macos": | ||
| return macos | ||
| if platform == "pyodide": | ||
| return pyodide | ||
| assert_never(platform) |
There was a problem hiding this comment.
This function is probably not needed anymore given it's just a look-up in the ALL_PLATFORM_MODULES dict now ?
|
Can be rebased now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation of an idea from #2070 (comment)
cibuildwheel will error out if the
buildconfig contains something that it doesn't recognise. Forskipandtest-skip, it just prints a warning.This is to highlight misconfiguration as seen in #2070. Notably, the set of identifiers that cibuildwheel will consider is not dependent on platform, so users can still do a global CIBW_BUILD that's shared across platforms.
Also in this PR, I wanted to iterate through platform modules, and so decided it was time to factor them out into their own package.
I'll keep this draft to merge #2286 first, since that adds a new platform.