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

Add github action for packaging and publishing python packages to PyPI (#1592) #2126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vlcek
Copy link

@vlcek vlcek commented Dec 18, 2024

Hello,

this PR relates to #1592. I tried to create a gits of the packaging and publish workflows to PyPI so people can more easily use Nickel withing Python code.

What I've managed to do here:

  • multiplatform support
    • Linux via manylinux, both aarch64 and x86_64 and glibc and musl support
    • Windows
    • MacOS - both aarch64 and x86_64
  • uploading to PyPI via trusted publishers
  • support for creating and uploading build attestations via the official pypa GH action
  • basic support for "test uploads" to TestPyPI (pypi deployment for testing package uploads)

Unfortunately, for sake of testing I had to rename pyckel package to py-nickel (and nickel as the python module), because the name on PyPI is already taken. The py-nickel is just a name I picked to progress further. If you like this approach, I can easily change the name to whatever you like.

I found testing github workflows in a forked repository rather hard and my inexperience with GitHub did not helped either so please take this as a draft :) I highly suspect that the workflow will need some adjustments to fit your release process, however, I am not sure if I can help you with this any further :)

Setting up repository on PyPI is quite easy, here is the official guide.
Basically, once the pypi repository is created, you just set up the "trusted publisher" on github and there is no need to set up any credentials - everything is handled via OIDC and trust setup between PyPI and GitHub.

You can check/test the PyPI uploads here https://test.pypi.org/project/py-nickel/

You can see the list of uploaded artifacts here https://test.pypi.org/project/py-nickel/1.9.0/#files
Attestation https://test.pypi.org/project/py-nickel/1.9.0/#py_nickel-1.9.0-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl (some files don't have attestations, that's only because of my experiments with uploads via maturin).

Example of a slightly different workflow version here - I needed different triggers to check if the builds and publishing work.

@yannham
Copy link
Member

yannham commented Dec 18, 2024

Hi,

Thanks a lot for tackling this! This is great. We'll give it a look soon. Just to make sure I understand, the package that you published on TestPyPI has been done through this workflow, right?

I don't have a strong opinion about the name - I find pyckel nice but it's fine if we pick something else. However, although it's been taken on TestPyPI, it's not taken on the actual PyPI, if I understand correctly. Do you think we could keep it as pyckel once all the tests are satisfactory, or it'd be a pain to change back and forth? I don't know too much about Python convention, so if there's a official or unofficial naming convention for language bindings in PyPI, we could also just use that, to make it more discoverable.

Copy link
Contributor

github-actions bot commented Dec 18, 2024

@vlcek
Copy link
Author

vlcek commented Dec 18, 2024

Just to make sure I understand, the package that you published on TestPyPI has been done through this workflow, right?

Yes, I wanted to make sure the whole workflow works as expected. You can check, for example, this job to see some details https://github.com/vlcek/nickel/actions/runs/12383038938/job/34565207353
It took me few iterations to get it working with attestations so that's the reason why you can see some differences between uploads for different platforms.

However, although it's been taken on TestPyPI, it's not taken on the actual PyPI, if I understand correctly.

Yes, I made change just as a workaround so I can test the recommended way of package publishing on TestPyPI.

From what I understand, the packages/projects on TestPiPY are regularly wiped and somewhere in some FAQ I read you can ask administrators to "hand it over" if you want to claim the name.

Just a word of caution. pyckel is quite similar to pickle which is part of the stdlib and it's a serialization library.
There is also pyckle on PyPI and that quite confused me when I started with this PR. At first I though pyckel was some play on words using the pickle library.
I am not native English speaker so maybe that's part of the problem (it took me a while before I noticed different letter order), however, maybe it will cause similar confusion to other people looking for Nickel python package.

Renaming packages later is... problematic and perhaps a different name would make python bindings on PyPI more discoverable. I really like Nickel and I hope it will gain more adoption and "infiltrating" Python ecosystem seems to me like a great help :)
Nonetheless, the choice is of course yours :) I don't mind changing the names as necessary to make official packages on PyPI a reality.

I don't know too much about Python convention, so if there's a official or unofficial naming convention for language bindings in PyPI, we could also just use that, to make it more discoverable.

I am no expert either. From what I read there is no strict convention. From what I saw, packages containing python bindings usually have py- prefix or -py postfix to the project name and tend to skip the prefix/postfix when importing the module (the same thing I've done).
Some Rust projects use -rs postfix.
image

@yannham
Copy link
Member

yannham commented Dec 18, 2024

I think the fact that Nickel is implemented in Rust is a bit of an implementation detail for Pythonistas (I don't think it deserves to be in the name). Several config languages just have the language as package name (Dhall or Jsonnet for example), but nickel is unfortunately also already taken. We could use nickel-lang, but the wrapper crate in this repo might become confusing. So maybe py-nickel is the best pick, indeed.

@suimong
Copy link
Contributor

suimong commented Dec 19, 2024

I think it's my earlier attempt on this (shortly after posting the issue) that occupied the pyckel name, then my focus turned away from Python, leaving a mess behind...Sorry about that, will clean it up shortly. Btw I also like pyckel

@suimong
Copy link
Contributor

suimong commented Dec 19, 2024

I deleted the pyckel project from TestPyPI. The name should be available now.

Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Thanks, I'm not very knowledgeable in Python packaging, but this looks good plus you've tested it before.

steps:
- uses: actions/checkout@v4
with:
ref: ${{ github.event_name == 'release' && '' || github.event.inputs.release_tag }}
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, this means: if the first condition evaluates to true, then && evaluates to its second condition, and || to its first condition. Otherwise, && evaluates to false and || to its second condition.

I'm not a GitHub action expert, and given that workflow expressions are limited (well, maybe they should use Nickel :p), that might be the idiomatic way to do things but I find this a bit confusing.

@yannham
Copy link
Member

yannham commented Dec 23, 2024

Regarding the name, while I like pyckel as well, I also agree that it can be confusing with Pkl and other things. In the end a boring but discoverable name is probably better than a clever but confusing one, so I would vote to keep the naming as it is currently in the PR. @vlcek any reason to keep this PR as a draft anymore, or anything else left to do? I would be inclined to merge it as it is

@yannham yannham marked this pull request as ready for review December 23, 2024 15:27
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.

3 participants