fix: more reliably validate Podman API version #2016
Conversation
It's possible for some container engines to report their versions with a dash (e.g., "4.9.4-rhel"), which breaks packaging.version.Version's ability to parse the string. This commit introduces a version_from_string method which santizies the version string and returns an instance of Version.
for more information, see https://pre-commit.ci
|
Aa far as I know, you cannot upload such a wheel on PyPI? |
Indeed, but this isn't a problem with parsing python package versions; it's that For example, on my machine: $ podman version -f "{{json .}}"
{"Client":{"APIVersion":"4.9.4-rhel","Version":"4.9.4-rhel","GoVersion":"go1.21.11 (Red Hat 1.21.11-1.module+el8.10.0+1831+fc70fba6)","GitCommit":"","BuiltTime":"Tue Aug 13 05:04:24 2024","Built":1723539864,"OsArch":"linux/arm64","Os":"linux"}}That Podman's reported version is "4.9.4-rhel" ends up causing the $ export CIBW_CONTAINER_ENGINE=podman
$ cibuildwheel
...
Here we go!
Starting container image quay.io/pypa/manylinux_2_28_aarch64:2024.09.09-0...
info: This container will host the build for cp312-manylinux_aarch64...
+ podman version -f '{{json .}}'
Traceback (most recent call last):
File "/home/zozobra/.local/share/pipx/venvs/cibuildwheel/lib64/python3.11/site-packages/cibuildwheel/oci_container.py", line 112, in _check_engine_version
client_api_version = Version(version_info["Client"]["APIVersion"])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/zozobra/.local/share/pipx/venvs/cibuildwheel/lib64/python3.11/site-packages/packaging/version.py", line 202, in __init__
raise InvalidVersion(f"Invalid version: '{version}'")
packaging.version.InvalidVersion: Invalid version: '4.9.4-rhel'(which in turn raises |
Also, lift the method up and prefix with a "_" to better match the existing conventions
for more information, see https://pre-commit.ci
zachlewis
changed the title
|
I should note here that the new minimum container engine version stuff that was recently committed breaks cibuildwheel on systems whose "docker" or "podman" CLI utilities do not support the I've only experienced this issue with Podman -- I have no idea if this is a problem with Docker. Likewise, I dunno if Docker ever uses dashes for their server or client Version or API Version; whereas Podman uses the same value for Version and API Version. That said, I propose using a slightly different mechanism for validating Docker and Podman versions:
|
Use the "podman --version" command instead of "podman version -f {{json .}}" for better reliability across distributions.
for more information, see https://pre-commit.ci
This comment is not true anymore. We don't use docker cp again. Do we still need the min version check, @mayeut? |
It seems like the cp command is used here: cibuildwheel/cibuildwheel/oci_container.py Line 355 in 4ccfcaf The minimum version check was just added in #1961, merged last week -- that's what prompted this PR, cuz fresh installs of cibuildwheel suddenly stopped working for me. I'm renaming this PR title to better reflect what it addresses. |
zachlewis
changed the title
|
I don't see a |
We still need the version check for |
The We can see that I'm running different versions of podman for the client and server side (most likely a thing on macOS/Windows ?) |
Lower Docker API check to 1.41 Podman versions are not PEP440 compliant, remove distro specific suffixes before parsing. Add tests with real-world outputs and some made up ones.
| ), | ||
| ( | ||
| "docker", | ||
| '{"Client":{"Version":"19.03.15","ApiVersion": "1.40"},"Server":{"ApiVersion": "1.40"}}', |
There was a problem hiding this comment.
I've kept the "Version" info here to get a real sense of what docker version the ApiVersion relates to. It's unused & just informative.
On second thoughts, now that With check: Without check: |
Huh... very interesting...! I appreciate you looking into this. I don't have the cycles to more thoroughly test this at the moment, but it sounds like this definitely is indicative problematic podman environments, like you've suggested. And, honestly, if I'm the only one reporting this trouble, I'm even more inclined to chalk it up to user-error. Feel free to ignore! The changes you've made to parsing the version string look terrific. Thanks again.
This also seems reasonable to me. It's a little less clear what the problem is, but the produced error message does imply that there may be a workaround that doesn't require upgrading Docker, which is pretty convenient. |
There was a problem hiding this comment.
Thanks for putting this together. Would you mind to change the method of version comparison? I think using packaging.version.Version here is wrong because it's specifically designed for python packages.
Here's a class I wrote for comparing flexible versions (it can live in util.py):
@total_ordering
class FlexibleVersion:
version_str: str
version_parts: tuple[int, ...]
suffix: str | None
def __init__(self, version_str: str) -> None:
self.version_str = version_str
# Split into numeric parts and the optional suffix
match = re.match(r"^(\d+(\.\d+)*)(.*)$", version_str)
if not match:
msg = f"Invalid version string: {version_str}"
raise ValueError(msg)
version_part, _, suffix = match.groups()
# Convert numeric version part into a tuple of integers
self.version_parts = tuple(map(int, version_part.split(".")))
self.suffix = suffix.strip() if suffix else ""
# Normalize by removing trailing zeros
self.version_parts = self._remove_trailing_zeros(self.version_parts)
def _remove_trailing_zeros(self, parts: tuple[int, ...]) -> tuple[int, ...]:
# Remove trailing zeros for accurate comparisons
# without this, "3.0" would be considered greater than "3"
while parts and parts[-1] == 0:
parts = parts[:-1]
return parts
def __eq__(self, other: object) -> bool:
if not isinstance(other, FlexibleVersion):
return NotImplemented
return (self.version_parts, self.suffix) == (other.version_parts, other.suffix)
def __lt__(self, other: object) -> bool:
if not isinstance(other, FlexibleVersion):
return NotImplemented
return (self.version_parts, self.suffix) < (other.version_parts, other.suffix)
def __repr__(self) -> str:
return f"FlexibleVersion('{self.version_str}')"
def __str__(self) -> str:
return self.version_strHere's the test case (could you also add this to utils_test.py?)
def test_flexible_version_comparisons():
assert FlexibleVersion("2.0") < FlexibleVersion("2.1")
assert FlexibleVersion("2.1") > FlexibleVersion("2")
assert FlexibleVersion("1.9.9") < FlexibleVersion("2.0")
assert FlexibleVersion("1.10") > FlexibleVersion("1.9.9")
assert FlexibleVersion("3.0.1") > FlexibleVersion("3.0")
assert FlexibleVersion("3.0") < FlexibleVersion("3.0.1")
# Suffix should not affect comparisons
assert FlexibleVersion("1.0.1-rhel") > FlexibleVersion("1.0")
assert FlexibleVersion("1.0.1-rhel") < FlexibleVersion("1.1")That's a bit more code, but it'll work for both Docker and Podman, and remove the podman hacks.
per review comment
This PR addresses two separate issues sometimes encountered with Podman:
podmancli utility does not support theversionsubcommand, whose output is currently used for ascertaining the "APIVersion".For clients whose
podman version -f "{{json .}}"command does function as expected, the reported "Version" and "APIVersion" values seem to match; so, this PR implements an alternate method of determining Podman's version that seems to behave more reliably across distributions + architectures: we use regex to match a valid version pattern from the single-line output of thepodman --versioncommand.fix #2020
*edit: rewritten for clarity