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

Add io.js support #564

Closed
wants to merge 3 commits into from
Closed

Add io.js support #564

wants to merge 3 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 17, 2015

fixes #563

Added a Makefile and a couple of devdeps to form a functional test that can be used to test compatibility on both Node and io.js, also in the form of a Docker image so you can run the tests on Linux without having install both runtimes locally.

@@ -21,6 +21,8 @@ var fs = require('graceful-fs')
, minimatch = require('minimatch')
, mkdir = require('mkdirp')
, win = process.platform == 'win32'
, runtime = semver.parse(process.version).major < 1 ? 'node' : 'iojs'
Copy link
Contributor

Choose a reason for hiding this comment

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

Longer-term, I'd really like to see iojs add an "iojs" property to process.versions so this test, which feels like a flaky heuristic that somebody, somewhere is going to forget about at some point, with unpredictable, but probably dire, consequences, can go away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. This is pretty lame.

Copy link
Member Author

Choose a reason for hiding this comment

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

discussion here nodejs/node#491

@TooTallNate
Copy link
Contributor

General comment: this seems like a departure from being "node-compatible". Why is changing the build file names so necessary?

@othiym23
Copy link
Contributor

I proposed nodejs/node#491 to improve the tests for whether node-gyp is being run against iojs or node.

@isaacs
Copy link
Contributor

isaacs commented Jan 18, 2015

Just so I'm clear, changing the build filenames is because io.js is shipping as a binary named "iojs" and only symlinked to "node", right?

It seems like, rather than key on the version or rely on a process.versions.iojs, we might want to add some kind of API in iojs for "What is the build filename target?", which we can also add to Node.js, or any other forks/distros in the future.

That might also actually bring peace to any remaining debiants who still call it "/usr/bin/nodejs".

@TooTallNate
Copy link
Contributor

@isaacs Possibly something like this?

path.basename(fs.realpathSync(process.execPath))

@isaacs
Copy link
Contributor

isaacs commented Jan 18, 2015

In the very short term, it seems like a version number switch is the only option before us, and in the medium term, adding a one-off flag for iojs is probably easier than getting something into Node, so +1 from me for pragmatism on both counts, as long as we don't lose sight of cleaner solutions to the core meta-problem.

Tests and everything else about this pull req are awesome.

@isaacs
Copy link
Contributor

isaacs commented Jan 18, 2015

@TooTallNate Actually you don't even need the realpath, since execPath is already realpathed.

$ which node
/usr/local/bin/node

$ node
> process.execPath
'/usr/local/bin/iojs'

@rvagg
Copy link
Member Author

rvagg commented Jan 18, 2015

execPath is problematic on Windows, we currently have a link but a stub node.exe was planned; I don't know if that's still on the table, @piscisaureus

@rvagg
Copy link
Member Author

rvagg commented Jan 18, 2015

re build filenames - we're not going to get people to download node-v1.0.2.tar.gz to get io.js, it's a different product and is Node-compatible (ish) but there's a limit to what we can do. Remember that Joyent were the ones that forced us to change from "node-forward" to io.js because of tradmark threats so we need some differentiation.

@isaacs
Copy link
Contributor

isaacs commented Jan 18, 2015

If process.execPath isn't an option, then I'd strongly prefer a more full-featured process.release option which could answer these questions explicitly, rather than inferring from the version number.

@rvagg
Copy link
Member Author

rvagg commented Jan 18, 2015

process.release:

{
  source: "https://iojs.org/dist/v1.0.2/iojs-v1.0.2.tar.gz"
  headers: "https://iojs.org/dist/v1.0.2/iojs-v1.0.2-headers.tar.gz" # fictional but easy
  lib: "https://iojs.org/dist/v1.0.2/win-x64/iojs.lib" # only on Windows
}

Then node-gyp could even support nightlies.

@TooTallNate
Copy link
Contributor

Very nice @rvagg :)

@piscisaureus
Copy link
Contributor

execPath is problematic on Windows, we currently have a link but a stub node.exe was planned; I don't know if that's still on the table, @piscisaureus

The stub trick I wanted to use turned out to be a bit problematic (issues with the stack protection cookie; ask me on irc for details.) The hard link solution we have now seems to work fine, so I want to stick to that.

{
source: "https://iojs.org/dist/v1.0.2/iojs-v1.0.2.tar.gz"
headers: "https://iojs.org/dist/v1.0.2/iojs-v1.0.2-headers.tar.gz" # fictional but easy
lib: "https://iojs.org/dist/v1.0.2/win-x64/iojs.lib" # only on Windows
}

That seems like a nice solution.
I'd still like to ship the headers plus lib file in the installer so no download would be necessary at all.

@rvagg
Copy link
Member Author

rvagg commented Jan 18, 2015

headers are shipped with all distros but Windows currently, they have been from somewhere in Node 0.10, but nobody has put in the work to make node-gyp aware of them.

However, even if we do use installed headers there's no guarantee that they are actually going to be available on the system (plain binary install, stripped-down installer of some kind, manual install), so there needs to be a facility to fetch the headers anyway.

@XadillaX
Copy link
Contributor

➜  thmclrx git:(master) node --help
Usage: node [options] [ -e script | script.js ] [arguments]
       node debug script.js [arguments]

Options:
  -v, --version        print node's version
  -e, --eval script    evaluate script
  -p, --print          evaluate script and print result
  -i, --interactive    always enter the REPL even if stdin
                       does not appear to be a terminal
  --no-deprecation     silence deprecation warnings
  --throw-deprecation  throw an exception anytime a deprecated function is used
  --trace-deprecation  show stack traces on deprecations
  --v8-options         print v8 command line options
  --max-stack-size=val set max v8 stack size (bytes)

Environment variables:
NODE_PATH              ':'-separated list of directories
                       prefixed to the module search path.
NODE_MODULE_CONTEXTS   Set to 1 to load modules in their own
                       global contexts.
NODE_DISABLE_COLORS    Set to 1 to disable colors in the REPL

Documentation can be found at https://iojs.org/

here you can see iojs.

so just run node --help and you can judge whether it's iojs or nodejs.

@@ -21,6 +21,8 @@ var fs = require('graceful-fs')
, minimatch = require('minimatch')
, mkdir = require('mkdirp')
, win = process.platform == 'win32'
, runtime = semver.parse(process.version).major < 1 ? 'node' : 'iojs'
, defaultDisturl = runtime == 'node' ? 'http://nodejs.org/dist' : 'https://iojs.org/dist'

Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea to support nvm custom dist url env by default? https://github.com/creationix/nvm/blob/master/nvm.sh#L73

Copy link
Member Author

Choose a reason for hiding this comment

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

@fengmk2 would this be to support mirrors, like in China? I'm tinkering around in there currently and it wouldn't be hard to bring in custom URLs from where my local version is at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not only for mirrors, but also can be used in private NPM services. @rvagg

Copy link
Contributor

Choose a reason for hiding this comment

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

@rvagg yes, it will be very helpful for us.

inspect process.release object for details about the platform and
where to get source files
@fengmk2
Copy link
Contributor

fengmk2 commented Mar 11, 2015

I temporary use pangyp to build io.js and nodejs cpp module now.
Required npm@>2.7.0

$ npm i -g pangyp
$ npm --node-gyp=$your-npm-global-dir/pangyp/bin/node-gyp.js

@leecade
Copy link

leecade commented Mar 11, 2015

@fengmk2 出手 PR 啊, 哈哈

@XadillaX
Copy link
Contributor

@leecade siwujidan yong zhongwen zhende haome?

@trevorlinton
Copy link

Curious question... Why is the node native module system requiring a CRT//DllMain type initialization?

Couldn't NODE_MODULE macro be changed to define a stub "#modulename#_register" that returns the initialization function (rather than trying to run the node_module_register from within #modulename#register" that then gets ran and passed the options exports?

The current method relies on a very smart dynamic link loader thats transitive (and OS X is very much so) but with Windows and statically linking the node.lib its creating a lot of sticky situations where the proc addresses are being hard coded for, i'm not sure why.

Granted, i may not fully understand this but it seems like theres a simple non-breaking change that could be made if both node.js and io.js agreed to mutually change the definition of NODE_MODULE macro.

Or just have the NODE_MODULE macro kick out symbols for the old way and the new way, and node.js can continue doing whatever it wants, and others like io.js or tint can successfully register modules on windows.

Thoughts?

@hanzhao
Copy link

hanzhao commented Apr 18, 2015

Why this pr isn't merged so far?

return callback(e)
}

console.log(release)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this (I assume?)

@deepsweet
Copy link

does pangyp solution work for io.js@3?

@JCMais
Copy link

JCMais commented Aug 13, 2015

@deepsweet yes

@rvagg
Copy link
Member Author

rvagg commented Sep 4, 2015

new impl, using process.release and taking convergence into consideration #711

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

Successfully merging this pull request may close these issues.

node-gyp and iojs