-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add support for CMake compiler features #1098
Conversation
2083cf3
to
449606a
Compare
Is there a way to detect whether something has set the minimum version to 3.8? Or in other words, what happens to my pybind project if I have |
That's a fairly significant blocker; the default changing from C++14 to C++11 is definitely going to break some projects that assume the default is (at least) C++14. |
Yes, I've got an idea or two; selecting the default as "the latest the compiler supports" is not the problem, it's allowing people to build C++11 code if PyBind asks for 14. It could be (another) CMake variable, or it could look for "-or/ std=X" but there might be a better way. Still a work in progress, and open to suggestions. |
Sort of, you can check the settings of CMake policies. CMP0067 was added in 3.8, and even covers a C++ standard setting. |
So, here are ideas:
Even in the current implementation, nothing is changed if you use Thoughts? |
cdc49bc
to
9d663ce
Compare
Okay, I'm pretty happy with this now. Here's the logic, from top to bottom, new parts in bold:
I've added a one time message if auto-discovery works, telling the user the mode and the mechanism. It won't display if you set one of these variables yourself or if you are rerunning (already in cache). This allows pybind11 to take advantage of the faster and officially supported CMake methods for C++ standard discovery (CMake pre-populates the compiler feature list). It also improves interaction with other targets, removes double std listings, and might fix #1097, allowing CMake 3.8+ to mix C++ and C targets (please check). To test this, you'll need to clear your CMakeCache.txt or remove the value from If it looks good, I can add a note to the docs and possibly release notes. The other option would be adding compile features for 3.1-3.8, but those would be a list of features pybind11 needs, and CMake would then pick what flags to pass; the meta-features seems simpler and seems to be the primary way CMake is adding feature support for C++17. The old way of individual features was nice because it could detect if you didn't have support, such as G++ 4.7 vs. 4.8. This method still supports someone adding a list of features to |
aed702f
to
b1d212c
Compare
Added the new C++ discovery to pybind11 CONFIG, as well. |
CMake requires a different syntax for IMPORTED targets; I've fixed the adder function to handle that (fixes config targets). Note: In CMake 3.10 or 3.11, this oddity may go away; there's work being done to fix that here. |
Adding CXX flags is now consolidated into one place, in a helper function ( Ready for review. By the way: I could use an explicit list of compile features, and that would get the benefits for CMake 3.3+ or even 3.1+ with a bit more work. The current method is a bit cleaner in the (very) long run. |
9760502
to
5b925e9
Compare
5b925e9
to
30d39aa
Compare
30d39aa
to
924c34b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies that it took me a really long time to get to this. Some questions/comments for you to review.
# These meta-features were added in CMake 3.8: | ||
set(PYBIND11_CXX_FEATURES cxx_std_11) | ||
set(PYBIND11_CXX_FEATURES cxx_std_14) | ||
set(PYBIND11_CXX_FEATURES cxx_std_17) # Experimental C++17 support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The # Experimental
comment can be removed I suppose? :)
# Only try to get the standard manually if CMake doesn't support compiler features | ||
if(MSVC) | ||
set(PYBIND11_CPP_STANDARD "/std:c++14") | ||
message(STATUS "pybind11 selected C++14 flag, MSVC") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change all the status messages to: pybind11: using C++14
, etc.?
# otherwise PYBIND11_CXX_FEATURES), and then at this point are promoted to the CACHE. | ||
|
||
set(PYBIND11_CPP_STANDARD ${PYBIND11_CPP_STANDARD} CACHE STRING | ||
"C++ standard flag, e.g. -std=c++11, -std=c++14, /std:c++14. Defaults to highest supported for CMake 2.8-3.7") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if nothing is specified and we are on CMake 3.8? Of course it would be good if the highest standard is used even there.
if(NOT CMAKE_VERSION VERSION_LESS 3.1) | ||
#Only provide the option if CMake >= 3.1 | ||
set(PYBIND11_CXX_FEATURES ${PYBIND11_CXX_FEATURES} CACHE STRING | ||
"List of compile features for PyBind, will use highest detected C++ version in CMake 3.8+") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyBidn -> pybind11
Also, can you clarify the interaction of this PR with #1089? |
This is a suggested implementation for #1097.
This causes pybind11 to use CMake's built-in compiler features feature if no
PYBIND11_CPP_STANDARD
is set on CMake 3.8+. This integrates with the rest of the CMake infrastructure if the user wants to usecompiler_features
. It can also be added manually now. For example:In this example, scripting will be build with
-std=c++14
, and fooapi will be built with-std=c++11
. No flags will be duplicated, only one std flag will be listed, etc. Before the patch, the standard gets listed twice, would be included on C only compilation, etc.See below for a description of how the auto-discovery works.