Add workflow to publish on PyPI #139
Conversation
|
Tested on Test PyPI with my fork : https://github.com/saimn/pytest-doctestplus/runs/1553716657?check_suite_focus=true |
Anyway we can contact PyPI to reclaim the name or something? Would be nice to have it back for future testing... |
|
It seems not: https://test.pypi.org/help/#file-name-reuse (but we could still use another project, or another name, so even if it's a bit annoying) |
There was a problem hiding this comment.
Thanks! I like the concept and my comments are mostly minor.
CI failure is real but unrelated: AttributeError: 'LocalPath' object has no attribute 'is_file'
But that failure also blocks the actual release, as in we should not release until it is fixed. (See #140)
p.s. Unfortunate about not able to claim the test namespace back but good to know we have a workaround.
.github/workflows/publish.yml
Outdated
| on: | ||
| push: | ||
| tags: | ||
| - '*' |
There was a problem hiding this comment.
Do we ever need to push a tag without releasing?
Do we need to disable this in forks? Can we?
There was a problem hiding this comment.
Do we ever need to push a tag without releasing?
I don't understand ?
Do we need to disable this in forks? Can we?
Forks will not have the secret to upload, and should not push tags on their repo, so think it's fine.
There was a problem hiding this comment.
I don't understand ?
Will it ever be a situation where one would do something like git tag aas-tutorial or something crazy, where we need to push a tag but it is not for release? If not, then it's fine.
There was a problem hiding this comment.
Ah ok, I don't think in a general case. And the build would probably fail, since the package version that would be generated by setuptools_scm is by default not a valid version accepted by PyPI (when it contains the commit hash for dev versions).
There was a problem hiding this comment.
So to make sure the workflow works, I'm thinking that we could run it on push events (not sure if that include merges on master ?), and just skip the upload part if the push is not a tag with:
if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags')
Thoughts?
There was a problem hiding this comment.
Is there a reason why the testing needs to be done for every push to master?
Just to be sure it continues to work in the future, and avoid finding issues when pushing a tag. Running it for pushes/merges on master seems a good solution ? (Or it could also be another build for pull requests)
There was a problem hiding this comment.
How about "push tag only + scheduled cron" instead of blind "push"? And we skip upload on cron.
There was a problem hiding this comment.
That could work as well. I would prefer push because it would run just after a PR is merged and also if you push directly a README/Changelog update just before a release.
There was a problem hiding this comment.
Since you have a strong preference to using push, let's try it your way and we can revisit if needed. 😄
There was a problem hiding this comment.
Ok ;), let's try this.
| run: ls -l dist | ||
|
|
||
| - name: Check long_description | ||
| run: python -m twine check dist/* |
There was a problem hiding this comment.
If you want the publish to not happen if there are warnings, it has to be strict.
| run: python -m twine check dist/* | |
| run: python -m twine check dist/* --strict |
There was a problem hiding this comment.
Currently there is warning about "long description type", not sure if we want to fail for this ? Oh, and I don't see --strict in twine check's help ?
There was a problem hiding this comment.
Hmm I see it here: https://twine.readthedocs.io/en/latest/#twine-check
There was a problem hiding this comment.
Oh, funny:
❯ twine check --help
usage: twine check [-h] dist [dist ...]
positional arguments:
dist The distribution files to check, usually dist/*
optional arguments:
-h, --help show this help message and exit
There was a problem hiding this comment.
Ah... Maybe pypa/twine#715 is not released yet. It is milestoned to 3.3.0 and current release is 3.2.0.
Yes, seems related to some internal changes (pytest-dev/pytest#8122), hopefully this will disappear... |
This comment has been minimized.
This comment has been minimized.
It's still failing today after the commit with your suggestions, so maybe good to report it indeed. |
|
I now get a |
|
Because 6.2.1 does not have the pathlib changes. |
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
|
Ok, this time it works, and skips the upload: https://github.com/astropy/pytest-doctestplus/pull/139/checks?check_run_id=1561218146 |
|
Cool ! And the job ran on master (https://github.com/astropy/pytest-doctestplus/runs/1564407053?check_suite_focus=true). |
No description provided.