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

Fix node-arch sniffing for node 0.6 and to use the correct node. #24

Closed
wants to merge 1 commit into from

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Nov 1, 2012

  • Use npm_config_prefix to use the node invoking this npm
  • node < 0.8 doesn't have 'process.config'

Hi Chris,

Without this patch the "ARCH=" sniffing would blow up with node 0.6, resulting in a fallback to 'isainfo -k' arch... which in my case was 64-bit when the relevant node build was 32-bit -> boom.

- Use npm_config_prefix to use the node invoking this npm
- node < 0.8 doesn't have 'process.config'
@chrisa
Copy link
Owner

chrisa commented Nov 2, 2012

Thanks for the report!

I've actually gone a slightly different way to fix the issues - I moved the javascript oneliner into a script and extended it to try process.arch as an alternative to process.config. This avoids getting involved in the differences in file(1) output on various platforms.

I also checked for a value in $npm_config_prefix to support building without npm, directly using node-gyp.

Would you be able to try the changes in my issue_24 branch, and see if they work OK in your environment?

@trentm
Copy link
Contributor Author

trentm commented Nov 2, 2012

Yup, your changes work for me (0.6 and 0.8 on SmartOS and Mac). Looking forward to a published ver with this. Thanks!

FWIW, a side nit: there is also no process.arch in node 0.4. But that's 0.4. :) It might be nice to add this to your package.json:

"engines": {"node": ">=0.6"},

to be explicit.

@chrisa
Copy link
Owner

chrisa commented Nov 2, 2012

Good call on "engines" - I've added that.

Version 0.2.4 is now published into NPM with these changes.

Thanks!

@chrisa chrisa closed this Nov 2, 2012
@trentm
Copy link
Contributor Author

trentm commented Nov 2, 2012

Thanks! Working for me in [email protected]

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.

2 participants