-
Notifications
You must be signed in to change notification settings - Fork 9
Modernize gh actions and pyproject #97
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
base: master
Are you sure you want to change the base?
Modernize gh actions and pyproject #97
Conversation
- .githook should not be committed - skosify is installed, no skosify.py required install as developer with pip install -e .[dev]
222ef73 to
3d2dc16
Compare
| Quality of SKOS Vocabularies. Journal on Data Semantics, vol. 3, no. | ||
| 1, pp. 47-73, June, 2014 | ||
| (`PDF <https://seco.cs.aalto.fi/publications/2014/suominen-mader-skosquality.pdf>`_) | ||
| (`PDF <https://seco.cs.aalto.fi/publications/2014/suominen-mader-skosquality.pdf>`__) |
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.
Avoids sphinx WARNING: Duplicate explicit target name: "pdf". [docutils]
- pyproject replaces pytest.ini, setup.py, setup.cfg. MANIFEST.in - hatchling as build backend - hatch-vcs for dynamic versioning based on git tags - hatch as build frontend
The RTD theme currently pins Sphinx to <9.0
3d2dc16 to
a2728fb
Compare
9cefa0f to
d7f5fce
Compare
|
Thanks a lot for this PR @dalito ! Whoa, that's a huge amount of work you have put into this PR! We'll have to look over this and see what to do. As you've surely noticed, it's been a long time since we've done any maintenance work on Skosify. The tool itself has been working fine (for us) so there hasn't been an urgent need to touch it. As a result the codebase has been stagnant. |
I did very similar stuff in several other projects recently so it was mainly adapting not first-time elaboration. The Skosify code required no work (good sign!). Skosify is quite useful so it deserves some fresh air. 😃 |
Hi @dalito, could you add also short, basic instructions for how to use Hatch and tox with Skosify to the README? It would help in testing this, as I haven't used those tools before. |
juhoinkinen
left a comment
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.
Just read this through and dropped some comments and questions :)
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.
Are the different jobs for building and publishing necessary? Running these steps in one job would remove the need for upload-artifact and download-artifact steps.
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.
The main reason is security / isolation. The OpenID connect token is only available in pubishing this way. This is now best practice and also suggested in Python packaging guides.
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.
Ok, to me it feels a bit insecure to also download the distribution to publish from another job, but that is suggested also here https://docs.pypi.org/trusted-publishers/security-model/:
Limit the scope of your publishing job: your publishing job should (ideally) have only two steps:
a. Retrieve the publishable distribution files from a separate build job;
b. Publish the distributions using pypa/gh-action-pypi-publish@release/v1.
By using a separate build job, you keep the number of steps that can access the OIDC token to a bare minimum. This prevents both accidental and malicious disclosure.
For a reference see also this: pypa/gh-action-pypi-publish#324 (comment)
.github/workflows/pypi-publish.yml
Outdated
|
|
||
| - name: Publish package 📦 to PyPI | ||
| if: github.event_name == 'release' | ||
| uses: pypa/gh-action-pypi-publish@ed0c53931b1dc9bd32cbe73a98c7f6766f8a527e |
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.
| uses: pypa/gh-action-pypi-publish@ed0c53931b1dc9bd32cbe73a98c7f6766f8a527e | |
| uses: pypa/gh-action-pypi-publish@ed0c53931b1dc9bd32cbe73a98c7f6766f8a527e # v1.13.0 |
For clarity the version number could be shown as a comment.
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.
Dependabot makes the versions clear in the update-PRs (but would not add/update such version-comment again). I used the current versions from when I made the PR (Dec 25th)
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.
Actually Depedabot does update the version comments, which is surprising but nice, see e.g. here: https://github.com/NatLibFi/Annif/pull/925/changes
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.
Interesting (and quite nice!)
| # GitHub glob matching is limited [1]. So we can't define a pattern matching | ||
| # pep 440 version definition [N!]N(.N)*[{a|b|rc}N][.postN][.devN] | ||
| - 'v[0-9]+.[0-9]+.[0-9]+.?*' | ||
| 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.
Do we want to have this workflow trigger on releases too? Previously there was only the trigger on tags.
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 is triggered on both. Typically I publish to test-pypi on the tag and to pypi by creating a gh release. Note, that below is still the filter to only publish to PyPI for a GitHub-release. if: github.event_name == 'release'
Tell me what you prefer (or change it - committing to the PR is allowed for maintainers).
Co-authored-by: Juho Inkinen <34240031+juhoinkinen@users.noreply.github.com>
|
I added some notes on using tox and how to build locally to README.rst. |
This pull request modernizes the project's Python packaging and CI/CD setup. It brings the project in line with modern Python packaging and CI/CD best practices, and ensures compatibility with current Python versions.
Build system and packaging modernization:
pyproject.toml, adopting Hatch as the build back-end, and removed legacy files likesetup.py, andMANIFEST.in(setup.cfghad to kept for flake8).skosify/__init__.pyto retrieve the version dynamically.tox.inifile for easier local cross-vesrsion testing with tox.Continuous integration and deployment improvements:
python-package.yml) with two new workflows:ci.ymlfor testing (across Python 3.9–3.14) and documentation builds,pypi-publish.ymlfor publishing to PyPI/TestPyPI using trusted publishing and Hatch.Documentation updates:
docs/conf.py) to use modern settings, dynamic version retrievalREADME.rstto reflect the new CI system and supported Python versions.I did not modernize the code or break Python 3.6 compatibility but only removed the end-of life Python versions 3.6-3.8 from CI testing. I kept 3.9 which is also end-of-life but it was the newest version before this PR.