Skip to content

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?

*, 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
@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