Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Move back to node-gyp #1124

Closed
xzyfer opened this issue Sep 8, 2015 · 17 comments
Closed

Move back to node-gyp #1124

xzyfer opened this issue Sep 8, 2015 · 17 comments

Comments

@xzyfer
Copy link
Contributor

xzyfer commented Sep 8, 2015

node-gyp@3 has been released with iojs support - nodejs/node-gyp#711

Since iojs support was the primary reason for moving to pangyp it's time to start looking at moving back.

From the pangyp readme

A (hopefully temporary) fork of TooTallNate/node-gyp adding io.js support

@mgol
Copy link
Contributor

mgol commented Sep 8, 2015

node-gyp@3 brings also support for RCs, nightlies and basically everything that has properly set up the process.release object. It now takes URLs from there instead of hardcoding them in node-gyp (they remain hardcoded only for old Node/io.js versions that didn't have process.release).

So it's definitely worth to move!

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

Looks like binaries build with node-gyp@3.0.1 causes errors in the binding.

      ✓ should always map null, true and false to the same (immutable) object
      should properly bubble up errors from sass color constructor
libc++abi.dylib: terminating with uncaught exception of type std::invalid_argument: Constructor arguments should be numbers exclusively.
[1]    40186 abort      npm test

@saper
Copy link
Member

saper commented Sep 8, 2015

@xzyfer did you compile your node yourself? Is it linked with libstdc++ or libc++ ? Just ldd your-node

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

Looks like this error happens with pangyp as well. Interesting. Nothing in my environment has changed.

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

@saper I install node via nvm like I always have.

/Users/michael/.nvm/versions/io.js/v3.2.0/bin/node:
    /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1153.18.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1213.0.0)
    /usr/lib/libstdc++.6.dylib (compatibility version 7.0.0, current version 104.1.0)
    /usr/lib/libgcc_s.1.dylib (compatibility version 1.0.0, current version 283.0.0)

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

$ ls -lsa /usr/lib/libstdc++.6.dylib
8 lrwxr-xr-x  1 root  wheel  21 20 Dec  2013 /usr/lib/libstdc++.6.dylib -> libstdc++.6.0.9.dylib

@mgol
Copy link
Contributor

mgol commented Sep 8, 2015

FWIW, there are experimental (they may not be the official ones yet) Node.js 4.0.0 binaries available at https://ci.nodejs.org/job/iojs+release/165/.

@saper
Copy link
Member

saper commented Sep 8, 2015

@xzyfer I think I might have introduced a bug in binding.gyp. Can you open a separate issue with that and run npm install --verbose and post the output? I need your compiler's flags.

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

@saper done #1127

@xzyfer xzyfer self-assigned this Sep 8, 2015
@abritinthebay
Copy link

Has this project given any thought to using NAN (Native Abstractions for Node)?

It would remove all these compatibility issues - and is already fully Node 4.0 compatible.

In fact it's the recommended practice by those on the Node Foundation

@abritinthebay
Copy link

whelp.. ignore me then ;) heh.

@jonnybarnes
Copy link

I’m finding node-sass fails to install and its gyp that’s failing, should we move back to node-gyp?

@saper
Copy link
Member

saper commented Sep 8, 2015 via email

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

Hm since the error I encountered seems not to be related to node-gyp is might be worth shipping so at least the local compilation fallback works. Thoughts @saper?

@saper
Copy link
Member

saper commented Sep 8, 2015

+1 Let's merge #1130 first, add its AppVeyor counterpart, run tests of #1128 again and then merge it. Would be good to know if your post-nan 2.0 testing was inadequate or what has changed (Xcode upgrade, maybe?)

@xzyfer
Copy link
Contributor Author

xzyfer commented Sep 8, 2015

Merged #1130. About to start walking to work. Will be back online in about an hour.

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

No branches or pull requests

5 participants