🧪 Pass sdist into cibuildwheel directly #82

webknjaz · 2024-05-28T19:17:30Z

This patch also brings a copy of cibuildwheel in through the GitHub Actions interface instead of PyPI.

It also fixes referencing the sdist artifact name from wheel building jobs.

webknjaz · 2024-05-28T19:21:50Z

@nitzmahone while working on this I also identified invalid output references in the artifact download steps. Will try to address that now.

It wasn't obvious right away, but referencing a job in the `${{ needs }}` context that is not being depended on, and does not even exist, silently returned an empty string for the whole expression passed to the `name:` input of the `download-artifact` action invocations. This subsequently triggered the behavior of said action to change to "download all the artifacts that exist in the workflow". This "download all" behavior has an additional quirk, though — it downloads each artifact in a subdirectory named after the artifact name. The behavior of the "download one specific artifact" mode, on the other hand, is different in that it does not create an extra level of nesting. This was the reason why it felt necessary to unpack two-levels deep sdist tarballs in the first place. With this patch, references to said sdists will change.

webknjaz · 2024-05-28T19:51:49Z

@nitzmahone I think 2bea383 constitutes a release blocker.

webknjaz · 2024-05-28T20:51:35Z

.github/workflows/ci.yaml

@@ -258,14 +260,14 @@ jobs:
      id: fetch_sdist
      uses: actions/download-artifact@v4
      with:
-        name: ${{ needs.build_sdist.outputs.artifact_name }}


(this was an empty string because there's no job with ID build_sdist; causing all the artifacts to be downloaded and unpacked in subdirectories)

Ugh- another bug introduced by moving back and forth between PyYAML and CFFI trying to standardize the workflows between them. GHA doesn't make it easy to debug these kinds of things because so many different layers just silently succeed with glaring issues like this. Their (lack of) expression type system also makes me appreciate Jinja so much more.

Interestingly, in the past, their YAML parser would error out if you were to reference an identifier not listed in the needs: key. It used to be rather hard to find that web page that displays the error too since the checks wouldn't appear in the PR.

webknjaz · 2024-05-28T20:52:39Z

.github/workflows/ci.yaml

-
-        mkdir cffi
-
-        tar zxf ${{ steps.fetch_sdist.outputs.download-path }}/cffi*.tar.gz/cffi*.tar.gz --strip-components=1 -C cffi


(with the requested GHA artifact name being non-empty, the new path is actually ${{ steps.fetch_sdist.outputs.download-path }}/cffi*.tar.gz)

Yeah- the artifact upload/download pathing is painful. It's even worse on Windows, since the GHA context/event properties are usually (but not always) Windows paths with drive letters and backslashes, and there aren't enough expression conversion operators to (reliably) convert those. Using bash on Windows for "glue" instead of cmd/PS helps some, but there were still some things that just plain didn't work properly, which is why I had to fall back to not using the artifact fetch paths in all cases.

Pretty sure bash can handle those. I had to work around this problem in some action already.

webknjaz · 2024-05-28T20:53:35Z

.github/workflows/ci.yaml

-
-        tar zxf ${{ steps.fetch_sdist.outputs.download-path }}/cffi*.tar.gz/cffi*.tar.gz --strip-components=1 -C cffi
+      with:
+        package-dir: >-


(cibuildwheel unarchives sdist tarballs if supplied in place of a directory)

I'm likely going to move the libffi build to its own cached-artifact job though, which means we'll still have to give cibuildwheel a full directory with an exploded tarball to get the cached artifact dir into the container envs alongside the sdist- that's why I abandoned just giving cibuildwheel the sdist tarball directly. I looked over the code- it didn't look like there was a way to build directly from an sdist tarball and send a project directory with build deps at the same time.

Any reason those things shouldn't just be in the sdist? It's supposed to be self-contained.

webknjaz · 2024-05-28T20:54:44Z

.github/workflows/ci.yaml

-        mkdir cffi
-
-        tar zxf ${{ steps.fetch_sdist.outputs.download-path }}/cffi*.tar.gz/cffi*.tar.gz --strip-components=1 -C cffi
-        python -m pip install --upgrade "${{ matrix.cibw_version || 'cibuildwheel' }}"


(I didn't keep any ${{ matrix.cibw_version }} references because it's likely some legacy leftover that doesn't actually exist anywhere)

We should leave support in for that in some form- they're necessary every time cibuildwheel doesn't support something we need to (eg, every pre-beta Python, or when cibuildwheel drops support for a target Python before we do). I don't want to have to plumb it back in every time, since we're always testing new Pythons well before cibuildwheel has released support for them, and I haven't gotten around to PRing any of my "bring your own Python" hacks to cibuildwheel.

Can I do that in a follow-up? There's no structure for it left in the matrix even. Pretty sure I could sketch a better solution anyway.

webknjaz · 2024-05-28T20:58:05Z

.github/workflows/ci.yaml

-        name: ${{ steps.build.outputs.artifact_name }}
-        path: dist/*.whl
+        name: ${{ steps.built-artifact-lookup.outputs.artifact_name }}
+        path: wheelhouse/${{ steps.built-artifact-lookup.outputs.artifact_name }}


(I replaced a wildcard with the known value, since we actually have it in a var already)

webknjaz · 2024-05-28T20:59:38Z

.github/workflows/ci.yaml


    - name: configure docker foreign arch support
      uses: docker/setup-qemu-action@v3
      if: ${{ ! contains(matrix.spec, 'x86_64') }}

    - name: build/test wheels
-      id: build
+      uses: pypa/[email protected]


(I'm referencing the official action, since it's better integrated into the ecosystem and none of the dynamic detection magic is actually necessary anymore)

webknjaz · 2024-05-28T21:00:21Z

.github/workflows/ci.yaml

@@ -29,6 +29,8 @@ env:
 jobs:
  python_sdist:
    runs-on: ubuntu-22.04
+    outputs:
+      artifact_name: ${{ steps.build_sdist.outputs.artifact_name }}


(this was another missing bit that prevented the wheel jobs from referencing the tarball name)

This patch also brings a copy of `cibuildwheel` in through the GitHub Actions interface instead of PyPI.

webknjaz mentioned this pull request May 28, 2024

Build wheels from sdist in CI #18

Closed

webknjaz force-pushed the maintenance/cibuildwheel-from-sdist branch 4 times, most recently from bf7ba01 to 5e4aee6 Compare May 28, 2024 19:48

webknjaz added 2 commits May 28, 2024 21:49

💅 Move whl name lookup to dedicated GHA job step

4e60e2a

webknjaz force-pushed the maintenance/cibuildwheel-from-sdist branch 2 times, most recently from 26b4f18 to 4314e7b Compare May 28, 2024 19:50

webknjaz commented May 28, 2024

View reviewed changes

🧪 Pass sdist into cibuildwheel directly

2eda2d1

This patch also brings a copy of `cibuildwheel` in through the GitHub Actions interface instead of PyPI.

webknjaz force-pushed the maintenance/cibuildwheel-from-sdist branch from 4314e7b to 2eda2d1 Compare May 28, 2024 21:04

webknjaz mentioned this pull request May 28, 2024

CI refresh for 1.17.0b1 #79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧪 Pass sdist into cibuildwheel directly #82

🧪 Pass sdist into cibuildwheel directly #82

webknjaz commented May 28, 2024 •

edited

Loading

webknjaz commented May 28, 2024

webknjaz commented May 28, 2024

webknjaz May 28, 2024

nitzmahone May 29, 2024

webknjaz May 29, 2024

webknjaz May 28, 2024

nitzmahone May 29, 2024

webknjaz May 29, 2024

webknjaz May 28, 2024

nitzmahone May 29, 2024 •

edited

Loading

webknjaz May 29, 2024

webknjaz May 28, 2024 •

edited

Loading

nitzmahone May 29, 2024

webknjaz May 29, 2024

webknjaz May 28, 2024

webknjaz May 28, 2024

webknjaz May 28, 2024


		mkdir cffi

		tar zxf ${{ steps.fetch_sdist.outputs.download-path }}/cffi.tar.gz/cffi.tar.gz --strip-components=1 -C cffi

🧪 Pass sdist into cibuildwheel directly #82

Are you sure you want to change the base?

🧪 Pass sdist into cibuildwheel directly #82

Conversation

webknjaz commented May 28, 2024 • edited Loading

webknjaz commented May 28, 2024

webknjaz commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitzmahone May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented May 28, 2024 •

edited

Loading

nitzmahone May 29, 2024 •

edited

Loading

webknjaz May 28, 2024 •

edited

Loading