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

Always specified CXX as language. #1027

Merged
merged 1 commit into from
Aug 30, 2017
Merged

Always specified CXX as language. #1027

merged 1 commit into from
Aug 30, 2017

Conversation

apollo13
Copy link
Contributor

there should be no need to test for a C compiler since pybind only uses CPP

@jagerman
Copy link
Member

It's correct, in theory, but as you can see from the travis-ci builds, it causes some issues with older (but supported) versions of cmake.

@apollo13
Copy link
Contributor Author

This new patch should fix that, not sure if it is worth it. (I'd like to see it because I do not really need a C compiler and the checks sometimes cause more problems…)

@apollo13 apollo13 force-pushed the cmake branch 2 times, most recently from adadd80 to 82917f5 Compare August 24, 2017 15:54
@dean0x7d
Copy link
Member

Well, this does shave off a bit of time of the CI builds (especially AppVeyor) which is nice. But I haven't really been using the language settings in CMake, so I have a couple of questions:

  1. What's actually the issue with CMake < 3.4?
  2. Would this cause any issues for mixed C and C++ parent projects? (I would guess not, but just want to make sure we don't break anyone's build.)

@apollo13
Copy link
Contributor Author

What's actually the issue with CMake < 3.4?

The issue is that try_compile on cmake < 3.4 does not work if you have no C compiler, later version happily use the C++ compiler which works just fine to test features (especially if you target CPP anyways).

Would this cause any issues for mixed C and C++ parent projects? (I would guess not, but just want to make sure we don't break anyone's build.)

This should not cause issues for parent or child projects. The final supported languages basically are an union of all defined ones. There might be the weird situation where a parent project specified only CXX but relied on a C compiler to be present and usable… I think this case should not be a concern for pybind itself though (also by default cmake defaults to C & CXX, so that project would have to actively set CXX only…)

@henryiii
Copy link
Collaborator

I believe @apollo13 is correct; the same thing happens if you are specifying CUDA as a language (CMake 3.7+); the final language is a union of the defined languages.

Older versions of CMake didn't take into account that someone might be only compiling CXX, so modules (notoriously the pthread module, for example) didn't work without C specified. That's been fixed in newer CMake versions.

So I don't think there is any downside to avoiding the C specification in PyBind11 for newer CMake versions.

@dean0x7d
Copy link
Member

OK, that seems good to me. @apollo13 Could you just add a very brief comment to CMakeLists.txt to explain the CMake < 3.4 check?

@dean0x7d
Copy link
Member

Thank you, @apollo13.

@apollo13 apollo13 deleted the cmake branch August 30, 2017 12:20
@dean0x7d dean0x7d modified the milestone: v2.2 Aug 30, 2017
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