Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Node gyp addon #2337

Closed
wants to merge 11 commits into from
Closed

Node gyp addon #2337

wants to merge 11 commits into from

Conversation

andreasbotsikas
Copy link

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.

…_module.gypi's location must be specified inside the module.gyp file.
Fixed a minor path bug that occurred if you haven't added the trailing / in the environment variable.
@TooTallNate
Copy link

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
Copy link

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).

'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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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).

}],
[ 'OS=="mac"', {
'defines': [
'uint=unsigned int', #Mac doesn't have uint either

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

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
Copy link

@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!

…dule.gyp. Updated sampleGypModule/module.gyp to demonstrate that there is no need to have a reference to the gypi file any more.
@andreasbotsikas
Copy link
Author

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
Copy link
Author

@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
Copy link

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

@TooTallNate
Copy link

@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
Copy link

This seems to be relevant for #2136

@ry
Copy link

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
Copy link
Author

@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
Copy link

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
Copy link
Author

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

…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
Copy link
Author

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
Copy link
Author

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)',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 'type': 'loadable_module'

@ry
Copy link

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
Copy link

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
Copy link

ry commented Dec 21, 2011

@TooTallNate node-waf is going away

@TooTallNate
Copy link

??? /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
Copy link

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
Copy link

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: 18b92201b#diff-4 ?

@defunctzombie
Copy link

@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
Copy link

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
Copy link

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

@arturadib
Copy link

@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
Copy link
Author

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
Copy link

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
Copy link

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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants