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

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

Conversation

sloev
Copy link

@sloev sloev commented Jan 16, 2025

issues touched by this:

i had to rename the GITHUB_TOKEN to REPO_TOKEN due to github policies, see attached screenshot
Screenshot from 2025-01-16 12-25-42

@sloev
Copy link
Author

sloev commented Jan 16, 2025

@sloev sloev changed the title [WIP] autobuild python wheels on release tags and create github releases with them Autobuild python wheels on release tags and create github releases with them Jan 16, 2025
Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

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

Thanks for this. This will be quite useful.

It already looks like a standard pipeline for artifact handling. But I would like to see the following changes:

  1. Simply use build/pypangolin-*.whl as the path to the wheel file instead of moving the files around. Each build will only generate a single wheel file and, therefore, the build/pypangolin-*.whl pattern will only match one file.
  2. I suggest using ncipollo/release-action to create the release with the artifacts since it requires less configuration and, above all, does not require a token. I cannot add or update tokens. So this will be a very string limitation that we can easily avoid with ncipollo/release-action.
  3. Please squash your commits together since they are changing the same things multiple times back and forth.


- 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.

- name: Add prefix to python_wheel artifact
run: |
for file in ./build/pypangolin-*.whl; do
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.

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.


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

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.

@sloev
Copy link
Author

sloev commented Jan 17, 2025

@christian-rauch thanks for very good feedback. However i am unable to follow this through due to stress.

So this pr is more seen as my limited try at contributing to a solution.

Kind regards

@christian-rauch
Copy link
Collaborator

@christian-rauch thanks for very good feedback. However i am unable to follow this through due to stress.

So this pr is more seen as my limited try at contributing to a solution.

Thanks so far. I will have a look at this and will adjust myself later, if you cannot work on this.

@christian-rauch christian-rauch marked this pull request as draft January 17, 2025 15:30
@christian-rauch
Copy link
Collaborator

@christian-rauch check it out, it builds!!!! https://github.com/sloev/Pangolin/releases/tag/v1.0.2

Did you actually try to install and use your wheel files? pip will complain that the file name is not correct: ERROR: ubuntu has an invalid wheel, ubuntu has an invalid wheel, .dist-info directory 'pypangolin-0.9.2.dist-info' does not start with 'ubuntu'. Also, the pypangolin.cpython-312-x86_64-linux-gnu.so included in the wheel file links against libOpenNI.so.0, which is not provided with the wheel file.

@sloev
Copy link
Author

sloev commented Jan 17, 2025

I tried to install the ones on my other repo. And it worked great on ubuntu24.04 migjt be the renaming that doesnt work

@sloev
Copy link
Author

sloev commented Jan 17, 2025

As i mention in the readme you have to install openni firat, check here for the example i succesfully ran:
sloev/pypangolin-autobuild#1

@christian-rauch
Copy link
Collaborator

As i mention in the readme you have to install openni firat, check here for the example i succesfully ran: sloev/pypangolin-autobuild#1

Which README? There is no such change in this (your) PR. In any case, it is not just OpenNI that you have to install. Since the CI builds with all dependencies, the resulting binaries also have all the libraries linked. A user of the wheel files therefore has to install all these dependencies.

Based on this PR, I created #970. That is the cleaned up version with the fixes I proposed in your PR. The resulting wheel files still have the dependant libraries not included, hence they cannot be used out-of-the-box.

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

Successfully merging this pull request may close these issues.

2 participants