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

use process.release, make aware of io.js & node v4 differences #711

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@rvagg
Member

rvagg commented Sep 4, 2015

Looking for patient reviewers, ASAP please. I'd like to be able to ship v4 with this version patched over the top of npm and upstream it to npm as soon as they are happy with it as well.

  • introduce a lib/process-release.js module that encapsulates all of the logic around downloads, names, versions and directories
  • introduce a test for process-release.js that hits it with various configurations and asserts that it comes up with the right answers
  • make node-gyp aware of process.release where it exists (>=3.0.0) and use it to determine download locations and names
  • make node-gyp aware of "iojs" where process.release doesn't exist, thereby making it work for all versions of Node and io.js from 0.8 to current so an npm install npm@latest -g even on one of the older versions of io.js will just work (magic).
  • make node-gyp able to compile against header-only tarballs, see the one-line change in addon.gypi and the search in two locations for common.gypi, for >=3.0.0 we are shipping header-only tarballs which are super quick to download, but it also needs to be able to compile against full source as well
  • keep the --target stuff intact (although I'd love to know if anybody actually uses that)
  • make node-gyp aware of the change in location for node.lib (& iojs.lib) for >=1.0.0. In <1.0.0 you find 32-bit node.lib in the root of the version's dist directory (e.g. http://nodejs.org/dist/v0.12.7/node.lib) while the 64-bit node.lib is in an x64 subdirectory (e.g. http://nodejs.org/dist/v0.12.7/x64/node.lib), for io.js we took the opportunity to clean this up and they are now in win-x86 and win-x64 subdirectories (e.g. https://iojs.org/dist/v3.3.0/win-x86/iojs.lib & https://iojs.org/dist/v3.3.0/win-x64/iojs.lib) (and the .msi's have been renamed and are in the parent directory, but that's not relevant here). Note that it still downloads both 32-bit and 64-bit .lib files, I haven't changed that logic.
  • introduced a test/docker.sh script that runs it forreal, but only on Linux obviously:
    1. set up a base image with compile tooling
    2. set up a clones image on top of the base image with git clones of some native addons to test
    3. set up an image on top of the clones image for each of the versions of node and io.js to test plus the latest version of npm over the top
    4. start a new container for each version of node & io.js, copy pwd into npm's node-gyp dependency folder so it uses the current code for testing when you use npm install etc., then do an npm install of the projects to see how it behaves.
    5. you can see it downloading and using the source tarballs for all but io.js v3.3.0 for which it grabs the headers file because it's only in v3 that we started shipping that and introduced process.release

What does this buy us?

  • The ability to ship actually useful RC's, nightlies and master/unstable/canary builds which don't have to have proper release version numbers and sit in the release downloads directories, node-gyp will just work
  • The additional ability for sophisticated users to create custom / corporate / internal builds of Node that point to header/source tarballs that are at a different location e.g. inside a protected & HA network, thereby removing one more point of failure & build/bundle time (add in npme or similar and you can be fully HA). Note that this requires a custom build of Node, so re security it's the same discussion as the --link-module feature we introduced.
    • The ability to support existing versions of io.js with new versions of npm (no more "you have to use the npm that ships with that version of io.js). We also get the ability to re-use the io.js brand if we revive it and start shipping again for some purpose, no more floating patches.

What's not here?

We need to finally make node-gyp aware of locally installed headers: this has been on multiple people's radars for a very long time but has never got done but it should get done. When you install via a complete tarball, or a make install from the source, you get the header files installed for that version. node-gyp should be able to use those to avoid any downloading. However this is not a panacea because:

  1. This hasn't been enabled at all for Windows installers, although that's not an impossible task
  2. Some old versions don't unpack headers and some just have broken headers—from memory there's some broken stuff across 0.12 and a couple of versions of io.js shipped without openssl headers
  3. Some users don't have complete installs, we offer a plain node.exe for instance
  4. node-gyp still has the ability to "cross compile" addons which won't work here (--target)
  5. version managers will have to get creative with how they ship versions and communicate with node-gyp about the locations of header files
  6. Doesn't solve the node.lib problem on Windows, although we may be able to ship that in the msi as well
  7. ... more mess ..

So this is a TODO, someone needs to take a bite at this soon, but it's not a top priority for shipping core.

Possible reviewers: @nodejs/build, @nodejs/addon-api, @TooTallNate, @Fishrock123, @othiym23, @bnoordhuis, @piscisaureus

@@ -5,6 +5,7 @@
'product_prefix': '',
'include_dirs': [
'<(node_root_dir)/include/node',

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

headers tarball has a different structure to the source tarball, no deps or src subdirectories, but this addition makes existing addons work with a headers tarball, a source tarball or a --nodedir

@rvagg

rvagg Sep 4, 2015

Member

headers tarball has a different structure to the source tarball, no deps or src subdirectories, but this addition makes existing addons work with a headers tarball, a source tarball or a --nodedir

@rvagg

View changes

Show outdated Hide outdated lib/configure.js Outdated
, nodeLibPath64 = path.resolve(dir64, 'node.lib')
, nodeLibUrl32 = distUrl + '/v' + version + '/node.lib'
, nodeLibUrl64 = distUrl + '/v' + version + '/x64/node.lib'
, libPath32 = path.resolve(dir32, release.name + '.lib')

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

we have local libPath*'s and also release.libPath*'s, the former are where we put the x.lib files for the compiler to use (ia32 and x64 directories) and the latter is their location on the server and in the shasums lookup

@rvagg

rvagg Sep 4, 2015

Member

we have local libPath*'s and also release.libPath*'s, the former are where we put the x.lib files for the compiler to use (ia32 and x64 directories) and the latter is their location on the server and in the shasums lookup

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 4, 2015

Member

localLibPath*?

@Fishrock123

Fishrock123 Sep 4, 2015

Member

localLibPath*?

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

nothing else is called local in the codebase, I'm opting for blending in here

@rvagg

rvagg Sep 4, 2015

Member

nothing else is called local in the codebase, I'm opting for blending in here

This comment has been minimized.

@othiym23

othiym23 Sep 4, 2015

Contributor

How is this change going to interact with legacy deployments originally built using an older version of node-gyp? Will the old node.lib just be left there untouched until node_modules gets nuked?

@othiym23

othiym23 Sep 4, 2015

Contributor

How is this change going to interact with legacy deployments originally built using an older version of node-gyp? Will the old node.lib just be left there untouched until node_modules gets nuked?

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

Should leave everything in place–and it should set up the .node-gyp/* folders identical to older versions of node-gyp for 0.x versions of Node, including directory names source unpack and node.lib files.

@rvagg

rvagg Sep 4, 2015

Member

Should leave everything in place–and it should set up the .node-gyp/* folders identical to older versions of node-gyp for 0.x versions of Node, including directory names source unpack and node.lib files.

@rvagg

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@rvagg

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@rvagg

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/configure.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/process-release.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/install.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/install.js Outdated
@Fishrock123

View changes

Show outdated Hide outdated lib/install.js Outdated
input.pipe(gunzip).pipe(extracter)
return
}
var req = download(tarballUrl)
var req = download(release.tarballUrl)
if (!req) return

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 4, 2015

Member

Should this.. error?

@Fishrock123

Fishrock123 Sep 4, 2015

Member

Should this.. error?

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

old request bug I believe, related to #114, I dare not touch it without tests to understand it

@rvagg

rvagg Sep 4, 2015

Member

old request bug I believe, related to #114, I dare not touch it without tests to understand it

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 4, 2015

Member

I'd put a TODO or log something. I can in a different PR if I remember.

@Fishrock123

Fishrock123 Sep 4, 2015

Member

I'd put a TODO or log something. I can in a different PR if I remember.

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

oh, you know what this is for, it's for the cb(err) case inside download where it doesn't return a req because there was an error setting it up; so this makes perfect sense and the error gets handled in the callback.

leaving it alone

@rvagg

rvagg Sep 4, 2015

Member

oh, you know what this is for, it's for the cb(err) case inside download where it doesn't return a req because there was an error setting it up; so this makes perfect sense and the error gets handled in the callback.

leaving it alone

}
// 0.x.y-pre versions are not published yet and cannot be installed. Bail.
if (version.prerelease[0] === 'pre') {
log.verbose('detected "pre" node version', versionStr)
if (release.semver.prerelease[0] === 'pre') {

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

@Fishrock123 -pre matching comes from https://github.com/nodejs/node/blob/a338eb3c3e712fcaacdb475cc6f3378259a3d534/src/node_version.h#L19 i.e. you compiled this thing yourself, you should use --nodedir to point to it

@rvagg

rvagg Sep 4, 2015

Member

@Fishrock123 -pre matching comes from https://github.com/nodejs/node/blob/a338eb3c3e712fcaacdb475cc6f3378259a3d534/src/node_version.h#L19 i.e. you compiled this thing yourself, you should use --nodedir to point to it

var version = semver.parse(v)
if (!version) {
return callback(new Error('Invalid version number: ' + v))

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 4, 2015

Member

Is statement now missing? What if the version is invalid?

@Fishrock123

Fishrock123 Sep 4, 2015

Member

Is statement now missing? What if the version is invalid?

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

it could come in here like io.js-3.0.0 which is invalid, i.e. versionDir, and anyway, if it's an invalid version then it just won't delete

@rvagg

rvagg Sep 4, 2015

Member

it could come in here like io.js-3.0.0 which is invalid, i.e. versionDir, and anyway, if it's an invalid version then it just won't delete

}
// flatten the version Array into a String
version = version.version
var versionPath = path.resolve(gyp.devDir, version)

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 4, 2015

Member

Won't this need to specify like above? e.g. (name != 'node' ? name + '-' : '') + versionStr

@Fishrock123

Fishrock123 Sep 4, 2015

Member

Won't this need to specify like above? e.g. (name != 'node' ? name + '-' : '') + versionStr

This comment has been minimized.

@rvagg

rvagg Sep 4, 2015

Member

that comes in through versionDir from install, and if you want to remove an io.js version then you'd need to call it as io.js-3.3.0

I doubt very much whether this is ever called directly though so I'm not so concerned about tuning this to some imaginary scenario of how someone might want to use it

@rvagg

rvagg Sep 4, 2015

Member

that comes in through versionDir from install, and if you want to remove an io.js version then you'd need to call it as io.js-3.3.0

I doubt very much whether this is ever called directly though so I'm not so concerned about tuning this to some imaginary scenario of how someone might want to use it

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 4, 2015

Member

but it also needs to be able to compile against full source as well

@rvagg only for older versions, right?

Member

Fishrock123 commented Sep 4, 2015

but it also needs to be able to compile against full source as well

@rvagg only for older versions, right?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 4, 2015

Member

Also, Fixes: #701

Member

Fishrock123 commented Sep 4, 2015

Also, Fixes: #701

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 4, 2015

Member

but it also needs to be able to compile against full source as well

@rvagg only for older versions, right?

No, also for when you're compiling from the source tarball you can --nodedir and same for when you're doing it directly from the repo—like a lot of us in core have to do; we're stuck with both

Member

rvagg commented Sep 4, 2015

but it also needs to be able to compile against full source as well

@rvagg only for older versions, right?

No, also for when you're compiling from the source tarball you can --nodedir and same for when you're doing it directly from the repo—like a lot of us in core have to do; we're stuck with both

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 4, 2015

Member

comments added to process-release.js

Member

rvagg commented Sep 4, 2015

comments added to process-release.js

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Sep 4, 2015

Contributor

I've given it a quick pass-through but I want to pull it down and run the tests, which is going to take an hour or two. So far: looks good, like the addition of tests, npm will probably just integrate this as-is once it's published.

If it weren't for Node 4, I'd probably only land this in npm@3, because there are probably going to be enough knock-on effects from the switch as to cause issues (or at least surprises) for some users upgrading across npm@2 versions. I kind of wish that either npm@3 were the version going into Node 4 or this had a little more time to bake in before Node 4 drops, because I'm not sure I can anticipate all the implications of some of the changes in here.

Contributor

othiym23 commented Sep 4, 2015

I've given it a quick pass-through but I want to pull it down and run the tests, which is going to take an hour or two. So far: looks good, like the addition of tests, npm will probably just integrate this as-is once it's published.

If it weren't for Node 4, I'd probably only land this in npm@3, because there are probably going to be enough knock-on effects from the switch as to cause issues (or at least surprises) for some users upgrading across npm@2 versions. I kind of wish that either npm@3 were the version going into Node 4 or this had a little more time to bake in before Node 4 drops, because I'm not sure I can anticipate all the implications of some of the changes in here.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 7, 2015

Member

Updated to address the concern @bnoordhuis pointed out, it now looks for /\/win-(x86|x64)\// in the URL to switch for the two arch's, so you have to have withe /win-x86/ or /win-x64/ in the URL, which is harder to overload.

Unless there are no objections, I might merge this and release a v3 eh?

Member

rvagg commented Sep 7, 2015

Updated to address the concern @bnoordhuis pointed out, it now looks for /\/win-(x86|x64)\// in the URL to switch for the two arch's, so you have to have withe /win-x86/ or /win-x64/ in the URL, which is harder to overload.

Unless there are no objections, I might merge this and release a v3 eh?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 7, 2015

Member

@rvagg The regex still looks the same to me. Can it be that you didn't push the commit?

Member

bnoordhuis commented Sep 7, 2015

@rvagg The regex still looks the same to me. Can it be that you didn't push the commit?

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 7, 2015

Member

sorry, it seems that I have process.release branches on both this repo and my own fork .. pushed to the one that's behind this PR now.

  , bitsre = /\/win-(x86|x64)\//
  , bitsreV3 = /\/win-(x86|ia32|x64)\// // io.js v3.x.x shipped with "ia32" but should
                                        // have been "x86" 
Member

rvagg commented Sep 7, 2015

sorry, it seems that I have process.release branches on both this repo and my own fork .. pushed to the one that's behind this PR now.

  , bitsre = /\/win-(x86|x64)\//
  , bitsreV3 = /\/win-(x86|ia32|x64)\// // io.js v3.x.x shipped with "ia32" but should
                                        // have been "x86" 
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 7, 2015

Member

No problem and LGTM.

Member

bnoordhuis commented Sep 7, 2015

No problem and LGTM.

@ralphtheninja

This comment has been minimized.

Show comment
Hide comment
@ralphtheninja

ralphtheninja Sep 7, 2015

Contributor

Unless there are no objections, I might merge this and release a v3 eh?

Yes! 👯

Contributor

ralphtheninja commented Sep 7, 2015

Unless there are no objections, I might merge this and release a v3 eh?

Yes! 👯

rvagg added some commits Sep 7, 2015

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 7, 2015

Member

OK, two more changes. nodejs/node#2719 reminded me that we can't trust semver.satisfies() any more so I've switched to checking major version numbers where needed, see bfaf7df. Also I've removed node_modules from the tree in 44dae28, I hope there are no objections to this.

Member

rvagg commented Sep 7, 2015

OK, two more changes. nodejs/node#2719 reminded me that we can't trust semver.satisfies() any more so I've switched to checking major version numbers where needed, see bfaf7df. Also I've removed node_modules from the tree in 44dae28, I hope there are no objections to this.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 7, 2015

@rvagg Argh, sorry for this suggestion, I totally forgot most semver ranges ignore pre-releases now.

mgol commented Sep 7, 2015

@rvagg Argh, sorry for this suggestion, I totally forgot most semver ranges ignore pre-releases now.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 7, 2015

nodejs/node#2719 reminded me that we can't trust semver.satisfies() any more so I've switched to checking major version numbers where needed

Any chance for applying the same patch to pangyp?

mgol commented Sep 7, 2015

nodejs/node#2719 reminded me that we can't trust semver.satisfies() any more so I've switched to checking major version numbers where needed

Any chance for applying the same patch to pangyp?

@mgol mgol referenced this pull request Sep 7, 2015

Closed

Build node 4.0 binaries - NODE_MODULE_VERSION 46 #1122

8 of 9 tasks complete
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 7, 2015

Member

remove node_modules from tree

If we could do that elsewhere that would be nice. I'm actually curious if there are any advantages to them being in-tree for us.

Member

Fishrock123 commented Sep 7, 2015

remove node_modules from tree

If we could do that elsewhere that would be nice. I'm actually curious if there are any advantages to them being in-tree for us.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 7, 2015

Member

@mzgol done and released a patch update, just note that as soon as node-gyp@3 goes out I'm using the deprecate hammer on all those versions

Member

rvagg commented Sep 7, 2015

@mzgol done and released a patch update, just note that as soon as node-gyp@3 goes out I'm using the deprecate hammer on all those versions

rvagg added a commit that referenced this pull request Sep 8, 2015

use process.release, make aware of io.js & node v4 differences
PR-URL: #711
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg added a commit that referenced this pull request Sep 8, 2015

refactor for clarity, fix dist-url, add env var dist-url functionality
PR-URL: #711
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg added a commit that referenced this pull request Sep 8, 2015

test version major directly, don't use semver.satisfies()
semver.satisfies() doesn't play nicely with prerelease tags

PR-URL: #711
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg added a commit that referenced this pull request Sep 8, 2015

remove node_modules from tree
PR-URL: #711
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Sep 8, 2015

Member

merged and published as v3.0.0, thanks for all the help with this one folks!

Member

rvagg commented Sep 8, 2015

merged and published as v3.0.0, thanks for all the help with this one folks!

@rvagg rvagg closed this Sep 8, 2015

@rvagg rvagg deleted the rvagg:process.release branch Sep 8, 2015

@xzyfer xzyfer referenced this pull request Sep 8, 2015

Closed

Move back to node-gyp #1124

rvagg added a commit to nodejs/node that referenced this pull request Sep 8, 2015

deps: float node-gyp v3.0.0
* support process.release
* support all io.js versions
* support node v4+ including new download locations
* enable delay-load hook by default by default
* download header-only tarballs instead of full source

See nodejs/node-gyp#711 for full details

PR-URL: #2700
Reviewed-By: Forrest L Norvell <forrest@npmjs.com>

rvagg added a commit to nodejs/node that referenced this pull request Sep 8, 2015

deps: float node-gyp v3.0.0
* support process.release
* support all io.js versions
* support node v4+ including new download locations
* enable delay-load hook by default by default
* download header-only tarballs instead of full source

See nodejs/node-gyp#711 for full details

PR-URL: #2700
Reviewed-By: Forrest L Norvell <forrest@npmjs.com>

rvagg added a commit to nodejs/node that referenced this pull request Sep 8, 2015

deps: float node-gyp v3.0.0
    * support process.release
    * support all io.js versions
    * support node v4+ including new download locations
    * enable delay-load hook by default by default
    * download header-only tarballs instead of full source

See nodejs/node-gyp#711 for full details

PR-URL: #2700
Reviewed-By: Forrest L Norvell <forrest@npmjs.com>

@brianc brianc referenced this pull request Sep 11, 2015

Closed

Support node v4.0 #27

rvagg added a commit to nodejs/node that referenced this pull request Sep 12, 2015

deps: float node-gyp v3.0.0
* support process.release
* support all io.js versions
* support node v4+ including new download locations
* enable delay-load hook by default by default
* download header-only tarballs instead of full source

See nodejs/node-gyp#711 for full details

PR-URL: #2700
Reviewed-By: Forrest L Norvell <forrest@npmjs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment