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

[9351] Upload wheel + sdist on PyPI when a tag is created. #1464

Merged
merged 31 commits into from Oct 26, 2020

Conversation

adiroiban
Copy link
Member

@adiroiban adiroiban commented Oct 23, 2020

Scope

Based on the code from #1458

I have moved that code to Twisted repo to use the secrets

This is just to see that wheel + sdist are automatically published on PyPi when a new tag is crated.
Release documentation will be done at https://twistedmatrix.com/trac/ticket/10034

The release fails if the tag name doesn't match the branch version. Versions are PEP440.

The release is not waiting for tests to be green as the pypy3 run can take more than 22 minutes to run and randomly fail...and this will block the release.

The idea is that a tag is manually created by a person only after checking that all tests are green.

Testing process

Step 1: Use PyPI testing and publish on every push to this PR
Step 2: Use PyPI testing and publish when a new tag is created - done for twisted-20.11.0.dev6

All steps were successfully executed. Result for last step at https://github.com/twisted/twisted/runs/1299338123?check_suite_focus=true

Final push - Use PROD PyPI and don't push any tag. We will test this on the first pre-release

Contributor Checklist:

@adiroiban adiroiban changed the title [9351] pypi upload take adiroiban [9351] Upload wheel + sdist on PyPI when a tag is created. Oct 23, 2020
tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@graingert
Copy link
Member

lgtm other than some minor comments - although it's based on code I wrote so you might want to get an approval from someone else too

Comment on lines 141 to 149
# Testing:
# TEST_PYPI_UPLOAD_TOKEN
# repository_url: https://test.pypi.org/legacy/
# Prod secret: PYPI_UPLOAD_TOKEN
if: startsWith(github.ref, 'refs/tags')
uses: pypa/gh-action-pypi-publish@v1.3.1
with:
password: ${{ secrets.TEST_PYPI_UPLOAD_TOKEN }}
repository_url: https://test.pypi.org/legacy/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Testing:
# TEST_PYPI_UPLOAD_TOKEN
# repository_url: https://test.pypi.org/legacy/
# Prod secret: PYPI_UPLOAD_TOKEN
if: startsWith(github.ref, 'refs/tags')
uses: pypa/gh-action-pypi-publish@v1.3.1
with:
password: ${{ secrets.TEST_PYPI_UPLOAD_TOKEN }}
repository_url: https://test.pypi.org/legacy/
if: startsWith(github.ref, 'refs/tags')
uses: pypa/gh-action-pypi-publish@master
with:
password: ${{ secrets.PYPI_UPLOAD_TOKEN }}

:shipit:

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 kept those comments to make it easy to switch to testing in a future branch.

@adiroiban adiroiban marked this pull request as draft October 23, 2020 14:50
Copy link
Member

@graingert graingert left a comment

Choose a reason for hiding this comment

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

needs the fetch-depth hack on lint.yaml

Copy link
Member

@graingert graingert left a comment

Choose a reason for hiding this comment

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

lgtm, I think this should get another reviewer though @exarkun ?

@adiroiban
Copy link
Member Author

I am going to merge this as I hope it will reduce the load generated by wheel builders...and I think that this is on the right track.

@adiroiban adiroiban merged commit c4bd80e into trunk Oct 26, 2020
@adiroiban adiroiban deleted the 9351-pypi-upload-take-adiroiban branch October 26, 2020 15:07
@rodrigc
Copy link
Contributor

rodrigc commented Feb 1, 2021

@adiroiban This should not have passed review by @graingert, since this impacts the release process. Unfortunately, I missed this review

@@ -8,7 +8,7 @@
; * nodeps - avoid installing any dependencies apart from testing tools.
; * withcov - run the tests wrapped using the coverage.
; * nocov - run the tests directly, without using the coverage wrapper.
; * wheel - build the wheel distribution
Copy link
Contributor

Choose a reason for hiding this comment

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

This wheel target should not have been removed.

Copy link
Member

Choose a reason for hiding this comment

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

tox now builds the wheel before running any stage - it doesn't need a separate target to do it anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

@graingert This affects the release process. You and Adiroiban are not doing the current release (I am), so this is out of your scope to review, although technically the Twisted review process does not forbid this.

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 think that we need the help from @glyph or @exarkun to weight their views on this.

I stand by my review and I think that @graingert did an excellent work
and this PR is a big improvement for the Twisted release process.


I know that the review process for this PR is on the grey area.

The code was created by @graingert in a fork.
To review and tests it, I have created this PR in a branch on the main repo.
So by creating this PR I was doing the review.


I might be wrong and I am not against reverting this PR if required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another argument for keeping this PR (inspired by Jean Paul, see irc logs https://freenode.logbot.info/twisted-dev/20210201#c6744337).

This argument represent only my views and I am not representing JP or the Twisted community here :)

This PR is part of a long term goal of automating the release process.
The idea is that with this PR, you only need to create a tag, and the wheels are published by themselves :)
The release manager will not have to spend time on manually creating and publish the wheels.

The current release is in process since a very long time and there was not much progress.
I was feeling that by just following the old release process, we will continue to have delays.

So in parallel with the current release, some Twisted contributors (including myself) have tried to find a way to make the release work.

Cheers

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 am not in a hurry to see a release or have it on PyPI.
For my personal project I use a fork and an private package index.

If you want to revert this, feel free to do it.

The changes that I made were not done in a hurry and I asked for feedback over IRC, beside the normal GitHub PR.


I have worked with Thomas in the past on ldaptor and I know he is a good release manager and up to date with latest best practices in terms of Python package management and release process.

I consider that he is very competent to do and review release tasks.

My suggestion is to move this conversation over the mailing list so that more people could weight their opinion.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to revert all these changes you have done to the release documentation and
process.

However, it looks like someone (probably you?) has made a change to the GitHub Twisted repository, so
that a PR can only be merged to trunk, if it has been approved by someone who has write access
to the Twisted repository. As someone with write access to the Twisted repository, in the past (as recent as summer 2020), it was possible
for me to get reviews approved by people who did not have write access to the repository.
This is not the case any more.
In private e-mail, Glyph said that he did not make this change to the GitHub repository, but he is in favor of keeping this configuration. So I will respect that.

If I start reverting these changes, I will need to get review approval from another person with write access to the Twisted repository. This will further delay the release, which I am not in favor of doing.

So in the interest of moving things along with the release, I will not be reverting these changes,
but I will express my disapproval.

I appreciate your enthusiasm for the Twisted project and all that you have done over the years, such as trying
to revive the Project Leadership Committee.
Unfortunately, in this area, I find that your recent behavior is very aggressive,
although it is entirely legal via the Twisted development process.

Copy link
Member

Choose a reason for hiding this comment

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

Craig, can I suggest that you take the contributions of adiroiban and graingert to be in good faith?

Twisted is a group effort. Right now, I see three people who want to help Twisted in this area. I think that can be a great thing as long as we are working towards a common goal and not at cross purposes.

A lot of Twisted developers engage on IRC and on the mailing list which helps them come to a common goal and synchronize their efforts.

From your comments, I infer that you've had some private conversations with some people - but not with adiroiban or graingert. Do you think you could start having some public conversations about what your goals and activities are? I would suggest starting a thread on the Twisted mailing list rather than continuing to comment on this PR (which is public but obscure enough that a lot of people are going to miss out on the comments - as you yourself missed out on this PR when it was first submitted). I think that until all interested parties are productively communicating with each other, it's going to be very hard to resolve this to everyone's satisfaction.

I hope you take this comment in the spirit it is given - as an attempt to constructively work through some obstacles that have arisen from lack of a shared goal and coordinated efforts towards that goal - not as an attack or any sort of aggression towards you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@exarkun thanks for the feedback. Yes, I believe that adiroiban and graingert have been doing things in good
faith, however, the way it has been done comes across as aggressive, and is unappreciated by me.

I think that I have expressed my views in this thread, in in other PR's, so I don't feel a need to
open another thread on the mailing list.

In terms of private conversations, I have reached out to Glyph and Amber (who did the last release).
I tried to be respectful of Amber's role, and make sure that I was not offending anyone by stepping up and trying to release Twisted. Unfortunately, Amber has not responded to any of my emails, but Glyph has.

With respect to the release, for now, I am focusing on the last technical issue, and then cutting the release.

I think some of the root causes behind this problems we are seeing in this thread are due to the lack of an active Project Leadership Committee. If there is anything you can do to help in that regard, especially in terms of answering these questions which I raised on the mailing list: https://twistedmatrix.com/pipermail/twisted-python/2021-January/065405.html , that would be great.

Copy link
Member

@glyph glyph Feb 1, 2021

Choose a reason for hiding this comment

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

I think some of the root causes behind this problems we are seeing in this thread are due to the lack of an active Project Leadership Committee. If there is anything you can do to help in that regard, especially in terms of answering these questions which I raised on the mailing list: https://twistedmatrix.com/pipermail/twisted-python/2021-January/065405.html , that would be great.

There's a lot going on in this discussion thread and I'll try to write a more substantive message later, but I just wanted to address this specific detail:

The Project "Leadership" Committee is perhaps unfortunately named. This implies that the question you were asking about "roles" was not what I thought it was — about division of responsibilities within the group — but instead about what the group does as a whole.

So: the committee's sole responsibility is to suggest that expenditures be approved to the Software Freedom Conservancy, based on existing community consensus. (I can't say "approve expenditures" because it doesn't have the unilateral ability to do so, its oversight is subject to the authority of the SFC itself.) A better name for the group would be the Twisted Project Budgetary Oversight Committee. Much as the Python Software Foundation does not manage the day-to-day operations of the Python core development team, even a highly efficient PLC would not have any role in addressing issues like this one, except in that it might hire someone to do the work.

So there's no authority coming to save us here, we need to build a community consensus through discussion of the issues on the merits.

lint: pre-commit {posargs:run --all-files --show-diff-on-failure}

apidocs: {toxinidir}/bin/admin/build-apidocs {toxinidir}/src/ apidocs
narrativedocs: sphinx-build -aW -b html -d {toxinidir}/docs/_build {toxinidir}/docs {toxinidir}/docs/_build/

newsfragment: python {toxinidir}/bin/admin/check-newsfragment "{toxinidir}"

manifest-checker: check-manifest --ignore "docs/_build/**,docs/historic/**,admin/**,bin/admin/**"

twine: twine check {distdir}/*.*
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not have been removed

Copy link
Member

@graingert graingert Feb 1, 2021

Choose a reason for hiding this comment

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

it was not removed, it was moved to tox -e release-prepare

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.

None yet

5 participants