add_module: include as SYSTEM

[ax3l]

pybind11 headers passed via the pybind11_add_module CMake function can now be included as SYSTEM includes (-isystem).

This allows to set stricter (or experimental) warnings in calling projects that might throw otherwise in headers a user of pybind11 can not influence. Examples are -Wpedantic#1417 and -Wshadow#1267.

Additionally, this change uncovered that builds against a python debug library were unreliable. Setting the Py_DEBUG define is necessary when linking against a debug build (ref): #1438

[ax3l]

@chuckatkins any thoughts on this one?

It's unfortunate the debug build has some weird symbol issue...

[chuckatkins]

So, this is a weird one. There's a good detailed explanation of this happening with boost::python here https://stackoverflow.com/questions/39161202/how-to-work-around-missing-pymodule-create2-in-amd64-win-python35-d-lib . The short answer is, that's what the python debug library does. It's by design. From looking in the headers, I believe you need to add -DPy_DEBUG when building against a debug python library.

[chuckatkins]

It looks like the intent of this is to purposely break ABI compatibility between the python debug and release libraries for modules and extensions.

[ax3l]

looks like the intent of this is to purposely break ABI compatibility

That's what I found from googling as well, but I did not see the flag. Let's try it!

[chuckatkins]

It's probably best suited to be added in pybind11_add_module via target_compile_definitions(Foo PRIVATE Py_DEBUG) when using a debug libpython.

[ax3l]

Yay, works! Thanks, Chuck!

[ax3l]

@jagerman fix ready for reliable linking to python debug libs and addresses two issues with strict(er) warning flags in downstream projects

[wjakob]

Hm, I'm not so excited about this patch. "-Isystem" is meant for standard system include directories, which is probably not true in many cases.

If you take a look at the current header files, you will see that they already turn off a bunch of warnings before including the Python header files, and it would be fine to add more of them if needed. It's not pretty, but this at least ensures that there is some consistency across build systems (CMake is just one of many build systems that can be used with pybind11.).

A side note: this PR also contains a change to deal with Debug python libraries. This is not related to the change about warnings and would be great to have as a separate PR.

[ax3l]

Thanks for taking a look!

As far as I am aware, -isystem includes are and should be used in cases where you want to avoid throwing warnings from headers one can not control as a user. This is exactly the case with the pybind headers when used downstream which add_module provides a convenience header for.

Maybe @chuckatkins can comment on that, but I think this is common practice, properly modularizes compiler flags and testing (e.g. no noise from dependencies) and has no downsides (e.g. it falls back to -I when compiler support is missing).

turn off a bunch of warnings before including the Python header files, and it would be fine to add more of them if needed

I respectfully disagree with that approach. I think for example, that adding more INTERFACE compile flags is contra-productive: I want to test my binding code for -Wshadow, -pedantic or anything exotic that one experiments with. Disabling those on whole translation units just because I add pybind11 headers will prevent downstream users to have simply different policies (on warnings) than their dependencies.

this PR also contains a change to deal with Debug python libraries. This is not related to the change about warnings and would be great to have as a separate PR.

yes it does contain an additional fix that turned up and I will provide it again in a separate PR: #1438
Just be aware that I can not demonstrate it rigorously in the current CI context without the other changes, that's why I added them here for demonstration. But now that we saw them and found the origin, that should be fine. I will rebase as soon as #1438 is merged.

[chuckatkins]

I think this is common practice

People do it but it's definitely not common practice. Well established behavior is to not use SYSTEM by default. It's the same behavior seen where PUCLIC is used by default in target_ commands if no PUBLIC|PRIVATE|INTERFACE is specified. So I see no issue with having the option in the function but its use should be inverted, i.e.

[NO_EXTRAS][SYSTEM][THIN_LTO]source1[source2...])

This would align with the syntax for SYSTEM used by target_link_libraries and taget_include_directories and be consistent with general cmake practices of public by default but private if specified.

[ax3l]

Sounds perfect to me, will adopt accordingly!

[ax3l]

@wjakob does this work? The patch now keeps the default includes and allows to include with -isystem when explicitly requested.

[jagerman]

I respectfully disagree with that approach. I think for example, that adding more INTERFACE compile flags is contra-productive: I want to test my binding code for -Wshadow, -pedantic or anything exotic that one experiments with. Disabling those on whole translation units just because I add pybind11 headers will prevent downstream users to have simply different policies (on warnings) than their dependencies.

It isn't disabled on the whole translation unit, but merely within the pybind headers (via a #pragma GCC diagnostic push/pop or equivalent for other compilers) for known compiler false-positives within pybind code. Some of these are disabled across the whole pybind code, some are much more localized, but all of the diagnostic changes are popped off at the end of the pybind11 header.

[ax3l]

Thank you for the info, that's right!

Since add_module basically wraps a target_include call (among others), I still think that it's useful to expose this option as proposed to the interested user. This is useful in scenarios such as experimenting with new gcc releases or exotic compilers (let's say PGI or XL which are common in HPC) which are unlikely to be mainlined as well in the same fashion.

[ax3l]

@jagerman@wjakob PR updated with comments - now implemented as opt-in feature as it's common in CMake (public by default)

[ax3l]

@wjakob added your review comments, kept the default and made it optional

[wjakob]

Looks good, thank you!