Introduce include_dir for use with gyp in a scalar context

[lovell]

This change reverts PR #757, adds a new include_dir scalar property and suggests the deprecation of the ambiguous include string-as-array property.

This is required due to the currently-documented approach of using the single include path in an array/list context instead of a scalar context with gyp.

-  'include_dirs': ["<!@(node -p \"require('node-addon-api').include\")"],
+  'include_dirs': ["<!(node -p \"require('node-addon-api').include_dir\")"],

The use of a gyp list/array context just happens to work when a path is absolute, hence no one spotting this ambiguity before, but shell expansion can fail on Windows when paths are relative and a gyp file contains multiple entries in its include_dirs directive.

This problem, which only affects v3.0.1 on Windows, will manifest itself as fatal error C1083: Cannot open include file: 'napi.h': No such file or directory.

This PR provides an overall approach that is more consistent with that used by nan - https://github.com/nodejs/nan#usage

"include_dirs" : ["<!(node -e \"require('nan')\")"]

Modules wishing to add support for filesystem paths containing whitespace will need to use this new include_dir property in the correct and newly-documented gyp scalar context.

[mhdawson]

@lovell, did we just break windows for existing users with 3.0.1 or just not fix it on windows?

[lovell]

node-addon-api v3.0.1 breaks node-gyp based installation for Windows users where a module that depends on it has defined multiple include_dir entries. Apologies for not picking this up in my testing.

As far as I can tell, the ability to support both filesystem paths containing whitespace and Windows users isn't possible with the <!@(... approach, hence this PR to revert and provide backwards compatibility via a new property to use with the preferred <!(... approach.

[mhdawson]

@lovell understood, thanks for the quick reply.

Would it be possible to add some tests to this PR which will test the broken case as well as the ones you were fixing? That way our CI will be able to validate it works across platforms (we have windows, linux etc. in that) My bad for not looking for tests in the first PR as we probably would have caught the issue.

@NickNaso we should get some messaging out to tell people not to use 3.0.1 and that a 3.0.2 will be coming soon. I think we should try to get a 3.0.2 out by end of next week which either reverts the breaking change or incorporates this PR if it is ready/landed.

[mhdawson]

@NickNaso I also meant to ask if you wanted to send out the messaging via twitter and I think also a message on the README.md (if so let me know when you do and I'll retweet what you send out) or if you'd like somebody else to take a look at doing that?

[lovell]

I notice there's a Windows CI environment defined in an Appveyor config file, but this isn't run for PRs and doesn't appear to actually work as-is.

I attempted to modernise it but there is a compilation failure in the existing tests:

  addon_data.cc
  arraybuffer.cc
..\addon_data.cc(71): error C2248: 'Napi::Env::DefaultFini': cannot access private member declared in class 'Napi::Env' [C:\projects\node-addon-api\test\build\binding.vcxproj]
  c:\projects\node-addon-api\napi-inl.h(357): note: see declaration of 'Napi::Env::DefaultFini'
  C:\projects\node-addon-api\napi.h(168): note: see declaration of 'Napi::Env'

Is there an example of a successful Windows CI config/build/log as I can't quite work out how the tests are currently passing on Windows?

UPDATE: Appveyor was defaulting to VS2015. Updating to VS2017 allows the tests to compile - I'll submit a separate PR to re-enable the use of Appveyor testing.

[mhdawson]

The regular CI runs are here: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-LTS%20versions/

But they use our jenkins infra as opposed to an external service.

[mhdawson]

With this as a windows example: https://ci.nodejs.org/job/node-test-node-addon-api-new/nodes=win-vs2019/2465/console, but seems you'd already figured out the issues, and @lovell thanks for looking at fixing up that file in a separate PR.

[mhdawson]

@lovell are you working on some tests? @NickNaso mentioned he might be able to add some tests to the PR if you were not able to get to it.

[lovell]

I had a quick look at how we might be able to reproduce this using the existing tests but haven't spotted an obvious solution yet. Nick, if you have an idea then please go ahead.

The underlying cause is a gyp bug/feature relating to path-mangling on Windows (in my experience >80% of gyp problems affect only Windows). Passing it an include_dirs value of ..\node_modules\node-addon-api in an array context appears to result in a mis-conversion to /I"..\node_modulesnode-addon-api".

[NickNaso]

Hi everybody,
I have some ideas and I will work on that in this weekend.

[mhdawson]

@NickNaso just wondering if you've had a chance to take a look at this one?

[NickNaso]

@mhdawson sorry for the delay I would complete the work in this weekend.

[mhdawson]

@NickNaso thanks.

[mhdawson]

Update from @NickNaso through email, -> just back from vacation, working on it.

[mhdawson]

LGTM, tests are going to be added through -> #808

[mhdawson]

Landed as f27623f

[lovell]

Thanks for merging, thank you both for your continued maintenance of this module, and with apologies for temporarily breaking things for Windows users.