-
Notifications
You must be signed in to change notification settings - Fork 45
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
Set up node-pre-gyp and appveyor #21
Conversation
Awesome, thanks for the contribution! I will look over this in more detail, and try running it through AppVeyor, in the next day or two. |
@@ -4,7 +4,8 @@ | |||
"description": "Use the Windows.Devices.Bluetooth UWP API directly from Node.js", | |||
"main": "lib/main.js", | |||
"scripts" : { | |||
"install" : "node-gyp rebuild --msvs_version=2015" | |||
"prepublish": "npm ls", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prepublish script npm ls
doesn't serve any purpose, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's purely optional, just to see if the right node-pre-gyp is installed (node-pre-gyp wiki)
@@ -4,7 +4,8 @@ | |||
"description": "Use the Windows.Devices.Bluetooth UWP API directly from Node.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be doing something different with the version number of these UWP packages. Should it match the root package version? Or should it match the version of the Windows SDK that the code was generated from? (Something like 10.15063.x) Probably it doesn't really matter since these packages are not published to npm.
appveyor.yml
Outdated
|
||
matrix: | ||
# node 4 x64 works, but x86 fails | ||
#- node_version: '4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the problem with Node v4 x86? Some people apparently would like to use Node v4: #18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails to build, i don't know what's the issue: build log
appveyor.yml
Outdated
# node 4 x64 works, but x86 fails | ||
#- node_version: '4' | ||
- node_version: '5' | ||
- node_version: '6' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about Node 7 and 8? I expect those should work just the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried them they work as expected!
The Node 4 x86 issue is mapbox/node-pre-gyp#209 The comments there suggest that upgrading node-gyp should fix the problem, however our build does upgrade to 3.6.2 which is supposedly new enough. So I applied the other workaround mentioned there, and then the build succeeded. I'm going to merge this now, along with that workaround. |
AppVeyor build for |
Great, are there any issues left that are blocking the integration in to noble (noble/noble#550)? |
This adds node-pre-gyp and node-pre-gyp-github to publish prebuilt binaries via Github releases.
An appveyor.yaml is also included to automate builts.
Steps to automate builds using appveyor:
Fixes #13