Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

gyp: Enabling /ZW compile option #26

Closed
wants to merge 1 commit into from
Closed

gyp: Enabling /ZW compile option #26

wants to merge 1 commit into from

Conversation

munyirik
Copy link

The change enables addons to compile with the /ZW (CompileAsWinRT) option when using node-gyp. e.g. if you have something like this in binding.gyp:

'targets': [
{
'target_name': 'foo',
'sources': [
'bar.cpp',
],
'msvs_settings': {
'VCCLCompilerTool': {
'CompileAsWinRT': 'true',
},
},

The change in MSVSSettings.py gets rid of the warning below:
Warning: unrecognized setting VCCLCompilerTool/CompileAsWinRT while converting to MSBuild.

The change in msvs.py gets rid of the error below:
TypeError: Appending "false" to a non-list setting "CompileAsWinRT" for tool "ClCompile" is not allowed, previous value: true

@thefourtheye
Copy link
Contributor

The dependency changes are normally sent to the upstream project and we pull from that. @nodejs/node-chakracore thoughts?

@orangemocha
Copy link
Contributor

Agreed. @munyirik , please refer to nodejs/node-gyp#873 (comment)

@kunalspathak
Copy link
Member

chakracore-master already has gyp changes that needs to be send upstream. We have tracking issue for it at #21. We can take this patch and then include it in the PR that we will send upstream.
Thoughts?

@thefourtheye
Copy link
Contributor

In Nodejs core, we sometimes float a patch if it is absolutely necessary to move forward.

If we start accepting dep changes then we may have to maintain them and deliver it to upstream. If upstream doesn't take it then we ll have a diverged project as a dependency and the complete maintenance will be on us. We wouldn't want to spend our energy this way, would we? :)

@munyirik
Copy link
Author

Makes sense. I'll follow the instructions to get it in the gyp project.

@munyirik munyirik closed this Feb 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants