Skip to content

🧪 Pass sdist into cibuildwheel directly #82

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

🧪 Pass sdist into cibuildwheel directly #82

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bea383
🐛🧪 Fix referencing GHA artifact @ whl builders
webknjaz May 28, 2024
4e60e2a
💅 Move whl name lookup to dedicated GHA job step
webknjaz May 28, 2024
2eda2d1
🧪 Pass sdist into `cibuildwheel` directly
webknjaz May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 40 additions & 49 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ env:
jobs:
python_sdist:
runs-on: ubuntu-22.04
outputs:
artifact_name: ${{ steps.build_sdist.outputs.artifact_name }}
Copy link
Author

Choose a reason for hiding this comment

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

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

steps:
- name: clone repo
uses: actions/checkout@v4
Expand Down Expand Up @@ -258,14 +260,14 @@ jobs:
id: fetch_sdist
uses: actions/download-artifact@v4
with:
name: ${{ needs.build_sdist.outputs.artifact_name }}
Copy link
Author

Choose a reason for hiding this comment

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

(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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

name: ${{ needs.python_sdist.outputs.artifact_name }}

- 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]
Copy link
Author

Choose a reason for hiding this comment

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

(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)

env:
CFLAGS: -Dffi_call=cffistatic_ffi_call # override name for ffi_call to break hard if we linked against someone else's libffi
CIBW_ARCHS_LINUX: all
Expand All @@ -291,24 +293,21 @@ jobs:
CIBW_PRERELEASE_PYTHONS: 'True'
CIBW_TEST_REQUIRES: pytest setuptools # 3.12+ no longer includes distutils, just always ensure setuptools is present
CIBW_TEST_COMMAND: PYTHONUNBUFFERED=1 python -m pytest ${{ matrix.test_args || '{project}' }} # default to test all
run: |
set -eux

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' }}"
Copy link
Author

@webknjaz webknjaz May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.


# actually build libffi + wheel (using env tweaks above)
python -m cibuildwheel --output-dir dist ./cffi
with:
package-dir: >-
${{ steps.fetch_sdist.outputs.download-path
}}/${{ needs.python_sdist.outputs.artifact_name }}

echo "artifact_name=$(ls ./dist/)" >> "$GITHUB_OUTPUT"
- name: determine built wheel filename
id: built-artifact-lookup
run: echo "artifact_name=$(ls ./wheelhouse/)" >> "${GITHUB_OUTPUT}"
shell: bash -eEuxo pipefail {0}

- name: upload artifacts
uses: actions/upload-artifact@v4
with:
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 }}
Copy link
Author

Choose a reason for hiding this comment

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

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

if-no-files-found: error
if: ${{ env.skip_artifact_upload != 'true' }}

Expand Down Expand Up @@ -389,7 +388,7 @@ jobs:
id: fetch_sdist
uses: actions/download-artifact@v4
with:
name: ${{ needs.build_sdist.outputs.artifact_name }}
name: ${{ needs.python_sdist.outputs.artifact_name }}

- name: install python
uses: actions/setup-python@v5
Expand All @@ -398,36 +397,33 @@ jobs:
# built-in virtualenv/pip are pinned to busted versions that fail on newer Pythons

- name: build wheel prereqs
run: |
set -eux
python3 -m pip install --user --upgrade "${{ matrix.cibw_version || 'cibuildwheel' }}"
brew uninstall --ignore-dependencies libffi 2>&1 || true
run: brew uninstall --ignore-dependencies libffi 2>&1 || true
shell: bash -eux {0}

- name: build/test wheels
id: build
uses: pypa/[email protected]
env:
CIBW_BUILD: ${{ matrix.spec }}
CIBW_PRERELEASE_PYTHONS: 'True'
CIBW_TEST_REQUIRES: pytest setuptools
CIBW_TEST_COMMAND: pip install pip --upgrade; cd {project}; PYTHONUNBUFFERED=1 pytest
MACOSX_DEPLOYMENT_TARGET: ${{ matrix.deployment_target || '10.9' }}
SDKROOT: ${{ matrix.sdkroot || 'macosx' }}
run: |
set -eux

mkdir cffi

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

with:
package-dir: >-
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@nitzmahone nitzmahone May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

${{ steps.fetch_sdist.outputs.download-path
}}/${{ needs.python_sdist.outputs.artifact_name }}

python3 -m cibuildwheel --output-dir dist cffi

echo "artifact_name=$(ls ./dist/)" >> "$GITHUB_OUTPUT"
- name: determine built wheel filename
id: built-artifact-lookup
run: echo "artifact_name=$(ls ./wheelhouse/)" >> "${GITHUB_OUTPUT}"
shell: bash -eEuxo pipefail {0}

- name: upload artifacts
uses: actions/upload-artifact@v4
with:
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 }}
if-no-files-found: error
if: ${{ env.skip_artifact_upload != 'true' }}

Expand Down Expand Up @@ -492,37 +488,32 @@ jobs:
id: fetch_sdist
uses: actions/download-artifact@v4
with:
name: ${{ needs.build_sdist.outputs.artifact_name }}
name: ${{ needs.python_sdist.outputs.artifact_name }}

- name: build/test wheels
id: build
uses: pypa/[email protected]
env:
CIBW_BUILD: ${{ matrix.spec }}
CIBW_PRERELEASE_PYTHONS: 'True'
CIBW_TEST_REQUIRES: pytest setuptools
CIBW_TEST_COMMAND: 'python -m pytest {package}/src/c'
# FIXME: /testing takes ~45min on Windows and has some failures...
# CIBW_TEST_COMMAND='python -m pytest {package}/src/c {project}/testing'
run: |
set -eux

mkdir cffi

tar zxf cffi*.tar.gz/cffi*.tar.gz --strip-components=1 -C cffi

python -m pip install --upgrade pip
pip install "${{ matrix.cibw_version || 'cibuildwheel'}}"
python -m cibuildwheel --output-dir dist cffi

echo "artifact_name=$(ls ./dist/)" >> "$GITHUB_OUTPUT"
with:
package-dir: >-
${{ steps.fetch_sdist.outputs.download-path
}}/${{ needs.python_sdist.outputs.artifact_name }}

shell: bash
- name: determine built wheel filename
id: built-artifact-lookup
run: echo "artifact_name=$(ls ./wheelhouse/)" >> "${GITHUB_OUTPUT}"
shell: bash -eEuxo pipefail {0}

- name: upload artifacts
uses: actions/upload-artifact@v4
with:
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 }}
if-no-files-found: error
if: ${{ env.skip_artifact_upload != 'true' }}

Expand Down
Loading