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

example: for GH Actions use action to publish #362

Merged
merged 15 commits into from
Jun 10, 2020

Conversation

breznak
Copy link
Contributor

@breznak breznak commented Jun 1, 2020

to PyPI, instead of calling twine directly.

Fixes #361

to PyPI, instead of calling twine directly.
@YannickJadoul
Copy link
Member

This is not a minimal example anymore. Could we just add this to a new example file examples/github-deploy-only.yml (cfr. examples/travis-ci-deploy-only.yml)?
Also, the Travis CI example only does this on tagged releases. Is it possible and easy to add to this example as well? Uploading every CI build to PyPI is not a good plan, as far as I know.

And just out of curiosity: what are the "problems/tricks" you mentioned in #361 that are avoided by this method?

@breznak
Copy link
Contributor Author

breznak commented Jun 1, 2020

Also, the Travis CI example only does this on tagged releases. Is it possible and easy to add to this example as well? Uploading every CI build to PyPI is not a good plan, as far as I know.

sure, it's easily done and documented right here:
https://github.com/pypa/gh-action-pypi-publish
I didn't want to overcomplicate the example, but I can add the line to do that.

And just out of curiosity: what are the "problems/tricks" you mentioned in #361 that are avoided by this method?

https://cibuildwheel.readthedocs.io/en/stable/faq/#no-module-named-xyz-errors-after-running-cibuildwheel-on-macos

@breznak
Copy link
Contributor Author

breznak commented Jun 1, 2020

This is not a minimal example anymore. Could we just add this to a new example file

sure, I can do that easily.

But imho this is a part of a minimal & useful example. People typically would want to publish the new wheel to PyPI. And honestly, this is just a single step added.
Imho, creating a second config for GH Actions would be more confusing (?)

@joerick
Copy link
Contributor

joerick commented Jun 1, 2020

But imho this is a part of a minimal & useful example. People typically would want to publish the new wheel to PyPI. And honestly, this is just a single step added.
Imho, creating a second config for GH Actions would be more confusing (?)

The minimal example is pulled into the docs as a 'starting point' for configs, which, to be consistent with each other, show the minimum required to run cibuildwheel only. But we can link from that page to your example config showing how to upload to PyPI.

@YannickJadoul
Copy link
Member

sure, it's easily done and documented right here:
https://github.com/pypa/gh-action-pypi-publish
I didn't want to overcomplicate the example, but I can add the line to do that.

Thanks! Given that this is a very sensible default (or stronger), I think it's good to include. This also nicely fits with the other examples we have.

https://cibuildwheel.readthedocs.io/en/stable/faq/#no-module-named-xyz-errors-after-running-cibuildwheel-on-macos

Ah, yes, OK. I knew about this, but never made the link with twine, indeed

But imho this is a part of a minimal & useful example.

Hmmm, not entirely sure. I still upload manually, just to be able to the build if there's an unexpected problem. Also, users might want to experiment first and only upload wheels afterwards. Or maybe they want to make wheels for private use. At any rate, it's not a core feature of cibuildwheel, so it's great to document but maybe not to include in the minimal example?

@breznak
Copy link
Contributor Author

breznak commented Jun 1, 2020

At any rate, it's not a core feature of cibuildwheel, so it's great to document but maybe not to include in the minimal example?

ok, I'll revert and copy to an extended example.

@breznak
Copy link
Contributor Author

breznak commented Jun 1, 2020

Ok, sorry for the hustle! :( Turns out the action is still only suppported on linux.
https://github.com/htm-community/htm.core/runs/727475761?check_suite_focus=true#step:15:9

There's a way I've been using (to upload & download artifacts and from linux to call the action), but for this example it's probably not worth it.

Feel free to close this and the issue. Sorry for wasting your time.

@henryiii
Copy link
Contributor

henryiii commented Jun 1, 2020

That's actually the "correct" way to do it, ideally you want to make sure all jobs are successful before you upload, and you should upload once (there is now way to ignore existing in the action because of that).

@YannickJadoul
Copy link
Member

Ok, sorry for the hustle! :( Turns out the action is still only suppported on linux.

Egh, that's annoying. We can keep this PR in mind for when it gets supported, though, so that your work wasn't for nothing. If you notice something, do feel free to let us know! :-)

(It's going to be quite interesting to see which one of all these newly create GitHub Actions is going to survive, in a few year, btw. I feel there's quite a few ad-hoc actions out there. But then again, this was from PyPA, so you'd think they'd support more than just Linux.)

@breznak
Copy link
Contributor Author

breznak commented Jun 1, 2020

. But then again, this was from PyPA, so you'd think they'd support more than just Linux.)

yea, so was my thinking. The irony is it was me who brought the issue up a year ago.
Ping to the issue for reference: pypa/gh-action-pypi-publish#15

@henryiii
Copy link
Contributor

henryiii commented Jun 2, 2020

Couldn't this just be changed to the correct way to do it, with jobs and artifacts? It's not much longer, and doesn't risk uploading some wheels when other wheels break.

@henryiii
Copy link
Contributor

henryiii commented Jun 2, 2020

Also, if we had alternate arch support, like on Travis, then twine doesn't even work there.

@henryiii
Copy link
Contributor

henryiii commented Jun 2, 2020

This is reinvented by everyone using gha, see also https://github.com/htm-community/htm.core/blob/master/.github/workflows/release.yml#L17 from #364 - I think it should be in the examples and docs...

@YannickJadoul
Copy link
Member

Couldn't this just be changed to the correct way to do it, with jobs and artifacts? It's not much longer, and doesn't risk uploading some wheels when other wheels break.

If something easily works, yes, we can make examples/travis-ci-deploy-only.yml (or examples/travis-ci-upload.yml or ...) and add it to the examples and link to it. I had understood it wasn't possible, but given your comment in #361, it would be nice to have that.

@YannickJadoul YannickJadoul reopened this Jun 2, 2020
@YannickJadoul
Copy link
Member

@henryiii I've implemented your suggestion from #361; do you mind having a look?

Also, what did you mean by

This is reinvented by everyone using gha, see also https://github.com/htm-community/htm.core/blob/master/.github/workflows/release.yml#L17 from #364 - I think it should be in the examples and docs...

Should something else still be added?

examples/github-deploy-only.yml Outdated Show resolved Hide resolved
examples/github-deploy-only.yml Outdated Show resolved Hide resolved
…ated into the pypa/gh-action-pypi-publish action
@YannickJadoul
Copy link
Member

Thanks, @henryiii! Fixed and pushed! :-)
Nothing's being tested, ofc, so we don't really know if I haven't made any mistake in doing so, but ... eh

@henryiii
Copy link
Contributor

henryiii commented Jun 5, 2020

I've made a release with a pure python package using a similar setup, and I think this should work, but here are some suggestions:

  • Build only on published, full releases, rather than all release changes:
release:
    types:
    - published
  • Use the jobs.<jobid>.if key to disable/enable the whole job, rather than just the final publish (as it's only a download then publish anyway).

Pages here and here have been updated.

examples/github-deploy.yml Outdated Show resolved Hide resolved
@YannickJadoul
Copy link
Member

  • Build only on published, full releases, rather than all release changes:
release:
    types:
    - published

Where should I add this?

@breznak
Copy link
Contributor Author

breznak commented Jun 8, 2020

Where should I add this?

https://github.com/joerick/cibuildwheel/pull/362/files#diff-b4971e357cad581b8716b72ee38f58a5R3

@YannickJadoul
Copy link
Member

So ...

on:
  - push
  - pull_request
  - release:
      types:
        - published

... ?

@YannickJadoul
Copy link
Member

OK, done, both!

@henryiii Is there anything left to do? Or could we merge this soon?

examples/github-deploy.yml Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

henryiii commented Jun 8, 2020

Other than my one comment, I think it's ready. :)

@YannickJadoul
Copy link
Member

OK, @joerick, ready if you're happy, then.

AppVeyor failing is related to current problems PyPy has with portably supporting macOS, it seems. See e.g. https://foss.heptapod.net/pypy/pypy/issues/3229.

I currently don't really have the time to investigate this, though. Not sure if there's a quick and dirty solution. And also not sure why this is suddenly breaking: we're still using the same PyPy 7.3.1 release and binary, so AppVeyor's image must have changed?

@henryiii
Copy link
Contributor

henryiii commented Jun 8, 2020

Since the other macOS builds are working, I would vote for an Appveyor change being the culprit.

@YannickJadoul
Copy link
Member

Since the other macOS builds are working, I would vote for an Appveyor change being the culprit.

Yes, but it seems it's just exposing an issue in PyPy's linking to HomeBrew's libffi?

@henryiii
Copy link
Contributor

henryiii commented Jun 8, 2020

Yes, that's what it looks like.

@YannickJadoul
Copy link
Member

Let's see: #371

@YannickJadoul
Copy link
Member

Also, unrelated, but something weird seems to be happening to the delocate installation:

ERROR: delocate 0.8.0 requires bindepend, which is not installed.
ERROR: delocate 0.8.0 requires machomachomangler, which is not installed.

examples/github-deploy.yml Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor

henryiii commented Jun 8, 2020

Also, unrelated, but something weird seems to be happening to the delocate installation:

ERROR: delocate 0.8.0 requires bindepend, which is not installed.
ERROR: delocate 0.8.0 requires machomachomangler, which is not installed.

This should only happen if you have delocate on Windows. Which I don't think you should be doing, since it is for relocating macOS wheels (unless it also supports Windows wheels, which I don't think it does)...

https://github.com/matthew-brett/delocate/blob/ef52b98b9f5e753a07cf5ce7495ff734187be3ad/setup.py#L31-L32

@YannickJadoul
Copy link
Member

Also, unrelated, but something weird seems to be happening to the delocate installation:

ERROR: delocate 0.8.0 requires bindepend, which is not installed.
ERROR: delocate 0.8.0 requires machomachomangler, which is not installed.

This should only happen if you have delocate on Windows. Which I don't think you should be doing, since it is for relocating macOS wheels (unless it also supports Windows wheels, which I don't think it does)...

https://github.com/matthew-brett/delocate/blob/ef52b98b9f5e753a07cf5ce7495ff734187be3ad/setup.py#L31-L32

Yes, good point. But it is happening on the macOS build (that was failing, which is why I noticed this in the output).

But the lines

  Ignoring machomachomangler: markers 'sys_platform == "win32"' don't match your environment
  Ignoring bindepend: markers 'sys_platform == "win32"' don't match your environment

do not show for Python 2.7. So maybe something with an old setuptools version there?

Copy link
Member

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

@joerick I think this can be merged soon. We've gone over these few lines of documentation quite a few times by now, and it would be nice to conclude the discussion.

The build failure on AppVeyor is unrelated (all of these files are documentation anyway), and is being investigated/solved in #371.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Just a small change from me. I agree, lets get this in.

@joerick joerick merged commit e12ca14 into pypa:master Jun 10, 2020
@breznak breznak deleted the gh-action-pypi-publish branch June 10, 2020 13:49
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.

For GH Actions, use gh-action-pypi-publish instead of twine
5 participants