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

feat: add support for composite-actions #494

Merged
merged 3 commits into from
Dec 28, 2020
Merged

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Dec 21, 2020

See #492

Demo / tested here: scikit-hep/boost-histogram#488.

Should we update the readme and/or examples? I've updated the readme but not the examples in the current state of the PR. I think we should also add a note on how to setup dependabot to update it - I first learned about it from a different GitHub action's readme, actually. Using pinned versions but having a bot make an update PR every so often is really ideal for CI. It really helps clarify when an update breaks your CI, or when you do. :)

@henryiii
Copy link
Contributor Author

henryiii commented Dec 21, 2020

Ran into a problem - ${{ github.action_path }} is a Windows-style path on windows, and you have to pick a shell. But powershell-core is supported on all platforms, and it understands Windows-style paths and Unix-style paths (bash does not do Windows style), so that fixed it.

Also note that all steps need to be run with shell, no actions-in-actions or automatic shell switching (they should add an auto choice to opt into the behavior they normally have by default - the reason you have to have a shell setting is because you can change the default in your workflow).

@henryiii
Copy link
Contributor Author

FYI, you can't drop the setup-python action, for some reason it seems to pick up python2 somewhere even though python3 should be available on the default runners. You are supposed to run setup-python to use python anyway, so not a big deal, was just curious and tried it out.

This action does leave cibuildwheel installed in your environment after the action runs, but don't think that's a big issue and it speeds it up afterwords if you run it again (such as to make 32-bit and 64-bit Windows wheels when you have to activate the Windows compiler, such as for pybind11 + Python 2.7). We could always make a venv and install it there, then delete the venv at the end, if that's a problem, though.

@joerick
Copy link
Contributor

joerick commented Dec 23, 2020

This looks really nice! Can we update the examples in /examples, too? Or should we do a release with this first, just to check it all works?

I'm thinking maybe we should just commit the action.yml now, then do a release. Then we can update the docs and test the minimal example configs, and release with confidence.

@YannickJadoul
Copy link
Member

release with confidence.

I wouldn't expect there's a lot not to be confident about (especially because @henryiii already tested this?). But yeah, it's just docs, so we can add them whenever :-)

@joerick
Copy link
Contributor

joerick commented Dec 23, 2020

I'm mostly thinking it would be nice to run bin/run_example_ci_configs.py, and that I don't want the README to be wrong while I'm getting the action set up :)

@YannickJadoul
Copy link
Member

Right, yes! For that to work, we will need to have a release?

Unless you can test it out with uses: joerick/cibuildwheel@master, once this PR is merged? @henryiii?

@henryiii
Copy link
Contributor Author

henryiii commented Dec 23, 2020

I've already been testing this out with uses: henryiii/cibuildwheel@feat/actions, actually, if you see the linked PR. You can pin to a commit, too. So I'm pretty sure it works, but those are also famous last words...

I'm perfectly happy to avoid changing the readme for now, then we can adapt to it later if we like it. We could (only) put this in a little section on "keeping up to date" for the next release, then expand later.

action.yml Show resolved Hide resolved
@joerick
Copy link
Contributor

joerick commented Dec 23, 2020

Yes, I think I'd prefer to merge just the actions.yml now. I can do a 'stealth release', not mention it in the CL. Then we can convert the example configs and README, and that next release would also give me a chance to ensure my bump_version.py works with the new version spec.

Sound okay? We can continue to use this PR to write the docs, I'll just copy actions.yml over to master and merge master back into here.

@henryiii
Copy link
Contributor Author

I'll make your change, and I'll keep the two changes as two separate commits - just cherry-pick the first if you want to avoid any docs changes (though check the new docs, it's much less intrusive and more of an option for now).

@henryiii
Copy link
Contributor Author

Using legacy 'setup.py install' for cibuildwheel, since package 'wheel' is not installed.
Using legacy 'setup.py install' for bashlex, since package 'wheel' is not installed.

I take it cibuildwheel doesn't have a pyproject.toml? :'(

@joerick
Copy link
Contributor

joerick commented Dec 23, 2020

I take it cibuildwheel doesn't have a pyproject.toml? :'(

haha, no, I've been dragging my feet on that one!

@henryiii
Copy link
Contributor Author

PR is three commits. First is composite-action support. Second is docs. Third is pyproject.toml. Though, if you don't want the third one, we need to add a step to the composite action to install wheel first, so we get a proper build procedure. (Also need to see why bashlex doesn't ship wheels, maybe they need to use cibuildwheel!)

@YannickJadoul
Copy link
Member

YannickJadoul commented Dec 23, 2020

(Also need to see why bashlex doesn't ship wheels, maybe they need to use cibuildwheel!)

bashlex is a pure Python library, no? (You'd sometimes forget they still exist :-p )

@joerick
Copy link
Contributor

joerick commented Dec 23, 2020

That's cool, thank you. I think I can merge the whole lot, in fact, since the README change is no longer there.

@henryiii
Copy link
Contributor Author

henryiii commented Dec 23, 2020

bashlex is a pure Python library, no?

Yes, what does that have to do with not shipping wheels? If anything, it's more important, because they always get downloaded regardless of the system! If you look at the benefits on https://pythonwheels.com, you'll see most of them apply to pure Python too:

  • Safer: no arbitrary code exception (no setup.py)
  • Faster: Pip pre-compiles all .pyc files on install (it does not with setup.py & SDist)
  • Smaller: fewer extra files (like setup.py, sometimes tests, etc)
  • Simpler: no setuptools or other packages need.

It's really just an organized zipfile, and Pip just copies the files around and pre-compiles bytecode. Much better than code execution on install!

@YannickJadoul
Copy link
Member

It's really just an organized zipfile, and Pip just copies the files around and pre-compiles bytecode. Much better than code execution on install!

Yup, I kind of knew that (though not as clear; thanks for the explanation! :-) ), but 1) that might explain why there are no wheels (the maintainer of the project not knowing about the difference), and 2) so the project can't use cibuildwheel :-(

@henryiii
Copy link
Contributor Author

henryiii commented Dec 23, 2020

We've had a few people ask about supporting pure-python packages in cibuildwheel. 🤦

@YannickJadoul
Copy link
Member

Meanwhile, the irony: https://pythonwheels.com/ shows that there are no wheels for future

@YannickJadoul
Copy link
Member

We've had a few people ask about supporting pure-python packages in ci_build_wheel.

That kind of makes sense, maybe? Arguably, going from setup.py to a wheel is still "building"?
But yeah, you could just set up a CI job, and you'd still build in a clean environment. Maybe that's something to add to the docs, since cibuildwheel is becoming part of the central knowledge/tool hub for making wheels? :-)

@YannickJadoul
Copy link
Member

YannickJadoul commented Dec 23, 2020

Off-topic: should we submit a PR to plug cibuildwheel for the C extensions on https://pythonwheels.com, or is that too pretentious?

EDIT: Very happy to do so, but I'd first like to hear @henryiii's opinion and @joerick's go-ahead, before doing so :-)

@henryiii
Copy link
Contributor Author

We should probably mention that python -m pip install build && python -m build -s makes SDists already, and then mention that for pure python wheels, all you have to do is drop the -s and you get both a wheel that works everywhere and the SDist - voilà, no cibuildwheel needed. I don't really understand the holdup on the wheel issue in future, it seems they are making platform specific pure Python wheels...

@henryiii
Copy link
Contributor Author

My edit to the setuptools docs was accepted, they no longer recommend the wrong tool (pep517.build, which almost got yanked because the author didn't want anyone to use it and didn't think anyone was using it 🤦) https://setuptools.readthedocs.io/en/latest/build_meta.html

Sure, try an issue or PR for it, can't hurt! You could recommend both multi-build and cibuildwheel, to be diplomatic. :)

@YannickJadoul
Copy link
Member

Sure, try an issue or PR for it, can't hurt! You could recommend both multi-build and cibuildwheel, to be diplomatic. :)

That's what I had in mind, indeed. @joerick?

@joerick
Copy link
Contributor

joerick commented Dec 24, 2020

Sounds great! I agree, I think the site could use a little pragmatic advice in that section. Cibuildwheel and multibuild are the best and most popular options, as far as I'm aware. :)

@YannickJadoul
Copy link
Member

Great! Submitted a (hopefully modest & polite) meshy/pythonwheels#117 :-)

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.

I think let's merge this as-is, and we can make a follow-up PR to update the docs to make the github action the preferred way of doing things.

@henryiii henryiii merged commit 4e17c27 into pypa:master Dec 28, 2020
@henryiii henryiii deleted the feat/actions branch December 28, 2020 03:26
@henryiii
Copy link
Contributor Author

henryiii commented Jan 1, 2021

Using the new release here: scikit-hep/boost-histogram#488. If all works well, I'll merge that & then could propose a doc update.

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