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

feat: use manylinux-interpreters tool if present #1630

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Oct 1, 2023

This will allow dropping some EOL (and maybe some none EOL) python interpreters in manylinux images which would allow to reduce its size (faster for a vast majority of user, a small slow-down for users of those interpreters).

Updates to keep tests times down are not here yet, this is just to get things started.

This will allow dropping some EOL (and maybe some none EOL) python interpreters in manylinux images which would allow to reduce its size (faster for a vast majority of user, a small slow-down for users of those interpreters).
Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Small suggestion - Defaulting to true then changing to false seems a little odd, maybe this is more normal? Also, which manylinux-interpreters could be used? It even seems like you could make this one step by just suppressing the ensure call?

@@ -117,10 +117,20 @@ def check_all_python_exist(
*, platform_configs: Iterable[PythonConfiguration], container: OCIContainer
) -> None:
exist = True
has_manylinux_interpreters = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
has_manylinux_interpreters = True
has_manylinux_interpreters = False

Comment on lines +123 to +127
try:
# use capture_output to keep quiet
container.call(["manylinux-interpreters", "--help"], capture_output=True)
except subprocess.CalledProcessError:
has_manylinux_interpreters = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
try:
# use capture_output to keep quiet
container.call(["manylinux-interpreters", "--help"], capture_output=True)
except subprocess.CalledProcessError:
has_manylinux_interpreters = False
with contextlib.suppress(subprocess.CalledProcessError):
# use capture_output to keep quiet
container.call(["manylinux-interpreters", "--help"], capture_output=True)
has_manylinux_interpreters = True

(also add contextlib import above)

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

The comments are minor, I'm also happy with this as is for now. I assume it's good to get in so that manylinux can eventually require it.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

I'm happy as-is. Nice neat implementation thanks to the design of manylinux-interpreters :)

@joerick joerick merged commit 70fae8d into pypa:main Oct 3, 2023
18 checks passed
@mayeut mayeut deleted the manylinux-interpreters branch October 3, 2023 20:20
mayeut added a commit to mayeut/cibuildwheel that referenced this pull request Nov 2, 2024
mayeut added a commit that referenced this pull request Nov 9, 2024
…2066)

* review: address review comments from #1630

* fix: error message when `manylinux-interpreters ensure ...` failed

* fix: error message when `manylinux-interpreters ensure ...` failed (option b)
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