This repository has been archived by the owner. It is now read-only.

Node gyp addon #2337

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
7 participants
@andreasbotsikas

Node-gyp is a utility that helps the node module’s project generation.
Node-gyp assumes (at the moment) that it is located in node's tool directory in order to locate the deps folder (one folder up) where all the include files reside.

Usage

node-gyp [gyp-filename] [make]

  • gyp-filename: is the gyp file that will be processed. If not specified it tries to locate and use the module.gyp file.
  • make: this is a command that dictates node-gyp to build the module after generating the project files

node_module.gypi

This file contains the common target setting that have to be applied in any gyp file in order to generate a valid node module project for windows, mac and the linux platform. This gypi file is included automatically by the script using the -I directive in order to avoid having to specify the absolute path of the node_module.gypi file.

We could also auto include the node's common.gypi file to inherit the target_arch variable, the Debug/Release configurations etc...

sampleGypModule

A simple module to demonstrate how to create a gyp file for your modules.

andreasbotsikas added some commits Dec 15, 2011

Unfortunately gyp can not import files using variables. Thus the node…
…_module.gypi's location must be specified inside the module.gyp file.
Added a node-gyp.Readme.txt file.
Fixed a minor path bug that occurred if you haven't added the trailing / in the environment variable.
@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 15, 2011

I'm interested in the concept. Even the name I like.

But I dislike the fact that these scripts depend on the node source tree. If this script is to be a replacement for node-waf then it needs to (IMO):

  1. Not require a NODE_ROOT env var. That seems unnecessary when we can do something at install-time to help this.
  2. Not depend on the full node source tree. The make install step should install the necessary files for this to work.

Anyways, that's my 2c. We should be going for the total replacement of node-waf.

I'm interested in the concept. Even the name I like.

But I dislike the fact that these scripts depend on the node source tree. If this script is to be a replacement for node-waf then it needs to (IMO):

  1. Not require a NODE_ROOT env var. That seems unnecessary when we can do something at install-time to help this.
  2. Not depend on the full node source tree. The make install step should install the necessary files for this to work.

Anyways, that's my 2c. We should be going for the total replacement of node-waf.

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 15, 2011

Another thing that may be cool is if the node-gyp script were written in JS itself, such that it could be executed on both Windows and Unix platforms. This eliminates the need to maintain two scripts (node-gyp.sh and node-gyp.bat).

Another thing that may be cool is if the node-gyp script were written in JS itself, such that it could be executed on both Windows and Unix platforms. This eliminates the need to maintain two scripts (node-gyp.sh and node-gyp.bat).

@TooTallNate

View changes

tools/node_module.gypi
+ 'uint=unsigned int', #Mac doesn't have uint either
+ ],
+ #MAC x64 users don't forget to comment out all line in
+ #gyp\pylib\gyp\generator\make.py that contain append('-arch i386') (2 instances)

This comment has been minimized.

@TooTallNate

TooTallNate Dec 15, 2011

There needs to be a better solution for this...

@TooTallNate

TooTallNate Dec 15, 2011

There needs to be a better solution for this...

This comment has been minimized.

@paddybyers

paddybyers Dec 16, 2011

Yes, it (or configure) should detect automatically. The issue is making sure that the addon is built with the same flags as node itself and ATM node isn't built with gyp, so gyp has to prescribe everything, instead of being able to assume that gyp just makes the same decisions both for node and the addon.

@paddybyers

paddybyers Dec 16, 2011

Yes, it (or configure) should detect automatically. The issue is making sure that the addon is built with the same flags as node itself and ATM node isn't built with gyp, so gyp has to prescribe everything, instead of being able to assume that gyp just makes the same decisions both for node and the addon.

@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 16, 2011

Is it possible to bundle the node.lib and the header files in the windows installer and add an environment variable to point to them? Moreover, we need gyp to be installed and added in the path.
As for the js solution, this can be easily done. A friend of mine suggested that I should go with python but I do agree that since node is all about javascript, we can easily go for it.
On the code review comment, are you refering to the Mac hack? I looked into gyp source (make.py is actually a huge string concatenator) and they have a comment that reads "TODO: Do not hardcode arch. Supporting fat binaries will be annoying."...
For visual studio, this is unfortunately the only way gyp supports setting the output directory (I looked into the source for a better solution but couldn't locate one).

Is it possible to bundle the node.lib and the header files in the windows installer and add an environment variable to point to them? Moreover, we need gyp to be installed and added in the path.
As for the js solution, this can be easily done. A friend of mine suggested that I should go with python but I do agree that since node is all about javascript, we can easily go for it.
On the code review comment, are you refering to the Mac hack? I looked into gyp source (make.py is actually a huge string concatenator) and they have a comment that reads "TODO: Do not hardcode arch. Supporting fat binaries will be annoying."...
For visual studio, this is unfortunately the only way gyp supports setting the output directory (I looked into the source for a better solution but couldn't locate one).

@paddybyers

View changes

tools/node_module.gypi
+ }],
+ [ 'OS=="mac"', {
+ 'defines': [
+ 'uint=unsigned int', #Mac doesn't have uint either

This comment has been minimized.

@paddybyers

paddybyers Dec 16, 2011

I don't think you need this; it must be an issue with the particular example addon. It's not needed for building on Mac generally.

@paddybyers

paddybyers Dec 16, 2011

I don't think you need this; it must be an issue with the particular example addon. It's not needed for building on Mac generally.

This comment has been minimized.

@TooTallNate

TooTallNate Dec 17, 2011

This was giving me problems too. I had to remove this line on my mac.

@TooTallNate

TooTallNate Dec 17, 2011

This was giving me problems too. I had to remove this line on my mac.

This comment has been minimized.

@andreasbotsikas

andreasbotsikas Dec 17, 2011

I removed the porting specific definitions I had added. It's probably better to have them in a wiki page (common porting issues like these).

@andreasbotsikas

andreasbotsikas Dec 17, 2011

I removed the porting specific definitions I had added. It's probably better to have them in a wiki page (common porting issues like these).

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 17, 2011

One more thought: the node_module.gypi file should IMO be included and implicitly by the node-gyp script, and packaged up by node. This file will include the stuff it currently contains, that would otherwise just be copy & pasted to each native module. This way the dev only has to maintain one gyp file per module.

So doing that makes it a 1:1 mapping from node-waf:

  • node-waf -> node-gyp
  • wscript -> module.gyp

One more thought: the node_module.gypi file should IMO be included and implicitly by the node-gyp script, and packaged up by node. This file will include the stuff it currently contains, that would otherwise just be copy & pasted to each native module. This way the dev only has to maintain one gyp file per module.

So doing that makes it a 1:1 mapping from node-waf:

  • node-waf -> node-gyp
  • wscript -> module.gyp
@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 17, 2011

@andreasbotsikas As an aside, I'm currently having trouble using a compiled binary (using this method) on a Windows machine that doesn't have VS C++ installed. The error is:

Error: Unable to load shared library C:\node_modules\weak\compiled\win32\ia32\weakref.node

Did you encounter this? And did you come across the solution? Thanks!

@andreasbotsikas As an aside, I'm currently having trouble using a compiled binary (using this method) on a Windows machine that doesn't have VS C++ installed. The error is:

Error: Unable to load shared library C:\node_modules\weak\compiled\win32\ia32\weakref.node

Did you encounter this? And did you come across the solution? Thanks!

node-gyp automatically includes the node_module.gypi file in every mo…
…dule.gyp. Updated sampleGypModule/module.gyp to demonstrate that there is no need to have a reference to the gypi file any more.
@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 17, 2011

Thanks both of you for your comments. I have updated the node-gyp to auto include the node_module.gypi file in every gyp file it processes. This solved my referencing problem because gyp doesn't support atm file inclusion using a variable for its location.
I have also updated the sample module.gyp file in the simple helloworld to demonstrate this change.

Thanks both of you for your comments. I have updated the node-gyp to auto include the node_module.gypi file in every gyp file it processes. This solved my referencing problem because gyp doesn't support atm file inclusion using a variable for its location.
I have also updated the sample module.gyp file in the simple helloworld to demonstrate this change.

@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 17, 2011

@TooTallNate I have seen this before on a virgin windows setup. Try installing Microsoft Visual C++ 2010 Redistributable Package (x86) (i guess you use a 32bit node version)

@TooTallNate I have seen this before on a virgin windows setup. Try installing Microsoft Visual C++ 2010 Redistributable Package (x86) (i guess you use a 32bit node version)

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 17, 2011

@andreasbotsikas Thanks, I'll try that out now. But how come node itself doesn't require this package (the one from the MSI)?

@andreasbotsikas Thanks, I'll try that out now. But how come node itself doesn't require this package (the one from the MSI)?

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 17, 2011

@andreasbotsikas And just to confirm, yes, installing that package did the trick for me on a clean Windows XP install.

I'm still curious as to if it could be avoided at all though.

@andreasbotsikas And just to confirm, yes, installing that package did the trick for me on a clean Windows XP install.

I'm still curious as to if it could be avoided at all though.

@arturadib

This comment has been minimized.

Show comment
Hide comment
@arturadib

arturadib Dec 18, 2011

This seems to be relevant for #2136

This seems to be relevant for #2136

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Dec 19, 2011

@andreasbotsikas This looks like a great start.

  • Can you move tools/sampleGypModule to test/cctest/sampleGypAddon?
  • tools/node-gyp should just be a python script and expect that it's being run from the root directory. I don't think we want to maintain separate bash and .bat scripts.
  • You need to agree to http://nodejs.org/cla.html before we can take the patch.

ry commented Dec 19, 2011

@andreasbotsikas This looks like a great start.

  • Can you move tools/sampleGypModule to test/cctest/sampleGypAddon?
  • tools/node-gyp should just be a python script and expect that it's being run from the root directory. I don't think we want to maintain separate bash and .bat scripts.
  • You need to agree to http://nodejs.org/cla.html before we can take the patch.
@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 20, 2011

@ry I have updated the sample location and signed electronicaly the cla. I do agree that we need to keep one file for both version. What's your opinion on running it through node.js as TooTallNate suggested instead of python?

@ry I have updated the sample location and signed electronicaly the cla. I do agree that we need to keep one file for both version. What's your opinion on running it through node.js as TooTallNate suggested instead of python?

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 20, 2011

Considering gyp is written in python, and the old node-waf was written in python, it seems to fit that this new node-gyp is written in python. It would be more difficult at least to have it be a node script (async and all that).

Considering gyp is written in python, and the old node-waf was written in python, it seems to fit that this new node-gyp is written in python. It would be more difficult at least to have it be a node script (async and all that).

@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 20, 2011

Ok, python it is. Should node-gyp produce xcode for mac or make (as it is for node atm)?

Ok, python it is. Should node-gyp produce xcode for mac or make (as it is for node atm)?

Changed node-gyp to python in order to run on both windows and linux/…
…mac from the same file. node-gyp.bat is a python wrapper. Added vsbuild.bat in order to build the gyp output using the visual studio tools.
@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 20, 2011

I rewrote the whole thing in python. This is actually my first python script, so feel free to comment on any language missuses I may have.
In this new version there is no need to have an environment variable, since the script tries to locate node.lib and the header files based on its own location.

TODO:

  • Check if os.system("make") works on linux
  • Check if we can have xcode instead of make for mac

I rewrote the whole thing in python. This is actually my first python script, so feel free to comment on any language missuses I may have.
In this new version there is no need to have an environment variable, since the script tries to locate node.lib and the header files based on its own location.

TODO:

  • Check if os.system("make") works on linux
  • Check if we can have xcode instead of make for mac

andreasbotsikas added some commits Dec 20, 2011

Added +x permission to node-gyp for linux. Fixed the readme in the sa…
…mple code. TODO: On node.js v0.7.0-pre, on linux, I get a segmentation fault when I try to load the helloworld.node.
@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 20, 2011

it seems that neither node-waf nor node-gyp can produce a valid native nodejs module on node v0.7.0-pre in linux... I get segmentation faults... Will look into it 2morrow.

it seems that neither node-waf nor node-gyp can produce a valid native nodejs module on node v0.7.0-pre in linux... I get segmentation faults... Will look into it 2morrow.

+ },
+ 'target_defaults': {
+ # Some default properties for all node modules
+ 'type': '<(library)',

This comment has been minimized.

@ry

ry Dec 21, 2011

This should be 'type': 'loadable_module'

@ry

ry Dec 21, 2011

This should be 'type': 'loadable_module'

+ 'PLATFORM="<(OS)"',
+ '_LARGEFILE_SOURCE',
+ '_FILE_OFFSET_BITS=64',
+ ],

This comment has been minimized.

@ry

ry Dec 21, 2011

Instead of doing the defines here, do

'includes': ['../common.gypi'],

which will include all the defines automatially

@ry

ry Dec 21, 2011

Instead of doing the defines here, do

'includes': ['../common.gypi'],

which will include all the defines automatially

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Dec 21, 2011

@andreasbotsikas i think the problem had to do with the visibility=hidden option that node was being compiled with. I went ahead and added the basis of the addon building system in 18b9220. There's still a lot to do

  • this only supports osx so far - we need windows
  • we need the test runner to build these
  • the binding.node gets placed into different directories depending on the OS

I should note that we don't think we will ever be distributing "node-gyp" as we have "node-waf". We will force addon builders to have a copy of the Node tree and we're looking into supporting binary downloads in npm.

Thanks for this patch - it was really helpful. I'm closing this pull request - please make changes to the current master.

ry commented Dec 21, 2011

@andreasbotsikas i think the problem had to do with the visibility=hidden option that node was being compiled with. I went ahead and added the basis of the addon building system in 18b9220. There's still a lot to do

  • this only supports osx so far - we need windows
  • we need the test runner to build these
  • the binding.node gets placed into different directories depending on the OS

I should note that we don't think we will ever be distributing "node-gyp" as we have "node-waf". We will force addon builders to have a copy of the Node tree and we're looking into supporting binary downloads in npm.

Thanks for this patch - it was really helpful. I'm closing this pull request - please make changes to the current master.

@ry ry closed this Dec 21, 2011

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 21, 2011

I don't see why we wouldn't want to distribute some replacement for node-waf. As a module author, I don't want to have to maintain both a binding.gyp file and wscript file...

Sent from my iPhone

On Dec 20, 2011, at 22:09, Ryan Dahlreply@reply.github.com wrote:

@andreasbotsikas i think the problem had to do with the visibility=hidden option that node was being compiled with. I went ahead and added the basis of the addon building system in 18b9220. There's still a lot to do

  • this only supports osx so far - we need windows
  • we need the test runner to build these
  • the binding.node gets placed into different directories depending on the OS

I should note that we don't think we will ever be distributing "node-gyp" as we have "node-waf". We will force addon builders to have a copy of the Node tree and we're looking into supporting binary downloads in npm.

I'm closing this pull request - please make changes to the current master.


Reply to this email directly or view it on GitHub:
#2337 (comment)

I don't see why we wouldn't want to distribute some replacement for node-waf. As a module author, I don't want to have to maintain both a binding.gyp file and wscript file...

Sent from my iPhone

On Dec 20, 2011, at 22:09, Ryan Dahlreply@reply.github.com wrote:

@andreasbotsikas i think the problem had to do with the visibility=hidden option that node was being compiled with. I went ahead and added the basis of the addon building system in 18b9220. There's still a lot to do

  • this only supports osx so far - we need windows
  • we need the test runner to build these
  • the binding.node gets placed into different directories depending on the OS

I should note that we don't think we will ever be distributing "node-gyp" as we have "node-waf". We will force addon builders to have a copy of the Node tree and we're looking into supporting binary downloads in npm.

I'm closing this pull request - please make changes to the current master.


Reply to this email directly or view it on GitHub:
#2337 (comment)

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Dec 21, 2011

@TooTallNate node-waf is going away

ry commented Dec 21, 2011

@TooTallNate node-waf is going away

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 21, 2011

??? /me confused
You previously said: "I should note that we don't think we will ever be distributing "node-gyp" as we have "node-waf""

??? /me confused
You previously said: "I should note that we don't think we will ever be distributing "node-gyp" as we have "node-waf""

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Dec 21, 2011

@TooTallNate this was clarified on the IRC channel - but let me state it for everyone else watching.

Users should not be compiling addons (for numerous reasons) - only addon authors . npm will be supporting binary installations. We are aiming for a good user experience for users and therefore not going to dedicate time towards having an addon build system that works for everyone. A good user experience is a small download and a package that runs out of the box - not a lengthy compilation procedure.

ry commented Dec 21, 2011

@TooTallNate this was clarified on the IRC channel - but let me state it for everyone else watching.

Users should not be compiling addons (for numerous reasons) - only addon authors . npm will be supporting binary installations. We are aiming for a good user experience for users and therefore not going to dedicate time towards having an addon build system that works for everyone. A good user experience is a small download and a package that runs out of the box - not a lengthy compilation procedure.

@arturadib

This comment has been minimized.

Show comment
Hide comment
@arturadib

arturadib Dec 21, 2011

Users should not be compiling addons (for numerous reasons) - only addon authors

This is awesome. But I'm still confused about what us authors will be using to compile our addons. If not node-waf and not node-gyp, then what? Perhaps this: 18b9220#diff-4 ?

Users should not be compiling addons (for numerous reasons) - only addon authors

This is awesome. But I'm still confused about what us authors will be using to compile our addons. If not node-waf and not node-gyp, then what? Perhaps this: 18b9220#diff-4 ?

@defunctzombie

This comment has been minimized.

Show comment
Hide comment
@defunctzombie

defunctzombie Dec 21, 2011

@ry IMHO this is a terrible idea. npm does not interface with the system's package manager in any way. What is this going to mean about ABI incompatible binaries shipped with npm? I would like to see some real use cases and deployment examples considered before assuming that everyone will just use npm to ship binaries. Are we now statically linking everything? What about people that don't want to do that for security reasons. I don't think this has been thought through fully from an operations and deployment standpoint.

@ry IMHO this is a terrible idea. npm does not interface with the system's package manager in any way. What is this going to mean about ABI incompatible binaries shipped with npm? I would like to see some real use cases and deployment examples considered before assuming that everyone will just use npm to ship binaries. Are we now statically linking everything? What about people that don't want to do that for security reasons. I don't think this has been thought through fully from an operations and deployment standpoint.

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Dec 21, 2011

uhhh how will this work for things like node-canvas that have many deps, and many optional deps (pdf backend, freetype, etc)

tj commented Dec 21, 2011

uhhh how will this work for things like node-canvas that have many deps, and many optional deps (pdf backend, freetype, etc)

@TooTallNate

This comment has been minimized.

Show comment
Hide comment
@TooTallNate

TooTallNate Dec 21, 2011

@arturadib Yes, gyp_addon will be used by module authors to compile addons.

@arturadib Yes, gyp_addon will be used by module authors to compile addons.

@arturadib

This comment has been minimized.

Show comment
Hide comment
@arturadib

arturadib Dec 21, 2011

@shtylman :

Are we now statically linking everything?

This is a good point. Realistically, I don't think most addon makers can afford to maintain the distribution of binaries on multiple platforms.

If we allow users to compile the addon themselves we can more realistically get help from the community in the form of patches/reports that help fix the build on different platforms, rather than having to build binaries ourselves on multiple platforms (few of us can afford to do it), or asking the community to build them on our behalf (which introduces an additional element of trust).

That being said, I don't see a problem with addon makers being able to distribute binaries via npm, that is if they have the means to do so. The security concern here is one of trust toward the addon maker, which can be mitigated via prompts ("Do you trust addon maker so-and-so at http://... ?"), and/or community flags ("Report this addon").

Makes sense?

@shtylman :

Are we now statically linking everything?

This is a good point. Realistically, I don't think most addon makers can afford to maintain the distribution of binaries on multiple platforms.

If we allow users to compile the addon themselves we can more realistically get help from the community in the form of patches/reports that help fix the build on different platforms, rather than having to build binaries ourselves on multiple platforms (few of us can afford to do it), or asking the community to build them on our behalf (which introduces an additional element of trust).

That being said, I don't see a problem with addon makers being able to distribute binaries via npm, that is if they have the means to do so. The security concern here is one of trust toward the addon maker, which can be mitigated via prompts ("Do you trust addon maker so-and-so at http://... ?"), and/or community flags ("Report this addon").

Makes sense?

@andreasbotsikas

This comment has been minimized.

Show comment
Hide comment
@andreasbotsikas

andreasbotsikas Dec 22, 2011

Since this is a closed pull request, the commits won't appear here. I incorporated ry’s suggestions in my branch. node-gyp includes node’s common.gypi and if you use the make command on windows, node-gyp patches the resulted visual studio file to add the missing < projectext > tag that gyp doesn’t put it.

Merry Christmas to all and thanks for your help and suggestions

Since this is a closed pull request, the commits won't appear here. I incorporated ry’s suggestions in my branch. node-gyp includes node’s common.gypi and if you use the make command on windows, node-gyp patches the resulted visual studio file to add the missing < projectext > tag that gyp doesn’t put it.

Merry Christmas to all and thanks for your help and suggestions

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Dec 22, 2011

@andreasbotsikas please make a new pull request on top of master with windows support. Try to get test/addons/hello-world working.

ry commented Dec 22, 2011

@andreasbotsikas please make a new pull request on top of master with windows support. Try to get test/addons/hello-world working.

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Dec 22, 2011

@shtylman managing our ABI dependencies is easier than managing the myriad complication environments that Node modules can find themselves in. We are considering forcing addons to not dynamically link to external libraries.

What about people that don't want to do that for security reasons.

What do you mean? Because you want to upgrade your libxml2 deb and have it automatically fix bugs in Node's libxml2 binding? No - we're not supporting that. I don't buy into this Debian SO mysticism which, in my opinion, has destroyed the usability of Unix. You can just install the new version of the libxml2 Node binding.

ry commented Dec 22, 2011

@shtylman managing our ABI dependencies is easier than managing the myriad complication environments that Node modules can find themselves in. We are considering forcing addons to not dynamically link to external libraries.

What about people that don't want to do that for security reasons.

What do you mean? Because you want to upgrade your libxml2 deb and have it automatically fix bugs in Node's libxml2 binding? No - we're not supporting that. I don't buy into this Debian SO mysticism which, in my opinion, has destroyed the usability of Unix. You can just install the new version of the libxml2 Node binding.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.