chore(pypi): publish `git-cliff` on PyPI #158

radusuciu · 2023-04-21T02:31:58Z

Description

This PR updates the CI/CD for packaging git-cliff for PyPI.

Motivation and Context

Closes #150

How Has This Been Tested?

At time of writing this has not been tested.

Screenshots / Logs (if applicable)

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other: packaging

Checklist:

My code follows the code style of this project.
I have updated the documentation accordingly.
I have formatted the code with rustfmt.
I checked the lints with clippy.
I have added tests to cover my changes.
All new and existing tests passed.

radusuciu · 2023-04-21T02:36:12Z

@orhun Took a crack at this mostly following the example of typos which you linked in the related issue, modifying the approach to conform to the existing style in the workflow. Please let me know if you've spotted an error or would like to see any changes.

I've set this as a draft since this was an initial effort - I haven't really reviewed everything.

codecov-commenter · 2023-04-21T02:39:29Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (4d5683f) 44.01% compared to head (8cf249c) 44.01%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #158   +/-   ##
=======================================
  Coverage   44.01%   44.01%           
=======================================
  Files          12       12           
  Lines         584      584           
=======================================
  Hits          257      257           
  Misses        327      327

Flag	Coverage Δ
unit-tests	`44.01% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.github/workflows/cd.yml

orhun · 2023-05-03T13:31:14Z

Hey @radusuciu, thank you for the implementation! I just had a couple of comments. After those are resolved, we can go ahead to update the documentation and we're pretty much ready. Overall looks great, good job! 💖

Sorry for the late response, I have been busy with other projects and the recent version (1.2.0) of git-cliff. But this is something that I really want to incorporate in the next release for sure!

radusuciu · 2023-05-03T18:47:37Z

Thanks for feedback/corrections! I'll have some time to take a look and incorporate your suggestions this weekend most likely.

orhun · 2023-05-03T18:50:16Z

Great, looking forward to it! For testing purposes on another repository, is there a dry-run option available? Otherwise we can simply create RC releases I think.

radusuciu · 2023-05-03T18:52:25Z

One more general question I had: in typos PyPI packaging is handled in a separate workflow yaml. For this PR I integrated it directly into cd.yaml since that seemed to me to be more appropriate given how the workflow is currently structured and I thought would lead to the least amount of duplication. Do you agree with that decision or would you prefer refactoring to a separate file?

radusuciu · 2023-05-03T18:53:30Z

Great, looking forward to it! For testing purposes on another repository, is there a dry-run option available? Otherwise we can simply create RC releases I think.

Yes! I think we should be able to use TestPyPI: https://packaging.python.org/en/latest/guides/using-testpypi/.. which I guess isn't fully "dry" in the sense I think you mean, but might be better

orhun · 2023-05-03T18:54:00Z

One more general question I had: in typos PyPI packaging is handled in a separate workflow yaml. For this PR I integrated it directly into cd.yaml since that seemed to me to be more appropriate given how the workflow is currently structured and I thought would lead to the least amount of duplication. Do you agree with that decision or would you prefer refactoring to a separate file?

I think it is good this way. We have the NPM packaging in the same structure as well.

orhun · 2023-05-03T18:55:01Z

Great, looking forward to it! For testing purposes on another repository, is there a dry-run option available? Otherwise we can simply create RC releases I think.

Yes! I think we should be able to use TestPyPI: packaging.python.org/en/latest/guides/using-testpypi.. which I guess isn't fully "dry" in the sense I think you mean, but might be better

Oh, nice. It is definitely better than a dry run.

radusuciu · 2023-05-18T03:47:46Z

Replied to some comments, there are two items to review still. I think we need to test it out against TestPyPI next

orhun · 2023-05-18T18:55:16Z

Looks good for testing now, how do we proceed?

radusuciu · 2023-05-18T19:22:36Z

You'll want to make an account with PyPI and TestPyPI -- they are separate, and generate API tokens that you can set in the repository secrets under the keys PYPI_API_TOKEN and TESTPYPI_API_TOKEN.

To roll it out, I can think of two options:

Option 1

Configure workflow to use TestPyPI and TESTPYPI_API_TOKEN (I need to look this up, but it should be an argument or envvar that is set on the maturin upload action)
Run workflow
Verify that everything works
Re-configure workflow to use PyPI and PYPI_API_TOKEN

Option 2
We tweak the workflow to use either TestPyPI or PyPI depending on the value of another repository variable eg. USE_TEST_PYPI. This is cleaner because it doesn't require editing the workflow to switch between the test and real package index.

If it doesn't work first shot, let's do some testing locally since it'll be faster to iterate than via Actions.

radusuciu · 2023-05-18T21:33:37Z

Seems the maturin upload command can be pointed to either PyPI or TestPyPI by adding an --repository <REPOSITORY> argument, with valid values being pypi or testpipy. Alternatively, the same values will be taken from the MATURIN_REPOSITORY environment variable. So maybe something like this:

publish-pypi:
  name: Publish PyPI packaage
  runs-on: ubuntu-22.04
  needs: [pypi-sdist, publish-binaries]
  steps:
    - uses: actions/download-artifact@v3
      with:
        name: wheels
    - name: Publish to PyPI
      uses: PyO3/maturin-action@v1
      env:
        MATURIN_PYPI_TOKEN: ${{ secrets.PYPI_API_TOKEN }}
        MATURIN_REPOSITORY: ${{ vars.USE_TEST_PYPI == 'true' && 'testpypi' || 'pypi' }}
      with:
        command: upload
        args: --skip-existing *

Also, just noted a spelling mistake: "packaage"

I haven't used vars in a workflow before, and according to the docs, they are still a beta feature, so we can use secrets if you prefer.

orhun · 2023-05-19T21:30:48Z

You'll want to make an account with PyPI and TestPyPI -- they are separate, and generate API tokens that you can set in the repository secrets under the keys PYPI_API_TOKEN and TESTPYPI_API_TOKEN.

Just created the accounts and added the API keys as secrets to the repository:

To roll it out, I can think of two options:

Yeah, I agree that the 2nd option is cleaner and let's go with that!

Seems the maturin upload command can be pointed to either PyPI or TestPyPI by adding an --repository <REPOSITORY> argument, with valid values being pypi or testpipy.

In that case, we need to use PYPI_API_TOKEN or TESTPYPI_API_TOKEN respectfully as well, right?

Alternatively, the same values will be taken from the MATURIN_REPOSITORY environment variable. So maybe something like this:

Looks good, except we need to add a check for using the test repository token if needed I think.

I haven't used vars in a workflow before, and according to the docs, they are still a beta feature, so we can use secrets if you prefer.

I haven't used it before as well, but looks convenient, let's give it a shot!

radusuciu · 2023-05-19T22:00:24Z

In that case, we need to use PYPI_API_TOKEN or TESTPYPI_API_TOKEN respectfully as well, right?

Yes, you're right, like so:

      env:
        MATURIN_PYPI_TOKEN: ${{ vars.USE_TESTPYPI == 'true' && secrets.TESTPYPI_API_TOKEN || secrets.PYPI_API_TOKEN }}
        MATURIN_REPOSITORY: ${{ vars.USE_TESTPYPI == 'true' && 'testpypi' || 'pypi' }}

note I also suggest renaming the USE_TEST_PYPI repository variable to USE_TESTPYPI for consistency/correctness.

Also tweaked verbiage a little

fixes mistake in use of endsWith and startsWith expression syntax. Thanks https://rhysd.github.io/actionlint/!

radusuciu · 2023-05-20T15:54:21Z

Just want to note that when I tested locally just now the build command yields an error.

$ maturin build --manifest-path git-cliff/Cargo.toml --release --out dist
📦 Including license file "/home/radu/github/git-cliff/LICENSE"
💥 maturin failed
  Caused by: Found a directory with the module name (git-cliff) next to Cargo.toml, which indicates a mixed python/rust project, but the directory didn't contain an __init__.py file.

I think this will happen in the context of the action too, but I have a fix in mind.

Changes: - fixes an error with maturin build - makes sure maturin commands are run in directory containing pyproject.toml - manifest path is now specified in pyproject.toml - sdist is now created as a part of the build command

radusuciu · 2023-05-20T20:24:09Z

I think we're good to go for real now, so long as all repository secrets and variables are set as discussed. Remaining questions:

can you run the workflow normally in it's current form without messing up other deployments?
will the version of the python project be correct? maturin should pull it from cargo.toml before deployment, but I saw that the version updates are handled through release.sh. I think it should be called but I didn't look at it in depth.
will maturin run correctly in the context of the action? It runs fine locally now and produced an installable wheel

orhun · 2023-05-20T20:35:42Z

Alright, I think we're ready for a test run so long as all of the repository variables/secrets are set as discussed above and defined in cd.yaml. Let me know if you'd want me to squash commits.

Cool! I will squash the commits on merge so there is no need :3

Also, will running this workflow manually cause any issues or side-effects? We're good on PyPI thanks to the test repo but not sure about other bits..
can you run the workflow normally in it's current form without messing up other deployments?

cd.yml affects the other releases so I was thinking of releasing a "rc" release for testing this. Wouldn't that work?

will the version of the python project be correct? maturin should pull it from cargo.toml before deployment, but I saw that the version updates are handled through release.sh. I think it should be called but I didn't look at it in depth.

If maturin uses the version from Cargo.toml, that's perfectly fine. I don't see a version set anywhere in the workflow file so we should be good.

will maturin run correctly in the context of the action? It runs fine locally now and produced an installable wheel

I guess we will see 🐻

I will take a look at the code and add a review. But before actually testing this with a new release, I would like to release 1.2.1 first for the pending changes in main. Let's have a clean slate for PyPI :3

From the semver standpoint, would you say we need to bump minor for these changes since there will be a new package?

radusuciu · 2023-05-20T21:51:25Z

cd.yml affects the other releases so I was thinking of releasing a "rc" release for testing this. Wouldn't that work?

That should work with the downside that you're creating release candidates for something that isn't a core feature -- but because they're rc's it should be fine and not annoying to end-users? Maybe we're splitting hairs here lol, but here are some alternatives I can think of:

split off this workflow file and attempt to avoid code duplication through use of reusable workflows.
split off workflow file into separate repository that only exists to test the workflow with TestPyPi... maybe can even do this with my fork
maybe try using act locally? Since I think we just need to see that this workflow works with TestPyPi, you can temporarily edit (or copy) cd.yaml to remove cargo and npm publishing steps

I will take a look at the code and add a review. But before actually testing this with a new release, I would like to release 1.2.1 first for the pending changes in main. Let's have a clean slate for PyPI :3

Sounds good! Now that you mention it, it should be possible to test this workflow without consequence at least once with the release of 1.2.1. If you merge this PR first, the repository variable USE_TESTPYPI is true and TESTPYPI_API_TOKEN is set, and then you release 1.2.1, we should see a git-cliff==1.2.1 on TestPyPi - which will not pollute the eventual PyPi release.

From the semver standpoint, would you say we need to bump minor for these changes since there will be a new package?

I would agree with that!

radusuciu · 2023-05-30T16:19:46Z

Hi @orhun, after a lot of back and forth, mostly dealing with issues related to paths, I've got a successful run of a pared down action: https://github.com/radusuciu/git-cliff/actions/runs/5123788390. This is my effort on bullet point 2 above which I thought was the simplest way forward. I did also try using act, but I couldn't get it to work even with the full 60GB github runner images. I think we do not need to split up the workflow.

Here's the package on TestPyPI: https://test.pypi.org/project/test-pypi-deploy-git-cliff/

I think we are ready for RC

orhun

Sorry for the delay and thank you so much for the implementation/testing! 🐻

radusuciu · 2023-08-16T18:43:51Z

No worries!

Please feel free to tag me in if there's any issues

orhun · 2023-08-19T12:32:53Z

Hey @radusuciu, I created the RC release today and there was a build error: https://github.com/orhun/git-cliff/actions/runs/5911302728/job/16033716352

Can you look into it? 🐻

edit: maybe we can disable win32-arm64 releases for PyPI...

#158 (comment)

orhun · 2023-08-20T20:17:24Z

Just disabled win32-arm64 and released another RC, it worked this time! 🥳

https://test.pypi.org/project/git-cliff/

$ pip install -i https://test.pypi.org/simple/ git-cliff

radusuciu · 2023-08-22T17:07:36Z

Hey @radusuciu, I created the RC release today and there was a build error: https://github.com/orhun/git-cliff/actions/runs/5911302728/job/16033716352

Can you look into it? 🐻

edit: maybe we can disable win32-arm64 releases for PyPI...

Hmm, most of the references I've come across for this point towards issues installing ring due to something musl related but that doesn't make sense here.

I was just travelling but jotting down some links for later (may or may not be relevant, I'll look into it more)

chore(pypi): First shot at PyPi publishing

a5a2413

radusuciu requested a review from orhun as a code owner April 21, 2023 02:31

radusuciu marked this pull request as draft April 21, 2023 02:32

docs(pypi): Draft docs for install from PyPI

cf5e59d