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
Automatically build and upload PyPI packages on release #69
Conversation
|
If you're not interested in this, let me know and I can just close the PRs. |
acdd7dc
to
2c8b988
Compare
|
@dralley I'm planning to attempt to set this up in the next few days, sorry about the long silence. |
f72a8ed
to
e24a1aa
Compare
|
@lukash I have added building documentation and running the C tests with check to the CI |
be1cc67
to
7a438e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dralley, sorry about keeping you waiting. I've created an account on PyPI for the team: rpm-software-management. You can add us as maintainers 🙂
Some comments below, in addition, could you please split this into three commits:
- python build CI workflow
- python release workflow
- the rest of the updates along with a bit of description (possibly split it into more commits if there are more unrelated fixes, but no need to go too fine-grained if you don't want to)
Thanks!
.github/workflows/ci.yml
Outdated
| on: [push, pull_request] | ||
|
|
||
| jobs: | ||
| test_standard_build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you drop this job, testing rpm build is part of our standard CI workflow now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
.github/workflows/ci.yml
Outdated
| make tests | ||
| make test | ||
|
|
||
| test_python_build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update this job to be identical to the createrepo_c one, except for necessary differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
.github/workflows/release.yml
Outdated
| types: [created] | ||
|
|
||
| jobs: | ||
| deploy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to test this job without making a release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could trigger it on push and tell it to use the credentials for the "test PyPI" instance rather than the production one.
I'm not really sure it's necessary though, we can check everything up to point of the upload pretty easily. https://github.com/rpm-software-management/createrepo_c/pull/256/checks?check_run_id=2099124911
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.github/workflows/release.yml
Outdated
| - name: Publish wheels to PyPI | ||
| env: | ||
| TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | ||
| TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use the password here, or could an API token be used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little awkward to do but it looks like it's possible
|
One more thing, could you name the workflows |
|
@lukash This is great! Yes, I can update the PR. |
2a80d70
to
6c0f962
Compare
5ca1905
to
8a54129
Compare
877f5ef
to
a6ff43f
Compare
| run: | | ||
| sudo dnf -y install dnf-plugins-core | ||
| sudo dnf -y builddep libcomps.spec | ||
| pip install --upgrade pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this stage, nitpicking 🙂 but is the upgrade call necessary? (Just trying to keep the variance between workflows minimal, we don't have this in createrepo_c, either it's a different case or we can be uniform?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary on Fedora, it is necessary on images with older versions of pip. Without it you need to manually install "skbuild" because pip doesn't know how to read pyproject.toml
With that said, if you would prefer to either drop it or do the manual install, either of those are fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the workflow is running on the latest fedora, so it's always going to work here IIUC? (if so, I'd prefer to drop it)
.github/workflows/release-python.yml
Outdated
|
|
||
| - name: Install and Test Python universal binary wheel package | ||
| run: | | ||
| pip install --user dist/libcomps-0.1.15.post1-cp39-cp39-manylinux2014_x86_64.whl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded version of the .whl file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have to manually specify the versions of Python to build wheels for, I figured this was OK, because if Fedora bumps their Python version then we need to edit the file anyway.
However, it wouldn't be ideal to only discover this when a new release is attempted. Maybe we should freeze the container image to Fedora 33?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it on fedora:latest.
But apart from the python version (for which I don't have a solution), there's also the libcomps version itself (0.1.15), that's the main problem I was pointing out 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. I was looking at the cp39-cp39 section because I spent half an hour trying to get glob patterns to work.
But a glob pattern for the libcomps version would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dralley any plans to address this? It's the main outstanding issue with the PR, the rest is mostly nitpicks...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've been really busy. I just pushed the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, no worries. But can you squash the fix with the commit that it fixes?
Also, looking at this, another question: It only releases the binary package for the python listed here, that is, python 3.9 (inferring from cp39), correct? So this won't work for distros with any other python than what we decide to do the release for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it only tests that package, but it releases all of them.
(It's the only one it can test, because you can't use a wheel built for 3.7 with Python 3.9, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, sorry 🙂
| @@ -0,0 +1,7 @@ | |||
| graft libcomps* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is adding this file intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it ensures that source packages can be built properly, because otherwise not all the necessary files are included by default. Granted that source packages are only a "backup" if for some reason there isn't a wheel available that is compatible with your platform.
There are attempts in the Python ecosystem to get rid of all the separate configuration files that are needed (setup.py, manifest.in, etc.) and standardize on pyproject.toml, but it will be a while until they can be reliably avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit annoying, I don't have thorough knowledge of the python build system, but since this only needs to be added now, I assume it wasn't needed until now and thus is used only in this special case. That makes it quite hard to maintain for someone who doesn't know all the details.
|
Ah, also: I assume this is still WIP because of the |
|
@lukash Apologies for the spam! It might be configurable, because we're also using Github Actions in our org and I don't get spammed by everyone else's failed PR runs. Re: https://packaging.python.org/specifications/pypirc/#using-a-pypi-token |
I didn't mean the spam of the failed workflow runs, just the spam of repeatedly force-pushing to the branch. I don't want to turn off those notifications, I use them. IMO people should only create PRs when the code is ready to be reviewed and same for updating PRs. If you push your WIPs, besides spamming it's actually not clear when your PR is ready for a review (I don't want to review WIP unless explicitly asked for feedback on it). Just my PoV, not sure how others see this.
Oh, that's actually not terrible, so the |
I understand. In this case, I was just testing the release CI (temporarily changed the trigger to "pull_request" in order to do so). Both PRs are reviewable at the present moment. |
Manylinux wheels include libython statically, so trying to dynamically link the lib will cause the build to fail.
|
@dralley I just force-pushed your master 😬 hope it's ok. I made a few cosmetic adjustments, as well as renaming the PYPI_API_TOKEN secret and changed the Python CI to run on pull_request only (same as I did on createrepo_c). Let's see how it works, we'll need to keep an eye on the release workflow once we actually do a release, libcomps is released very infrequently. |
Using Github Actions
This will build and upload both source and binary Python packages automatically every time a new release is published. The binary packages should enable Python users to use libcomps across any (linux) platform without needing the compile-time dependencies needed by Python source distributions.
We (current maintainers of the PyPI libcomps package) will need to add your PyPI account as a maintainer, and then you will need to add your credentials to the github repo secrets. Afterwards it should be automated and we will ensure it continues working if any problems arise in the future.
Here's a snapshot of using the binary package built on Centos 7 on a clean Debian VM with pip 20 (older versions can't use the binary package)