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

Make this action play nicely with new gh upload/download artifact actions v4 #199

Closed
alugowski opened this issue Dec 18, 2023 · 7 comments
Closed

Comments

@alugowski
Copy link

alugowski commented Dec 18, 2023

GitHub recently updated the upload-artifact and download-artifact actions to v4. Dependabot is rolling them out.

A major breaking change relevant here is that v4 of upload-artifact no longer allows appending to the artifact. The recommended pattern depends on the old behavior, which now breaks if more than one runner creates artifacts, like when using cibuildwheel to build wheels for multiple platforms.

With v4 actions:

  • The upload fails due to duplicate artifact name. The solution is simple, assign a unique name:
      - uses: actions/upload-artifact@v4
        with:
          name: wheels-${{ matrix.os }}-${{ matrix.cibw_archs }}
          path: wheelhouse/*.whl
  • The download in the pypi-publish step is trickier. The v4 action can download all artifacts, but it will unzip all the files into their own directories. One workaround is to manually flatten this back to a dist/ that pypi-publish expects:
      - uses: actions/download-artifact@v4
        with:
          path: dist_artifacts

      - name: Flatten artifacts to dist/
        run: mkdir dist && find dist_artifacts -type f -exec mv {} dist \;

This seems hacky. Could this action be improved to play better with the new versions of actions/upload-artifact and actions/download-artifact?

@alugowski
Copy link
Author

One option would be to support a recursive scan of dist/, even if to only support one level. Currently the action complains about any subdirectory of dist. A recurse would allow directly chaining the download (like with v3), skipping that flatten step in my example. Then the only change needed to folks' workflows would be to add unique artifact names in the upload step.

@woodruffw
Copy link
Member

FWIW, this restriction is inherited from twine: twine expects to be given one or more files, which this action does by tacking * onto the packages-dir input. The action itself isn't actually aware of nested directories.

@alugowski
Copy link
Author

If using twine directly then tacking on */* works: https://github.com/scikit-build/cmake-python-distributions/pull/438/files

@webknjaz
Copy link
Member

This is already solved in the download action. Not sure if I want extra pattern matching here.

@alugowski
Copy link
Author

This is already solved in the download action. Not sure if I want extra pattern matching here.

Correct. Just to clarify, download-artifact just released v4.1 which includes a merge-multiple switch that does the flattening. Now this is sufficient:

      - uses: actions/download-artifact@v4
        with:
          path: dist
          merge-multiple: true

This is now just a documentation issue, to update the examples so folks know about this non-default switch.

@webknjaz
Copy link
Member

This is now just a documentation issue, to update the examples so folks know about this non-default switch.

So this action doesn't showcase the use of matrices and other complex stuff. Neither does my PyPUG guide.
I suppose, it might be more useful to document it in cibuildwheel: pypa/cibuildwheel#1699.

@alugowski
Copy link
Author

This is now just a documentation issue, to update the examples so folks know about this non-default switch.

So this action doesn't showcase the use of matrices and other complex stuff. Neither does my PyPUG guide. I suppose, it might be more useful to document it in cibuildwheel: pypa/cibuildwheel#1699.

Makes sense. I already have a PR for cibuildwheel docs: pypa/cibuildwheel#1705

@alugowski alugowski closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2023
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

No branches or pull requests

3 participants