target_include_directories use -i instead of -isystem

[Sean-Der]

So this is a repeat of #4 but I will make sure to follow up.

So currently when I build I get a bunch of warnings from include/v8.h (which obscures warnings that are actually my fault) I am using set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -pedantic -Wextra")

node/v6.7.0/include/v8.h:4886:20: warning: unused parameter 'isolate' [-Wunused-parameter]
       v8::Isolate* isolate, v8::Local<v8::String> name) {
                    ^
node/v6.7.0/include/v8.h:4886:51: warning: unused parameter 'name' [-Wunused-parameter]
       v8::Isolate* isolate, v8::Local<v8::String> name) {
                                                   ^
node/v6.7.0/include/v8.h:5373:50: warning: unused parameter 'string' [-Wunused-parameter]
   virtual void VisitExternalString(Local<String> string) {}
                                                  ^
node/v6.7.0/include/v8.h:5383:57: warning: unused parameter 'value' [-Wunused-parameter]
   virtual void VisitPersistentHandle(Persistent<Value>* value,
                                                         ^
node/v6.7.0/include/v8.h:5384:47: warning: unused parameter 'class_id' [-Wunused-parameter]
                                      uint16_t class_id) {}
                                               ^
node/v6.7.0/include/v8.h:7391:55: warning: unused parameter 'isolate' [-Wunused-parameter]
   V8_INLINE static void CheckInitialized(v8::Isolate* isolate) {

If SYSTEM was added to https://github.com/cjntaylor/node-cmake/blob/dev/NodeJS.cmake#L602 it would fix it for me. Is there another way I can/should be doing this?

[cjntaylor]

I've been thinking about this for a while, because I'm not sure exactly how I want to handle it. At a minimum, here are my current thoughts:

• The re-written release of node-cmake assumes that NodeJS.cmake gets copied to the root of your project (peer to CMakeLists.txt) at some point (I recommend initially and then every time node-cmake is updated). I provide the command ncmake update to make this easier / automatable if desired. However, unless you configure otherwise, this is a one time copy. You can always edit NodeJS.cmake for your specific project if you need specific configurations. However, its then your burden to handle conflict resolution if you want to update to the newest release of node-cmake. Generally speaking, this is how I have people test changes to the code, and I try to incorporate as many as possible so that the burden is minimized.
• As before, I sort of feel like you're trying to have your cake and eat it too. You've turned on -Wall -pedantic -Wextra, and are complaining that, as you've requested, its throwing lots and lots of warnings to the console. Since this is only an issue when you have this extreme of a warning flag enabled, and it won't affect most users, I'm hesitant to change things. (Also, for what its worth, manually modifying ${CMAKE_CXX_FLAGS} is awfully non-portable. Have you looked for a way to add the warning flags without the call to set()? Maybe compile features? At least guarded by os, e.g. if(NOT WIN32)...?).
• Compounding this, my concern would be that adding SYSTEM could unknowingly mask an error deep in the node/v8 sources. If I understand what SYSTEM would change, if I messed up a templated V8 function, or a call to the node defintions, etc, the warning / error would be omitted since technically the origin of the code that caused the problem is within a path guarded by SYSTEM. I don't like the idea of having side effects like this where an end user without your specific use case could get bitten by not knowing some subtle warning was squashed by this flag.

All this said, I'm more than willing to add SYSTEM permanently to the NodeJS.cmake file. I just want to make sure I can adequately address these concerns in a portable, general way for my users.

In other words, please prove me wrong. In the mean time, a manual patch of NodeJS.cmake ought to handle your issue.

[Sean-Der]

I am going to close this, I solved this locally. If anyone is still hitting this issue you can do this after your call to add_nodejs_module

target_include_directories(${PROJECT_NAME} SYSTEM BEFORE PUBLIC ${NODEJS_INCLUDE_DIRS})