Fix compilation of tests with /std:c++17

[YannickJadoul]

Seems it needs to be closed to get it passed correctly?

Closes #2089

So this first commit should fail, given #2089.

[YannickJadoul]

Am I going crazy? The debug runs print PYBIND11_CPP_STANDARD=/std:c++17 from CMake, but the actual compile command contains /std:c++14???

[YannickJadoul]

Seriously!?

https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html

$<COMPILE_LANGUAGE:languages>
...
Note that with Visual Studio Generators and Xcode there is no way to represent target-wide compile definitions or include directories separately for C and CXX languages. Also, with Visual Studio Generators there is no way to represent target-wide flags separately for C and CXX languages. Under these generators, expressions for both C and C++ sources will be evaluated using CXX if there are any C++ sources and otherwise using C. A workaround is to create separate libraries for each source file language instead:

[henryiii]

The correct solution probably would be to use compile_features; when we make this 3.7+ only we should be able to change this properly.

But yes, this is a general issue that always needs a workaround for these IDE's.

[YannickJadoul]

The correct solution probably would be to use compile_features; when we make this 3.7+ only we should be able to change this properly.

Yes, I had thought of that as well. But depends on dropping old CMake support, then.

But yes, this is a general issue that always needs a workaround for these IDE's.

Good point. I'll check if there's more of these around!

(Should I also check for Xcode, btw, or not?)

[henryiii]

If it doesn't work on Xcode, I'd likely know pretty quickly. ;)

I can't think of any more of these that are likely to be a problem, but I wouldn't have remembered the one above (I had to fix in in CLI11 a long time ago). I know of some that probably don't affect us (like the ... syntax in version support being broken in MSVC's forked copy of CMake 3.11)

[YannickJadoul]

Thanks! I found 2 more occurences of $<COMPILE_LANGUAGE:CXX>: one for the embedding target (same fix), and one here:

pybind11/tools/pybind11Tools.cmake

Line 257 in 8b9eb96

target_compile_options(${target_name} PRIVATE $<$<NOT:$<CONFIG:Debug>>:$<$<COMPILE_LANGUAGE:CXX>:/MP>>)

Not sure what the last one does, but it's already MSVC-specific. Any idea what to do? Just ignore, since it's only about parallel builds?

[henryiii]

Maybe tag it with a TODO? Is there a reason that cmake --build -j<N> doesn't work for parallel builds? Manually sticking specific compile options is usually a bad idea. And, due to the COMPILE_LANGUAGE, this one should be a no-op.

[YannickJadoul]

Maybe tag it with a TODO? Is there a reason that cmake --build -j<N> doesn't work for parallel builds? Manually sticking specific compile options is usually a bad idea. And, due to the COMPILE_LANGUAGE, this one should be a no-op.

No clue. Never really touched the CMake internals before. I'll just apply the same fix anyway. If someone passes by that knows more about CMake, we can still change it? :-)

[YannickJadoul]

Builds are still using /std:c+14!? Am I somehow going crazy? I thought I had found the issue :-(

[bstaletic]

I know the title says /std:c++17, but could we maybe include this as well: #2159

[YannickJadoul]

I know the title says /std:c++17, but could we maybe include this as well: #2159

Yes, good point. I was planning to see if supporting VS 2019 would still be possible, but this is also something to add!
Once I get things to fail, that is ...

[LeslieGerman]

@YannickJadoul

Thanks! I found 2 more occurences of $<COMPILE_LANGUAGE:CXX>: one for the embedding target (sa
target_compile_options(${target_name} PRIVATE $&lt;$<NOT:$CONFIG:Debug>:$<$<COMPILE_LANGUAGE:CXX>:/MP>>)
Not sure what the last one does ...

/MP: Build with Multiple Processes

According to my experiences in previous years, it may or may not improve compilation time. See also MS own Guideline.

If compilation speed is not a key factor here, you can safely omit this option IMO.

[YannickJadoul]

/MP: Build with Multiple Processes

Yes, that I can look up. My question was just wondering out loud why it's in pybind11's CMake and why not in debug, etc. (Which I might be able to look up through some git blame, yes.)

[bstaletic]

What's the status of this? Are we just going to mandate /permissive-?

[YannickJadoul]

Yes, but I haven't gotten to cleaning up this PR and adding it to the docs. Feel free to take over, if you want?