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.

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

Autobuild python wheels on release tags and create github releases with them #969

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Autobuild python wheels on release tags and create github releases with them #969

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8a9d42
add automatic python wheel generation, fixes #https://github.com/stev…
sloev Jan 16, 2025
4c75577
change from GITHUB_TOKEN to REPO_TOKEN because github won't allow sec…
sloev Jan 16, 2025
a4838cf
use one file for all build and release
sloev Jan 16, 2025
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
17 changes: 17 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,23 @@ jobs:
run: |
cmake --build build --target test
- name: Make release_artifacts folder
run: mkdir release_artifacts

- name: Add prefix to python_wheel artifact
run: |
for file in ./build/pypangolin-*.whl; do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you looping over multiple wheel files? The build process should only create a single wheel file for the target platform.

Copy link
Author

Choose a reason for hiding this comment

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

Cuz i am unknowing of how to rename one single file with unknown name :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, if you can do ls ./build/pypangolin-*.whl and get a result, you can also do something like mv ./build/pypangolin-*.whl /tmp/ and will find the file there. Or do you mean something other? In any case, see my other comments, you won't need to move that wheel file around.

mv $file ./release_artifacts/${{matrix.os}}_shared_libs_${{matrix.shared_libs}}.$(basename $file);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this required? The wheel file already exists at build/pypangolin-*.whl.

Copy link
Author

Choose a reason for hiding this comment

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

I experienced that the shared libs on/off wheels ended up with the same name. So renaming them to unique names is a requirement for the later download and merge artifacts step, else they will just overwrite eachother

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean .so files within the wheel file .whl? That does not matter. You only need to upload the wheel file and that one usually has a unique name encoded by the OS and architecture. Can you provide a concrete example where this happens?

Copy link
Author

Choose a reason for hiding this comment

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

I mean that for example the wheels for sharedlibs on and sharedlibs on for ubuntu 22.04 will have the same name. The "shared libs" doesnt affect the name of the wheel. So in order to keep both the shared libs on/off versions i add the prefix containing os and sharedlibs boolean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. You are right. But we would only upload one of the two variants, shared or static, to the release page.

done
- name: 'Upload release_artifacts'
uses: actions/upload-artifact@v4
with:
name: "release_artifacts_${{matrix.os}}_${{matrix.shared_libs}}"
path: release_artifacts/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not directly use the path build/pypangolin-*.whl here? This would save you from the renaming above.

You should also add a if-no-files-found: error here to make sure that the CI fails if there is no file.

retention-days: 2


emscripten:
runs-on: ubuntu-22.04

Expand Down
30 changes: 30 additions & 0 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
name: release

on:
workflow_run:
workflows: [build]
types: [completed]
branches: [master]

jobs:
prepare_artifacts:
# Only run the deployment if the build workflow succeeded.
if: ${{ github.event.workflow_run.conclusion == 'success'}} && contains(github.ref, '/tags/v')
runs-on: ubuntu-22.04

steps:
- name: Download build artifact from triggered workflow
uses: dawidd6/action-download-artifact@v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use the more official actions/download-artifact here? I think this would be better as it from the same origin that also maintains actions/upload-artifact and I simply expect that they will always work well together.

Copy link
Author

Choose a reason for hiding this comment

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

Should be easily swapped. No reason from my side

with:
run_id: ${{ github.event.workflow_run.id }}
name: ./release_artifacts/*
name_is_regexp: true
path: ./release_artifacts
- name: Upload release
uses: softprops/action-gh-release@v2
Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably does not make a huge difference, but I used ncipollo/release-action before, which is as simple as:

    - uses: ncipollo/release-action@v1
      with:
        artifacts: "release.tar.gz,foo/*.txt"

so you do not have to set the other parameters or care about the token.

with:
name: ${{ github.ref_name }}
tag_name: ${{ github.ref_name }}
files: release_artifacts/*
token: ${{ secrets.GITHUB_TOKEN }}
generate_release_notes: true