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

Support external include directories with MSVC #6645

Closed
tru opened this issue Mar 9, 2020 · 4 comments
Closed

Support external include directories with MSVC #6645

tru opened this issue Mar 9, 2020 · 4 comments
Assignees

Comments

@tru
Copy link
Contributor

tru commented Mar 9, 2020

In MSVC 2015 microsoft added support for "external include directories" these works similar to isystem with GCC/Clang - meaning that you can set an separate warning level on the source files in these directories.

This is done by enabling the feature:
/experimental:external

Then including the directories with /external:I <path>

Then you can set your own warning level on this with /external:WX

This would be awesome if it was supported by conan. Since it's experimental and only works on 2015 and up we probably would need to make it opt-in, maybe by setting a variable like CONAN_MSVC_EXPERIMENTAL_EXTERNAL_INCLUDES and then also checking CMAKE_SYSTEM_INCLUDES.

This could all be handled in cmake_common.py and we could just pass these files as INTERFACE_COMPILE_OPTIONS instead of INTERFACE_INCLUDE_DIRECTORIES.

The best option I think would be if upstream CMake supported this, but this seems unlikely since there hasn't been much movement on the bug upstream: https://gitlab.kitware.com/cmake/cmake/issues/17904

Here is the microsoft blog post about it: https://devblogs.microsoft.com/cppblog/broken-warnings-theory/

@memsharded memsharded self-assigned this Mar 9, 2020
@memsharded
Copy link
Member

To be honest, I am a bit reluctant to workaround the lack of support of this feature in CMake, because we might break things in other ways too. My feeling is that:

  • Changes are not only in cmake_common, but also the cmake_find_package generators are become more relevant. Solutions should cover both.
  • We need to make sure to apply it only for the right VS2015, there are chances that we introduce bugs.
  • This should be configurable at the user level, it seems there are several different warnings to be applied, and it is likely that this is a case of one size does not fit all. While it is a bit more natural for the cmake generators, because you can pass things as arguments to conan_basic_setup(), I am not very happy to make the cmake_find_package generated scripts to change their behavior based on some existing variables.
  • That definition of target_compile_options() irks me. Reads too much of a workaround to modify the way conan plugs dependency information.

While I totally understand the value of this feature, I am not sure, it seems a bit too problematic for the value. This is not even a fully solved issued for the existing isystem functionality, CONAN_SYSTEM_INCLUDES only works for the cmakegenerator in old-legacy global CMake mode, not even with CMake targets. I wouldn't consider to add this, until the isystem works reliably in other compilers and generators.

@tru
Copy link
Contributor Author

tru commented Mar 10, 2020

Yeah I agree - maybe it's time to look at the CMake source code then.

@tru
Copy link
Contributor Author

tru commented Mar 10, 2020

Btw we recently had to have a switch for turning isystem on and off and we used CMAKE_NO_SYSTEM_FROM_IMPORTED and it solved the problem just fine.

@memsharded
Copy link
Member

Closing as outdated

@memsharded memsharded closed this as not planned Won't fix, can't repro, duplicate, stale Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants