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

fix: enforce minimum version of docker/podman #1961

Merged
merged 13 commits into from
Sep 12, 2024

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Aug 11, 2024

This allows to always pass --platform to the OCI engine thus fixing issues with multiarch images.
It also allows to use docker cp instead of system tar workaround.

fix #1771
fix #1957
fix #1962
close #1968

Last time this was attempted, it failed on some CI (c.f. #1773), let's check the status now.

@mayeut mayeut marked this pull request as draft August 11, 2024 13:03
@mayeut mayeut force-pushed the docker-platform branch 2 times, most recently from a60a00c to 0569010 Compare August 11, 2024 18:15
@mayeut
Copy link
Member Author

mayeut commented Aug 12, 2024

gitlab might or might not fail if this goes in.
@joerick, we might want to push this to a branch of the pypa repo rather than my fork in order to test gitlab (If I remember correctly, that's the only way to test gitlab) ?

Travis-CI is failing (despite result being OK).
AppVeyor is another issue entirely, I don't think it's being tested anymore ? (since the move to pypa ?)

@mayeut mayeut force-pushed the docker-platform branch 3 times, most recently from b102b9c to 5ee397b Compare August 15, 2024 16:35
@mayeut
Copy link
Member Author

mayeut commented Aug 16, 2024

So far, the only CI that exhibited issues is Travis CI on graviton2 VM (still missing gitlab tests & Travis CI ppc64le/s390x).
It uses docker 19.03 on Ubuntu Focal (Jammy not available yet).

We can either allow docker 19.03 but print a warning or require TravisCI graviton2 VM users to update docker.
I'd rather see users update docker (it's a matter of adding 8 lines in .travis.yml) as this might also allow docker cp without any workarounds for legacy behavior.

@henryiii, @joerick any thoughts ?

@mayeut
Copy link
Member Author

mayeut commented Aug 17, 2024

AppVeyor build can be seen successful on my fork: https://ci.appveyor.com/project/mayeut/cibuildwheel/builds/50422602
CirrusCI build also (no more compute credits here): https://cirrus-ci.com/build/5147426338635776
I tested gitlab by pushing my branch to the pypa repo.

@mayeut mayeut marked this pull request as ready for review August 17, 2024 10:25
@joerick
Copy link
Contributor

joerick commented Aug 22, 2024

I'd rather see users update docker (it's a matter of adding 8 lines in .travis.yml) as this might also allow docker cp without any workarounds for legacy behavior.

That sounds fine to me. I assume this is the incantation to upgrade docker on that platform?

.travis.yml

addons:
  apt:
    sources:
      - sourceline: 'deb https://download.docker.com/linux/ubuntu $(lsb_release -cs) stable'
    packages:
    - docker-ce docker-ce-cli containerd.io

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.

Broadly, very happy with this!

Apologies, I didn't make it all the way through my last review so I've reviewed again and spotted a couple more things.

bin/run_tests.py Show resolved Hide resolved
cibuildwheel/linux.py Outdated Show resolved Hide resolved
@mayeut
Copy link
Member Author

mayeut commented Sep 3, 2024

There's an issue with s390x & ppc64le on Travis CI so this requires a bit more work...

@mayeut
Copy link
Member Author

mayeut commented Sep 4, 2024

It seems the installation of QEMU is failing on s390x & ppc64le which leads to the oci_container_test timeouts.

@mayeut
Copy link
Member Author

mayeut commented Sep 6, 2024

Converting to draft until the remaining (weird) issue on s390x gets solved.

@mayeut mayeut marked this pull request as draft September 6, 2024 18:45
@mayeut
Copy link
Member Author

mayeut commented Sep 6, 2024

The weird issue is that docker container ls does not seem to list the just created/running container...
Adding a retry, the unit test passes:

diff --git a/unit_test/oci_container_test.py b/unit_test/oci_container_test.py
index e913c203..803b09bd 100644
--- a/unit_test/oci_container_test.py
+++ b/unit_test/oci_container_test.py
@@ -8,6 +8,7 @@
 import subprocess
 import sys
 import textwrap
+import time
 from pathlib import Path, PurePath, PurePosixPath
 
 import pytest
@@ -134,16 +135,23 @@ def test_container_removed(container_engine):
     with OCIContainer(
         engine=container_engine, image=DEFAULT_IMAGE, oci_platform=DEFAULT_OCI_PLATFORM
     ) as container:
-        docker_containers_listing = subprocess.run(
-            f"{container.engine.name} container ls",
-            shell=True,
-            check=True,
-            stdout=subprocess.PIPE,
-            text=True,
-        ).stdout
         assert container.name is not None
-        assert container.name in docker_containers_listing
         old_container_name = container.name
+        retry_count = 3
+        while retry_count > 0:
+            retry_count -= 1
+            docker_containers_listing = subprocess.run(
+                f"{container.engine.name} container ls",
+                shell=True,
+                check=True,
+                stdout=subprocess.PIPE,
+                text=True,
+            ).stdout
+            if container.name in docker_containers_listing:
+                break
+            if retry_count == 0:
+                pytest.xfail("s390x travis...")
+            time.sleep(0.5)
 
     docker_containers_listing = subprocess.run(
         f"{container.engine.name} container ls",

I will let the debug run finish before pushing again to this branch skipping this test on Travis CI s390x...

@mayeut mayeut marked this pull request as ready for review September 7, 2024 08:24
@mayeut
Copy link
Member Author

mayeut commented Sep 7, 2024

Skipping the flaky test on s390x allowed to run all tests successfully.
Installing only test dependencies of cibuildwheel allows not to build some wheels on ppc64le/s390x (cryptography, pynacl, pyaml, ...) reducing setup time.

cibuildwheel/oci_container.py Outdated Show resolved Hide resolved
@mayeut mayeut marked this pull request as draft September 7, 2024 09:33
cibuildwheel/oci_container.py Outdated Show resolved Hide resolved
cibuildwheel/oci_container.py Outdated Show resolved Hide resolved
@mayeut mayeut marked this pull request as ready for review September 8, 2024 10:31
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.

looks great!

@henryiii henryiii merged commit 0787a44 into pypa:main Sep 12, 2024
28 checks passed
@mayeut mayeut deleted the docker-platform branch September 12, 2024 19:57
@cdce8p
Copy link

cdce8p commented Sep 13, 2024

FYI This change causes a build failure for mypy on manylinux and musllinux with Github actions and ubuntu-latest.
https://github.com/mypyc/mypy_mypyc-wheels/actions/runs/10852483095/job/30118641732#step:4:461

For the build, the mypy repo is first cloned in a previous step and cibuildwheel subsequently run with package-dir: mypy.

The error

fatal: detected dubious ownership in repository at '/project/mypy'

--
I was able to work around it by adding this to linux.before-all:

git config --global --add safe.directory {project}/mypy

The link to the PR: mypyc/mypy_mypyc-wheels#82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants