Skip to content

Fix CMake scripts so projects using CUDA .cu files build correctly. #1441

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

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

davisking
Copy link
Contributor

This PR makes a few minor changes to the CMake scripts so projects using CUDA .cu files build correctly. For example, CMake allows you to use CUDA files in a python extension module with a CMakeLists.txt such as:

CMAKE_MINIMUM_REQUIRED(VERSION 3.11)
project(pystuff LANGUAGES CXX CUDA)

add_subdirectory(pybind11)

pybind11_add_module(pystuff 
   cpp_stuff.cpp 
   cuda_stuff.cu
   )

That works fine on Linux with the current pybind11 cmake scripts. However, it fails on OS X and windows with Visual Studio. On OS X it fails because of incorrect link visibility settings for the CUDA code. In Visual Studio it fails because the current CMake scripts end up passing /MP and /bigobj to nvcc, but nvcc doesn't understand those options and so it errors out.

This PR fixes those issues. Allowing CMake projects such as the one shown above to compile on all three platforms.

And as an aside, pybind11 is great. I switched dlib to it from boost python and my life is much improved :)

@jagerman
Copy link
Member

Seems a reasonable fix to me. One question about:
if(CMAKE_VERSION VERSION_LESS 3.11)
this seems like a very recent version; does it need to be that recent?

@davisking
Copy link
Contributor Author

I'm not really sure. You need a pretty new version of CMake to compile .cu files in the way I posted earlier, so I don't think you can go much farther back. However, I'm not sure where the exact version threshold is.

@jagerman
Copy link
Member

Sure, but I think the actual cmake code itself should work fine when not doing CUDA compilation -- in other words, we can simplify to avoid the if/else branch assuming that the target_compile_options(${target_name} PRIVATE $<$<COMPILE_LANGUAGE:CXX>:${msvc_extra_options}>) generator expression works in older cmake versions.

@davisking
Copy link
Contributor Author

Right, it doesn't though. I tried a few older versions and cmake errors out since it can't handle the generator expression there.

@jagerman
Copy link
Member

Aha, okay, in that case leave it at 3.11. From quick googling it looks like the CUDA first-class language support was added over 3.8--3.10 with the support in earlier cmake versions being rather incomplete/buggy. If someone really wants to be able to use older cmake on Windows for CUDA compilation they can work out a more precise version.

@davisking
Copy link
Contributor Author

Yep, my thoughts exactly :)

@henryiii
Copy link
Collaborator

henryiii commented Aug 7, 2018

with the support in earlier cmake versions being rather incomplete/buggy. If someone really wants to be able to use older cmake on Windows for CUDA compilation they can work out a more precise version.

Agreed. And this would be great to have, I've been manually working around this for quite some time.

And, on macOS and Windows, it's easy to keep the CMake version up to date. (Technically, it is easy on Linux too, but since many Linuxes already have old CMake's, a new version is often not added by hand)

@davisking
Copy link
Contributor Author

Right. So I think this is good to merge, no?

@wjakob
Copy link
Member

wjakob commented Sep 14, 2018

Looks good, thank you!

@wjakob wjakob merged commit 9343e68 into pybind:master Sep 14, 2018
@davisking
Copy link
Contributor Author

davisking commented Sep 15, 2018 via email

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