Skip to content
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

Added support for "node-gyp" build #61

Merged
merged 36 commits into from
Apr 23, 2012
Merged

Conversation

TooTallNate
Copy link
Contributor

Hey guys, so this should take care of #60. I started with the current windows branch and worked from there. I created a gyp file for sqlite3 itself, and then declared that as a dependency in node-sqlite3's bindings.gyp file.

Try it out and let me know what you think! Cheers!

Dane Springmeyer and others added 27 commits November 6, 2011 15:41
…s but we don't want to depend on boost threads unless we really have to for now
…- all tests are passing on OS X 10.7 with boost 1.47 - todo: test on linux - refs TryGhost#48
Conflicts:
	src/async.h
	src/statement.h
	wscript
they still contain absolute paths
This is how node does it so let's stay consistent.
@cmundi
Copy link

cmundi commented Feb 13, 2012

node-gyp configure
node-gyp build
copy %BASE%\Release\node_sqlite3.node %BASE%\lib\node_sqlite3.node

All attempts to require('sqlite3') fail with "cannot find module 'bindings'" so I patched %BASE%\Release\sqlite3.js as follows

< var sqlite3 = module.exports = exports = require('bindings')('node_sqlite3.node');
---
> var sqlite3 = module.exports = exports = require('node_sqlite3.node');

and now the example code in the README.md runs as expected. I have not tried to run the test suite yet.

@cmundi
Copy link

cmundi commented Feb 13, 2012

Sorry about the ugly formatting of that last comment. It looked fine before I hit the submit button.

@kkaefer
Copy link
Contributor

kkaefer commented Mar 7, 2012

so you can no longer type 'npm install sqlite3' but have to manually install node-gyp beforehand? I'm a bit worried about the added complexity and users failing to install node-sqlite3. Do you recommend adding "npm install -g node-gyp" to the preinstall script?

@kkaefer
Copy link
Contributor

kkaefer commented Mar 7, 2012

Still, shouldn't node-gyp look at the current node version and download the appropriate distribution?

@TooTallNate
Copy link
Contributor Author

@kkaefer Well I know that @isaacs is leaning towards phasing out the preinstall script, most likely in favor of precompiled binaries, but that will take some modifications to npm I believe.

The idea is that normal users shouldn't have to worry about compiling binaries (similar to how they download a .pkg or .msi now), and npm should have the module binaries ready to go when the user types in npm install sqlite3. That's the idea, but there's a transition phase which we are in right now that makes the setup seem more awkward.

@TooTallNate
Copy link
Contributor Author

Still, shouldn't node-gyp look at the current node version and download the appropriate distribution?

That's what it does! Downloads the .tar.gz for the current (minor) version that node is running.

@kkaefer
Copy link
Contributor

kkaefer commented Mar 7, 2012

So you're saying that there are no changes in header files between patch levels?

@TooTallNate
Copy link
Contributor Author

@kkaefer Correct, that and node minor versions have binary stability, so you don't need to target your specific node version, only the minor version.

@springmeyer
Copy link
Contributor

@kkaefer Correct, that and node minor versions have binary stability, so you don't need to target your specific node version, only the minor version.

node might but not the v8 version that is bundled, in my experience.

@springmeyer
Copy link
Contributor

@kkaefer Correct, that and node minor versions have binary stability, so you don't need to target your specific node version, only the minor version.

node might but not the v8 version that is bundled, in my experience.

wow, I just checked and I'm delighted I'm wrong on that:

~/src$ diff node-v0.6.0/deps/v8/include/v8.h node-v0.6.11/deps/v8/include/v8.h 
~/src$ 

@springmeyer
Copy link
Contributor

however a difference like this could mean that node 0.6.0 headers might not work on windows:

~/src$ diff -u node-v0.6.0/src/node_object_wrap.h node-v0.6.11/src/node_object_wrap.h 
--- node-v0.6.0/src/node_object_wrap.h  2011-11-04 23:48:57.000000000 -0700
+++ node-v0.6.11/src/node_object_wrap.h 2012-02-17 12:39:45.000000000 -0800
@@ -22,12 +22,13 @@
 #ifndef object_wrap_h
 #define object_wrap_h

+#include <node.h>
 #include <v8.h>
 #include <assert.h>

 namespace node {

-class ObjectWrap {
+class NODE_EXTERN ObjectWrap {
  public:
   ObjectWrap ( ) {
     refs_ = 0;

@TooTallNate
Copy link
Contributor Author

@springmeyer Thanks for pointing that out. node_buffer.h will probably be similar. That may be a good reason to bump the tar.gz download version for the 0.6 target.

This shouldn't happen anymore though, and I'll be keeping an eye on the node commits to make sure.

@cmundi
Copy link

cmundi commented Mar 8, 2012

FYI I use node-sqlite3 on both Linux and Windows, and I emphatically
believe that node-gyp is the best thing yet for multiple-platform support
of node modules. Node-gyp is different, but it is not more complicated
than what came before and lingers today. Node-gyp actually results in a
net reduction of complexity and makes modules more accessible to more users
on more platforms.

2012/3/7 Konstantin Kfer <
reply@reply.github.com

so you can no longer type 'npm install sqlite3' but have to manually
install node-gyp beforehand? I'm a bit worried about the added complexity
and users failing to install node-sqlite3. Do you recommend adding "npm
install -g node-gyp" to the preinstall script?


Reply to this email directly or view it on GitHub:

#61 (comment)

@TooTallNate
Copy link
Contributor Author

@springmeyer The latest version of node-gyp (which is now bundled with npm, starting from v1.1.8) actually downloads dev files for the specific version of node that is currently running, not the minor version, so your concerns about differences in the header files should be taken care of now.

@Mithgol Mithgol mentioned this pull request Apr 12, 2012
@kkaefer
Copy link
Contributor

kkaefer commented Apr 20, 2012

Looked at this again, but it seems like the most recent node stable (0.6.15) still comes with 1.1.16 which doesn't seem to include node-gyp.

@isaacs
Copy link

isaacs commented Apr 20, 2012

Yes, it does: https://github.com/joyent/node/blob/v0.6.15/deps/npm/node_modules/node-gyp/package.json

It just doesn't install it separately. If you have a wscript, and a binding.gyp, and no explicit install script defined, then npm will default to using the binding.gyp in recent versions, or the wscript in older versions that didn't have node-gyp yet.

@Mithgol
Copy link
Contributor

Mithgol commented Apr 21, 2012

See also https://github.com/isaacs/npm/blob/master/lib/utils/read-json.js#L360 and a dozen of immediately following lines.

@kkaefer
Copy link
Contributor

kkaefer commented Apr 21, 2012

Ah, so it's there but not in PATH? Or did nvm play a trick on my and not add that to the path?

@Mithgol
Copy link
Contributor

Mithgol commented Apr 22, 2012

It's not in PATH.

At least, npm update npm -g never puts node-gyp in PATH (and should not, because node-gyp is not installed separately this way, but only as a subfolder of npm/node_modules/).

If you need a separate install of node-gyp for some reason, then run npm install -g node-gyp (as README at https://github.com/TooTallNate/node-gyp recommends).

And after a quick look at package.json it seems that it won't extend PATH anyway, so you have to add that module's path in PATH manually if you need it there.

I know that node-gyp's README file contain just node-gyp commands without any path, as if they were in your PATH already, but I guess actually they are not. (That's a guess and not my personal experience, so @TooTallNate may correct me if I'm wrong.)

@TooTallNate
Copy link
Contributor Author

@kkaefer It's like @isaacs said: npm bundles it's own (internal) copy of node-gyp which is used when installing packages that don't have an "install" script phase, but do have a "binding.gyp" file in the root of the package. This internal copy does not get installed into your PATH; it's only used by npm for commands like npm install.

If you want to have node-gyp in your PATH (which is useful for development of native modules, but not necessary for user installation, since npm has its own copy), the you can npm install -g node-gyp and have your own global copy installed.

Hope that clears things up!

@kkaefer kkaefer merged commit 427fd32 into TryGhost:master Apr 23, 2012
@isaacs
Copy link

isaacs commented Apr 24, 2012

When npm runs node-gyp, it runs the node-gyp that it bundles, always. It sets up the PATH in the subprocess to be such that it will work out that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants