Skip to content

Conversation

@JeanChristopheMorinPerso
Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso commented Oct 11, 2021

Publish wheels and sdist (source code only) in PyPI.

Fixes #988 .

The method I used is simple. I added a job in the python-package workflow that will only run when the GitHub Action is triggered by tag pushed in the repo.

That's the simplest option. But it has one major downside. The artifacts will be regenerated for the release. This is a problem because there could be some amount of time between the last PR is merged and the tag creation and during this time some things might have changed (transitive dependencies, GitHub Actions environment, etc). On Linux it isn't much a problem as wheels are generated in docker images, but on macOS and Windows it's another story.

A safer approach would be to have the publish job be in its own workflow. This workflow would fetch the wheels from the last successful build (or a specific build if we make the workflow that can be manually triggered with custom inputs). This is the approach the the Java bindings is using, with the difference that the publish workflow is triggered by a release creation, not a tag pushed.

I will be happy to go with later (safer) approach if TSC prefers that.

Notes

  • For this to work, you will need to add a secret named PYPI_API_TOKEN to the repository. The value needs to be a PyPI token created from the user interface (see https://pypi.org/help/#apitoken for instructions). You will need to specify a name when creating the token. I suggest choosing a name that clearly states that it's going to be used by GitHub actions .
  • I left out GPG signing for now, but I'll be more than happy to add that if requested.

@codecov-commenter
Copy link

Codecov Report

Merging #1099 (c05a793) into master (aada787) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1099   +/-   ##
=======================================
  Coverage   82.78%   82.78%           
=======================================
  Files          75       75           
  Lines        3306     3306           
=======================================
  Hits         2737     2737           
  Misses        569      569           
Flag Coverage Δ
unittests 82.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aada787...c05a793. Read the comment docs.

matrix:
os: [ubuntu-latest, windows-latest, macos-latest]
python-version: [2.7, 3.7, 3.8, 3.9]
python-version: ['2.7', '3.7', '3.8', '3.9']
Copy link
Member Author

Choose a reason for hiding this comment

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

I took the opportunity to use strings here because without quotes these were floats and a future 3.10 would have been interpreted as 3.1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch!

@jminor
Copy link
Collaborator

jminor commented Oct 12, 2021

This is amazing. Thanks so much for setting this up @JeanChristopheMorinPerso !

I don't fully understand the risks of the less safe option. Is the risk that the released package could be broken due to drift outside the OTIO source code between the two builds? The time difference is likely to be very small, since we often land a few changes right before a release milestone. Is there any security risk in there too?

When we have done pypi package releases in the past, we did them to the test pypi index first, verified them, then pushed to the official one. Is there a similar step here where we can vet the final package before releasing it to the world? Perhaps that would come included in the safer approach you mentioned.

@ssteinbach
Copy link
Collaborator

This is awesome!

@jminor I think the 'verification' is less important here because in a way, the GitHub action itself is doing the verification -- the tests are running and passing and so on, using the wheels that will be uploaded to pypi, right?

@ssteinbach
Copy link
Collaborator

ssteinbach commented Oct 12, 2021

It looks like it builds an sdist separately - that is for platforms that don't have compatible wheels, correct?

@ssteinbach ssteinbach added this to the Public Beta 14 milestone Oct 12, 2021
@JeanChristopheMorinPerso
Copy link
Member Author

It looks like it builds an sdist separately - that is for platforms that don't have compatible wheels, correct?

Exactly. It's also to be a good citizen for systems consuming sdists (for example Linux distros tend (at least some of the are) to repackage popular PyPI packages and they normally use the sdists).

@ssteinbach
Copy link
Collaborator

Ok, I really like this. Thanks so much for submitting it! If you have a little time, do you think you could make one (temporary) change, so that we can test this out?

In this doc: https://github.com/marketplace/actions/pypi-publish#advanced-release-management

They note a way of having the action upload to the test server on every commit rather than just to the main one on publish. Could we turn that on for this PR so that we could see the result? Then turn it back off before landing it with the on-release-tag-only switch for actual usage?

If that seems feasible we can coordinate about setting up the tokens and so on. What do you think?

@ssteinbach
Copy link
Collaborator

Alternatively, it might just make sense to publish packages from commits on master to the test pypi. Is that commonly done?

@JeanChristopheMorinPerso
Copy link
Member Author

I don't fully understand the risks of the less safe option. Is the risk that the released package could be broken due to drift outside the OTIO source code between the two builds?

Exactly this yes. On Windows and macOS the compiler could be updated, etc.

Is there any security risk in there too?

I don't believe so.

I think the 'verification' is less important here because in a way, the GitHub action itself is doing the verification -- the tests are running and passing and so on, using the wheels that will be uploaded to pypi, right?

Not yet unfortunately. We don't run the tests against the produced wheels. To run the tests, we run pip install .[dev] and then we run the tests. This actually creates a wheel and it installs it but this wheel it not the one we upload to PyPI.

When we have done pypi package releases in the past, we did them to the test pypi index first, verified them, then pushed to the official one. Is there a similar step here where we can vet the final package before releasing it to the world? Perhaps that would come included in the safer approach you mentioned.

The "safer" approach is similar to what you described indeed.

@JeanChristopheMorinPerso
Copy link
Member Author

@ssteinbach I can certainly set the URL to the test PyPI. Just let me know once the credentials are setup in the repo and I'll do the change.

Alternatively, it might just make sense to publish packages from commits on master to the test pypi. Is that commonly done?

As far as I know it's uncommon. Though what we could have is a manual action where you specify a build ID (https://github.blog/changelog/2020-07-06-github-actions-manual-triggers-with-workflow_dispatch/) and it would take care of publishing in test.pipy.org or something similar.

@JeanChristopheMorinPerso
Copy link
Member Author

I will close this and re-open a new pull request later with new changes.

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.

PyPI Publish github action

4 participants