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

Fix cmake cxx standard definition #1089

Closed
wants to merge 2 commits into from
Closed

Fix cmake cxx standard definition #1089

wants to merge 2 commits into from

Conversation

ppwwyyxx
Copy link

@ppwwyyxx ppwwyyxx commented Sep 15, 2017

This compile option is only for cxx.
The current code will cause problem when compiling a c/cpp mix project. The -std=c++11 flag will be passed to c obj as well. This change fixes the issue.

The compile option is only for cxx.
The current code will cause problem when compiling a c/cpp mix project. The -std=c++11 flag will be passed to c obj as well. This change fixes the issue.
ppwwyyxx added a commit to facebookresearch/ELF that referenced this pull request Sep 15, 2017
@henryiii
Copy link
Collaborator

henryiii commented Sep 15, 2017

This feature was added in CMake 3.3. You'll need to wrap it with a version check or talk someone into dropping CMake < 3.3 support.

Additionally, it doesn't work with Visual Studio (see here), so that would also need to be checked for. I'd also verify it's only the Visual Studio generators and not all IDE generators...

Are you sure it's not better just to compile separately and link?

(note to devs: libraries are beginning to require 3.0-3.4, such as TBB (3.0), ROOT (3.4.3), etc., so not a ridiculous idea to consider for the future. CMake can be installed in one line, too.)

@henryiii
Copy link
Collaborator

A potentially better solution would be to use one of the tools CMake gives for C++ standards, but those require CMake 3.3 or so.

@henryiii
Copy link
Collaborator

Checked it on Xcode, seems to work fine.

@henryiii
Copy link
Collaborator

I've a better idea; how about checking the CMake version then doing the right thing for that version? IE, the old behavior for older CMake's, and adding the target_compile_features cxx_std_14 (or 11) instead? That has the added benefit of playing nicely with CMake, if PyBind adds 14 and the user adds 17, for example, you'll only get 17 on the command line. Then your problem would be fixed if you use a recent CMake (install with 1 line).

@henryiii
Copy link
Collaborator

Let's open an issue, and discuss it there.

@jagerman
Copy link
Member

talk someone into dropping CMake < 3.3 support.

Not likely to happen; there are still lots of people around using ubuntu trusty (cmake 2.8.12/g++ 4.8), which is essentially going to be the baseline until that hits EOL in around a year and a half. (As much as I've love to drop old cmake/g++ workarounds, that would impact a substantial number of pybind users).

I think it's more feasible that we make such a requirements bump as part of an eventual 3.0 release, at the same time that we strip out the deprecated parts of the API. (There's currently no timeframe for that--and no really pressing need to break the API).

@henryiii
Copy link
Collaborator

ubuntu trusty (cmake 2.8.12/g++ 4.8)

And CentOS 7, which is on most of my systems. I'm not recommending it be dropped, I just would be in favor of it if someone else was. ;)

The main problem is that because CMake has improved so much, and because C++11 support, CUDA support, targets, etc. has improved, that many packages are now requiring newer CMake, and all users simply have to install a new CMake. I believe part of the problem is that systems viewed CMake 3.0 like Python 3.0, which is not the case.

For completeness, since I keep mentioning that new CMake installs are easy:

pip install --user cmake

will give you a modern CMake on almost any system that supports pip. A local install of CMake on Linux can be obtained with

mkdir cmake && wget -qO- "https://cmake.org/files/v3.9/cmake-3.9.2-Linux-x86_64.tar.gz" | tar --strip-components=1 -xz -C cmake
export PATH=`pwd`/cmake/bin:$PATH

@henryiii
Copy link
Collaborator

henryiii commented Sep 19, 2017

PS: @ppwwyyxx Can you see if setting CMAKE_CXX_STANDARD fixes your issue? I think pybind11 might not add anything if you do that.

@ppwwyyxx
Copy link
Author

Our CMakeLists.txt does have CMAKE_CXX_STANDARD set before including pybind11.

@wjakob
Copy link
Member

wjakob commented Sep 11, 2018

@jagerman, @henryiii, @ppwwyyxx: So what's the status here? Shall I merge this given the constraint to maintain CMake < 3.x compatibility?

@henryiii
Copy link
Collaborator

I believe this is still fine for now to merge - it makes sure the C++ flags don't leak into CUDA or C if you have CMake 3.3+, and doesn't change anything otherwise. IDEs (Visual Studio and XCode) will want a somewhat newish version of CMake to use this, but they need a somewhat newish version anyway.

@wjakob
Copy link
Member

wjakob commented Jun 11, 2019

Superseded by #1678.

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.

4 participants