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/cmake #1

Closed
wants to merge 1 commit into from
Closed

Feat/cmake #1

wants to merge 1 commit into from

Conversation

henryiii
Copy link
Owner

@henryiii henryiii commented Jul 28, 2020

This is a complete CMake update, part 1 of approximately 2.

CMake 3.7 or better is now required

This enables many simplifications, and a more unified approach to targets. Many if statements dropped, though a few more added for enhanced support for later CMake versions.

Reworked C++ support to properly handle C++ version in CMake instead manually

CMake 2.8, which came out three years before 2011, does not have C++11 support (hopefully not a surprise, though given the number of people that seem to want to use a new compiler with a CMake version that significantly predates it, I'm not sure). Now it does, so PyBind11 plays nice with C++ support! FINALLY! Key points:

  • PYBIND_CPP_STANDARD is deprecated, and issues a warning. It does still work, though, just grabbing the last two digits and setting CMAKE_CXX_STANDARD with it.
  • If a compiler has a reasonable default (like many do now), we get to just use that, instead of injecting a flag.
  • CMAKE_CXX_STANDARD is not required - CMake knows that pybind requires at least C++11 support, and will automatically add at least the C++11 flag if needed to any target that links to it (via compile features) if the compiler has a default less than C++11.
  • If a target requires a higher level, that will be used instead (not duplicated, like before), either through compile features (per target) or via CMAKE_CXX_STANDARD (global) or CXX_STANDARD (per target, not inherited)
  • Because it's built into CMake, it correctly supports all compilers, and correctly mixes languages (that is, it is escaped correctly for CUDA!)
  • We get to remove the old caveats about setting the C++ standard before PyBind11 is used - it can be placed anywhere now.

Closes pybind#1097

If-less config files

The config file generated will not depend on the host system - this is a requirement for the CMake config files to be distributed in a pure Python wheel - which can't know what the host compiler / python version is beforehand. Generator expressions are used, which are evaluated during the build, rather than before.

Note: there are two ifs that affect the configs: If you generate with CMake 3.13+, it cannot be read by 3.7-3.12. However, this is much better than before, were you couldn't change much of anything after generating a Config file, and crucially, the 3.13+ one has a lower potential for bugs (it enforces that the dynamic lookup does not get de-duplicated accidentally). The main planned user of this feature is PyPI package, and there should be no problem getting CMake 3.13+ using pip.

Key first step for pybind#2266!

SYSTEM changes

The current handling of SYSTEM is not consistent with CMake standards. CMake treats all IMPORTED targets as SYSTEM, so this was/is always on when using configuration mode.Tthis now treats Python always as SYSTEM (may fix the warning we struggled with for a while), and treats PyBind11 as SYSTEM when being used as a subdirectory, so the SYSTEM argument to pybind11_add_module is no longer needed and is inconsistent with target_link_libraries. A warning is printed if given. You can always set NO_SYSTEM_FROM_IMPORTED if you want to make imported targets non-system.

Smaller features

This has gotten large enough, so I'll be working on new FindPython support in the next PR. Also, we currently might be a bit sloppy about putting build produces in the source tree - I'll investigate that in the near future, too. This also should enable CMake configs in the python wheel, which when mixed with PEP 518 builds and scikit-build, could make something truly beautiful. :)

As a further test, I dropped this into boost-histogram (CMake mode) and awkward1 (both submodule systems) and verified that no changes were needed, it is backward compatible and still builds correctly locally.

@henryiii henryiii force-pushed the feat/cmake branch 12 times, most recently from bdc4e03 to 6f8c2a8 Compare July 29, 2020 01:24
@henryiii henryiii closed this Jul 29, 2020
henryiii pushed a commit that referenced this pull request Aug 21, 2023
* Remove newlines from docstring signature

* Jean/dev (#1)

Replace newlines in arg values with spaces

* style: pre-commit fixes

* Don't use std::find_if for C++ 11 compatibility

* Avoid implicit char to bool conversion

* Test default arguments for line breaks

* style: pre-commit fixes

* Separate Eigen tests

* style: pre-commit fixes

* Fix merge

* Try importing numpy

* Avoid unreferenced variable in catch block

* style: pre-commit fixes

* Update squash function

* Reduce try block

* Additional test cases

* style: pre-commit fixes

* Put statement inside braces

* Move string into function body

* Rename repr for better readability. Make constr explicit.

* Add multiline string default argument test case

* style: pre-commit fixes

* Add std namespace, do not modify string repr

* Test for all space chars, test str repr not modified

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment