Node/Nan headers are not included with SYSTEM tag which does not play nice with -Weverything

[mbronk]

FindNodeJS.cmake has

target_include_directories(${NAME} 
        PUBLIC ${NodeJS_INCLUDE_DIRS}
        PUBLIC ${NAN_PATH}
    )

while should have

target_include_directories(${NAME} SYSTEM
        PUBLIC ${NodeJS_INCLUDE_DIRS}
        PUBLIC ${NAN_PATH}
    )

[cjntaylor]

Neither of these two locations are system folders for most use cases, when node headers are downloaded directly. This is particularly true for NAN, which is always included local to your project by downloading via NPM (either as a direct dependency of your project, which takes priority, or falling back to the dependency of this node module).

Can you give an example of the error your seeing, and some specifics on the platform/architecture/compiler you're using?

[mbronk]

Per https://cmake.org/cmake/help/v3.0/command/target_include_directories.html

If SYSTEM is specified, the compiler will be told the directories are meant as system include directories on some platforms (signalling this setting might achieve effects such as the compiler skipping warnings, or these fixed-install system files not being considered in dependency calculations - see compiler docs). If SYSTEM is used together with PUBLIC or INTERFACE, the INTERFACE_SYSTEM_INCLUDE_DIRECTORIES target property will be populated with the specified directories.

In other words, in my mind SYSTEM does not imply much about where the files are (they very much can be local to this project) but that they are external to the thing I am building and are provided by a 3rd party.
E.g. see this: http://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-in-system-headers

Now to the concrete example - I don't have any because I was just playing around with node-cmake, but was using clang 3.5 on Ubuntu14.
But - more importantly I had the following line in my CMakeLists:

SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Werror -Weverything")

---

As a side note - when using clang, you pretty much force usage of -stdlib=libc++, which is not configurable (but I guess it could be - I didn't want it and had to override it for my use case by doing some wizardry on the flags)

[cjntaylor]

I'm aware of the CMake documentation; thats not what I was asking. What that says to me is that its compiler specific, and prone to producing arbitrary side effects ("see compiler docs" is a scary postfix). I've never needed or used the SYSTEM specifier across all of the CMake projects I build. Your specific use case with -Weverything requires this, but what about the other platforms I support? Windows/MSVC? GCC? Apple Clang on OSX? Cross compiles (Android/ARM)? This gets complicated fast.

What about your usecase requires -Weverything? I'd expect this to fail for all sorts of reasons, ones that are out of my control. In particular, clang is a "standards-compliant" compiler with very few exceptions. Most code targets GCC or MSVC which are anything but this, supporting only a tiny (and frustratingly different) cross-section of the respective C++ standards. At the end of the day, you're compiling against at least header code provided by Node, which is unlikely to meet the standard. "Ignoring" this by abusing the SYSTEM header specification seems like a bad idea. I'd rather try to understand why its not working, and fix that.

What I was partially after was what kinds of errors do you get due to -Weverything, and where? I wanted to get an idea of scope to see how deeply this is a problem.

The standards compliance of libc++ was primarily why it was coded that way, as modern node requires a huge cross-section of C++11 support, and it tends to be far simpler to use their c++ implementation than others. Can I ask what c++ library you're linking against?

All this said, I'll work on tuning libc++ to be optional (its required under OSX, but I'll make it a "default" outside of this). Would an argument to the add_nodejs_module be sufficient for the SYSTEM specifier? Basically, you'd just specify "SYSTEM" after the name and it would configure the headers this way?

[cjntaylor]

No response, closing.