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 #12

Closed
wants to merge 6 commits into from

Conversation

TooTallNate
Copy link

Hey @indexzero! I've added support for the new gyp build system and node-gyp, since node-waf has been removed in node v0.8. So this simply ensures forward compatibility. Cheers!

@indexzero
Copy link
Owner

Thanks. @TooTallNate this is an all-or-nothing thing right? i.e. We should merge this into a v0.8 feature branch until v0.8 is out?

@TooTallNate
Copy link
Author

The way I set this one up is kinda all-or-nothing, since I deleted the install script and changed the preinstall phase. I could revert that commit and that way you would just have the bindings.gyp file ready, but the waf build would still take precedence for now. Does that sound better?

Otherwise ya, you should probably merge into a v0.8 branch or something.

@indexzero
Copy link
Owner

@TooTallNate I have no problem merging into a v0.8 branch. That was our approach for the v0.6 migration.

@indexzero
Copy link
Owner

Thanks again!

@TooTallNate
Copy link
Author

Bump?

@kangax
Copy link

kangax commented May 28, 2012

Any progress on this?

@indexzero
Copy link
Owner

When 0.8.0 goes live we'll merge this in.

@TooTallNate
Copy link
Author

What's the rationale there?

@indexzero
Copy link
Owner

@TooTallNate Is gyp in node@0.6.x? If I understand it correctly this branch won't work on 0.6.x so the rationale is not breaking the build.

If I'm wrong, please let me know what what I'm missing.

@TooTallNate
Copy link
Author

It's actually bound to the npm version:

npm v1.1.x and up come with node-gyp bundled internally (i.e. it doesn't get installed to your PATH, but it does get used when invoking npm install ...). So if the user has at least this version of npm, then node-gyp will be used when installing the module when it sees a binding.gyp file in the repo.

If they don't have a version of npm at least this new, then it will use the old npm behavior, which would be to invoke node-waf automatically when it sees a wscript file in the repo.

So it's possible (and actually ideal) to enable a "dual-build" setup, where there's both a wscript and binding.gyp file in the repo, and no "scripts.preinstall" or "scripts.install" phases in the package.json. Then npm will "just work" (use node-gyp on newer npms, node-waf on older npms) when it comes to compiling the module.

Make sense? I know it's a little confusing...

@indexzero
Copy link
Owner

Yes it does. Thanks for clarifying. We'll try to get this published sooner then.

@tomas
Copy link

tomas commented Jul 4, 2012

Hey guys,

v0.8.1 is out there and I'm getting a nasty error after installing/building the current version (no suitable image found, wrong architecture). Any chances this gets merged soon?

Thanks for the great module, by the way.

@TooTallNate
Copy link
Author

@tomas node v0.8.x has the new "detached" option, which supersedes this module. You should use that instead.

@tomas
Copy link

tomas commented Jul 4, 2012

Yikes, totally missed it. Thanks for pointing it out @TooTallNate.

I guess it still should be merged though. :)

@indexzero
Copy link
Owner

@tomas Actually no. I'm not going to merge this because this module is now officially deprecated. You should only be using this on node <= 0.7.10.

I will make the deprecation notice formal later today. @TooTallNate thanks for the work though!

@indexzero indexzero closed this Jul 6, 2012
@tomas
Copy link

tomas commented Jul 6, 2012

On second thought, it makes total sense, haha.

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.

None yet

4 participants