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: support setup helpers #60

Merged
merged 7 commits into from
Oct 22, 2020
Merged

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Sep 18, 2020

Moving to the new setup helpers infrastructure being developed in pybind11 2.6.0. Adding cibuildwheel. Adding GHA checks.

There are three methods to use this; I'm using the setup_requires method; while this is discouraged, it works just about everywhere (at least it does not require pip 10 like the PEP 518 method) and it's more complex than the PEP 518 method, which is basically just a simplification on this.

I'll probably refactor after this gets merged to use PEP 518, but for now, it is good to have this in the history, and it would help to have an actual release of pybind11, since most of the work in the PR is getting pip to install from PyPI, which requires CMake and such. I've disabled Python 3.5 + Windows + cibuildwheel, since it does not correctly use PEP 518 due to "portable" install issues. It can handle PEP 518 only in the main project, not a dependency. (We could enable this by splitting the install into cmake first, then pybind11, but it won't matter once we use a released version, or even a beta version from PyPI).

Actions run can be seen here: https://github.com/henryiii/python_example/actions/runs/261642597

I've also added pip and conda builds to Actions; Travis should be able to be removed after this is merged, and appveyer reduced quite a bit. I don't have macOS & Windows conda builds running yet, due primarily to the fact it's hard to setup pybind11 from source since CMake isn't finding a valid compiler. As soon as we have (even a beta) release of pybind11, then it should be easy to add these back in (and simplify everything else). If we wanted to make it work, the conda-forge "compilers" package would probably set up the correct env variables for it to work.

Here's the contents of the file attached to the run (click "artifact" on that page to download):

python_example-0.0.1-cp27-cp27m-macosx_10_9_x86_64.whl
python_example-0.0.1-cp27-cp27m-manylinux2010_i686.whl
python_example-0.0.1-cp27-cp27m-manylinux2010_x86_64.whl
python_example-0.0.1-cp27-cp27m-win_amd64.whl
python_example-0.0.1-cp27-cp27m-win32.whl
python_example-0.0.1-cp27-cp27mu-manylinux2010_i686.whl
python_example-0.0.1-cp27-cp27mu-manylinux2010_x86_64.whl
python_example-0.0.1-cp35-cp35m-macosx_10_9_x86_64.whl
python_example-0.0.1-cp35-cp35m-manylinux2010_i686.whl
python_example-0.0.1-cp35-cp35m-manylinux2010_x86_64.whl
python_example-0.0.1-cp36-cp36m-macosx_10_9_x86_64.whl
python_example-0.0.1-cp36-cp36m-manylinux2010_i686.whl
python_example-0.0.1-cp36-cp36m-manylinux2010_x86_64.whl
python_example-0.0.1-cp36-cp36m-win_amd64.whl
python_example-0.0.1-cp36-cp36m-win32.whl
python_example-0.0.1-cp37-cp37m-macosx_10_9_x86_64.whl
python_example-0.0.1-cp37-cp37m-manylinux2010_i686.whl
python_example-0.0.1-cp37-cp37m-manylinux2010_x86_64.whl
python_example-0.0.1-cp37-cp37m-win_amd64.whl
python_example-0.0.1-cp37-cp37m-win32.whl
python_example-0.0.1-cp38-cp38-macosx_10_9_x86_64.whl
python_example-0.0.1-cp38-cp38-manylinux2010_i686.whl
python_example-0.0.1-cp38-cp38-manylinux2010_x86_64.whl
python_example-0.0.1-cp38-cp38-win_amd64.whl
python_example-0.0.1-cp38-cp38-win32.whl
python_example-0.0.1-cp39-cp39-macosx_10_9_x86_64.whl
python_example-0.0.1-cp39-cp39-manylinux2010_i686.whl
python_example-0.0.1-cp39-cp39-manylinux2010_x86_64.whl
python_example-0.0.1-cp39-cp39-win_amd64.whl
python_example-0.0.1-cp39-cp39-win32.whl
python_example-0.0.1-pp27-pypy_73-macosx_10_9_x86_64.whl
python_example-0.0.1-pp27-pypy_73-manylinux2010_x86_64.whl
python_example-0.0.1-pp27-pypy_73-win32.whl
python_example-0.0.1-pp36-pypy36_pp73-macosx_10_9_x86_64.whl
python_example-0.0.1-pp36-pypy36_pp73-manylinux2010_x86_64.whl
python_example-0.0.1-pp36-pypy36_pp73-win32.whl
python_example-0.0.1.tar.gz

Note that AppVeyor was completely broken; I've remove conda from it, and fixed it for pip builds.

May help #58 (might need support of minGW in setup helpers)
Closes #48
Closes #44
May help #35
Closes #23
May help #21
Closes #16

@henryiii henryiii force-pushed the feat/setup-helpers branch 3 times, most recently from dc77f03 to be446a1 Compare September 18, 2020 02:07
@henryiii henryiii closed this Sep 18, 2020
@henryiii henryiii reopened this Sep 18, 2020
@henryiii
Copy link
Collaborator Author

henryiii commented Sep 18, 2020

@YannickJadoul, in cibuildwheel:

pip wheel /project -w /tmp/cibuildwheel/built_wheel --no-deps

Why --no-deps? This would work fine with PEP 518, but it seems like it should work with setup_requires/install_requires too? (or is it something else going wrong here?). At this point, since I've tested it with this method, I'd also be okay to switch to the "correct" PEP 518 system.

(Looking at the docs, this seems correct, just slightly odd, unless the worry is that extra wheels will be placed in the final directory, but I think that can happen anyway. Might just chalk it up to another reason PEP 518 is better.)

@henryiii henryiii force-pushed the feat/setup-helpers branch 4 times, most recently from c54cdc7 to 3f76df9 Compare September 18, 2020 05:22
@YannickJadoul
Copy link

Why --no-deps?

As far as I know, --no-deps stops pip wheel from downloading/building wheels for all the install_requires dependencies, and thus having many more files that you're actually not trying to build (for example, my project would drag in a numpy wheel, that's already on PyPI anyway).

When the created wheel gets tested with PyPI, pip install ....whl is ran, which installs all dependencies into the temporary virtual environment; so any wheel/package dependencies are downloaded and installed then.

@henryiii
Copy link
Collaborator Author

Okay, but this a) breaks setup_requires, and b) I really think pyproject.toml dependencies that need to be built into wheels to install get thrown here too (even though they are never installed into the active environment, the same wheelhouse directory used for placing built wheels). I'm not really worked about breaking setup_requires, because it's deprecated and a hack anyway. Putting pyproject.toml requirement's build wheels in is irritating, but the process must have them to build, so I don't think there's a better solution (I usually filter wheels based on project name when uploading) and (python -m build probably should solve this properly, I would think, by keeping the "output" directly separate from the wheelhouse).

@henryiii henryiii force-pushed the feat/setup-helpers branch 4 times, most recently from 96b934d to ee6126f Compare September 18, 2020 18:58
@henryiii henryiii marked this pull request as ready for review September 19, 2020 14:12
@YannickJadoul
Copy link

Actions are not yet enabled in this repository?

@henryiii
Copy link
Collaborator Author

I've been building them locally. There might be a setting in the "actions" tab that @wjakob can set, otherwise, it might need to be merged first. I'll trigger a build in my fork again.

@henryiii
Copy link
Collaborator Author

Edit: no, I can see the Actions tab, so I think it has to be in this repo before it will build.

@YannickJadoul
Copy link

Let's see when this is merged, then. I thought when Actions were enabled, things just started building when adding a workflow, but I might be mistaken.

@henryiii henryiii force-pushed the feat/setup-helpers branch 3 times, most recently from 65089a3 to 4527f0a Compare October 19, 2020 14:08
@henryiii henryiii force-pushed the feat/setup-helpers branch 2 times, most recently from e947f35 to 26a0a17 Compare October 19, 2020 14:48
Copy link

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

With the usual caveats (I don't know a lot about the details): this is a huge amount of work, going in a good direction, we can always fix details later.

Note that AppVeyor was completely broken

Sounds like it can only get better.

I'll probably refactor after this gets merged to use PEP 518, but for now, it is good to have this in the history,

I think that's a great plan.

@@ -1,5 +1,8 @@
#include <pybind11/pybind11.h>

#define STRINGIFY(x) #x
#define MACRO_STRINGIFY(x) STRINGIFY(x)
Copy link

Choose a reason for hiding this comment

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

I haven't come across this before ... why is the 2nd macro needed? Is this working around compiler bugs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an idiom if you need to stringify the contents post macro expansion. Just the first one would not expand the macro inside. It's BOOST_PP_STRINGIZE, basically.

@henryiii
Copy link
Collaborator Author

Okay, let's merge then cleanup as needed. :)

@henryiii henryiii merged commit 2c99526 into pybind:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants