Repository navigation
feat: clean up and update to pybind11 2.6.0 - #28
Conversation
|
One thing that bothers me, by the way, is that we are just running from the build directory; we never run the install step. CMake may not setup RPaths correctly, for example (unless you mess with |
|
I think all review points are now addressed. We still use Ninja by default (since it comes with a wheel, while make does not, along with being faster and better), but I now allow I also put in support of PS: Any PEP 517 system cannot directly interact with setup.py commands. Python is moving away from "setup.py" being required or used by users. A general hook for parallelization would need to be added. They are still working on an "editable mode" hook, so this is probably very low priority, especially since setuptools parallelization is so bad. |
|
You don't need |
|
Setting |
|
You must supply make or ninja in your pyproject.toml, otherwise you may not be able to build. You can't supply make, so that leaves ninja. There is no promise that make is available in UNIX. Usually, sure, but not always. Windows doesn't matter, but downloading ninja doesn't hurt (and I'd rather use it there, I just haven't figured out how to broadcast the setuptools compiler discovery to Ninja). I could use an environment marker to disable it on windows, I believe, but I think it's better simple (and not 100% about that).
And I did add support for that. But my point is that Also, one place you may need to limit the number of threads is when building wheels in CI (such as I have to do on ARM emulation for boost-histogram, more than one thread crashes without enough memory), then you can't do that if you can't access the parallel compiler control when But you have convinced me that it should be supported too. :) I just felt that it might not be needed in a "simple" example since the only way to use it requires knowledge of setting up a "fast and dirty" development environment. |
|
Looks like pyproject.toml just implements standard environment markers, PEP 518 just points at PEP 508 (which is good!). I can turn off ninja being downloaded needlessly (for now) for Windows then. |
|
Also, I think an example should cover in as many places as possible. A user can always remove the ninja part if they think make will always be available. It almost always is if you have a compiler. For a library, I might be willing to risk it, but I don't think a general example should. |
|
Let's simplify and mostly drop the custom thread handing, since Trying to use |
You still need compilers as well, and I think it's safe to assume that any Unix system with compilers has (or should have?)
Probably, yes. Though in this case, this example is meant as a minimal example (that's sturdy enough to build on and extend), right? That all being said, Henry's approach seems like giving a good and safe example for users, and, while I've not really used it myself, Ninja isn't all too new or radically different from |
YannickJadoul
left a comment
There was a problem hiding this comment.
Preferably, @HDembinski would also be happy with the found compromise, but I don't need any changes :-)
|
In my opinion, a minimal example should work as many places as possible (such as in docker images that don't have make), but it doesn't need to be full featured (say, having multithreaded builds). Again, this for me is a good starting point to attempt removing MSBuild and using pure Ninja, which would both simplify the if statements, and fix the current issue that MSVC isn't picked up correctly from setuptools. @HDembinski , is this a reasonable enough compromise for you to sign off on it too? I plan to refine with a PR in the near future to fix the MSVC discovery somehow. I could instead make-only PR if Ninja doesn't help solve that issue and I am able to fix the current Ninja on Unix, MSBuild on Windows structure on Windows. I think anything is better than the current broken-on-all-CI state... :) |
|
Like @YannickJadoul, I have never seen a UNIX-y image that has a compiler but comes without iminuit builds with my own version of this example (before @henryiii made these changes) just fine on Windows, MacOS and Linux without This is a template to give users a good start for customization, it is not a library solution that has to offer all possible features and has to cover all possible use cases. That's why I am so insistent on having it minimal. We want this to be easy to read and uncluttered. I will not argue further for my position since there is nothing else to say. |
docker run -it --rm ubuntu
apt update && apt install python3-dev python3-venv g++ git
python3 -m venv venv
. ./venv/bin/activate
pip install git+https://github.com/scikit-hep/iminuit.git(I've stopped here, it continues to spew errors that rather hide the first one here, making look more like it can't find a compiler) pip install git+https://github.com/henryiii/cmake_example.git@feat/260 |
|
Opps, sorry, it was only conda that's having a problem, I do have |
Updating CI and cleaning up the example. CI work mostly copied from python_example - after all, most packages should behave "nicely" under python Packaging and not need significant customizations/workarounds. Adds the full GitHub Actions testing, including conda and cibuildwheel.
Discovery is not perfect - if you build on windows-latest in GitHub Actions, for example, the conda example breaks because it's looking for MSVC 2017 instead of 2019 - while it works with the python_example. Something for a later followup. I'd like to try (again) to use Ninja everywhere in the future; the problem is communicating the correct MSVC to Ninja.
I took some inspiration from @HDembinski's work on iMinuit and @jpivarski's work on Awkward-1; this helped solve an issue with 32/64 bits and an issue with Conda (thank you!) (closes #25). Also closes #7.
This is still using the submodule method, rather than PyPI method. It's probably best to have a mix - python_example uses PyPI, so I don't think this one should change. But we might eventually want to note that somewhere.
Obviously needs a squash and merge since I left a mess in the history. I'll likely be referring to it later, though, so that's why I left it.