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

Adds flag to patch npm install for iojs compatibility #36357

Closed
wants to merge 6 commits into from

Conversation

bcomnes
Copy link
Contributor

@bcomnes bcomnes commented Jan 29, 2015

I extracted the relevant patches from nodejs/node@d1fc9c6 2015-01-24 io.js v1.0.4 Release

So that now the npm homebrew installs can work with the keg-only iojs for now.

the patch is still compatible with joyent node: http://logs.libuv.org/npm/2015-01-28#21:53:34.823

Current iojs formula discussions

working iojs taps

Past iojs formula discussions

npm formula issues:

// now download the node tarball
var tarPath = gyp.opts['tarball']
- var tarballUrl = tarPath ? tarPath : distUrl + '/v' + version + '/node-v' + version + '.tar.gz'
+ var tarballUrl = tarPath ? tarPath : distUrl + '/v' + version + '/iojs-v' + version + '.tar.gz'
Copy link
Member

Choose a reason for hiding this comment

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

This patch looks way too iojs specific; it's literally just overriding node urls with iojs ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be only temporary until node-gyp can decide where to download from on its own. This only touches the npm portion of the install and this is how npm devs are currently dealing with iojs compatibility. It also doesn't break joyent node compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather wait until this patch is accepted for this reason. It looks like it could cause issues if that node fallback is just removed.

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 29, 2015

Ok, fixed the npm patching and flagged it. Thoughts? I've been totally assured that the folks behind npm support the patch.

resource("npm").stage buildpath/"npm_install"

npm_buildpath = buildpath/"npm_install"
resource("npm").stage npm_buildpath
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure there's a huge amount of benefit in splitting this onto two lines? Purely IMO.

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 29, 2015

That last commit was an oops.

bcomnes added a commit to bcomnes/homebrew that referenced this pull request Jan 29, 2015
This PR
- takes iojs out of keg-only
- conflicts node and iojs
- patches iojs’s installed npm
- currently leaves in the iojs patch flag from Homebrew#36357
@bcomnes bcomnes mentioned this pull request Jan 29, 2015
2 tasks
@bcomnes bcomnes changed the title Patched npm install for iojs compatibility Adds option to patch npm install for iojs compatibility Jan 30, 2015
@bcomnes bcomnes changed the title Adds option to patch npm install for iojs compatibility Adds flag to patch npm install for iojs compatibility Jan 30, 2015
@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 31, 2015

Is it still a no on patching the current npm install process, even when its behind a flag? I'm not sure if people missed this or not. Adding the flag would really make the existing keg-only iojs install more useful.

@MikeMcQuaid
Copy link
Member

Is it still a no on patching the current npm install process, even when its behind a flag? I'm not sure if people missed this or not. Adding the flag would really make the existing keg-only iojs install more useful.

For now, for me: yes. The patch doesn't seem to be adding iojs compatability as much as just changing some node strings to iojs ones. I'd be amazed if this won't break anything on the node side (but am willing to suspend my disbelief if/when this patch gets merged upstream)

@bcomnes
Copy link
Contributor Author

bcomnes commented Feb 1, 2015

Ok, I understand. Closing for now.

@bcomnes bcomnes closed this Feb 1, 2015
@Homebrew Homebrew locked and limited conversation to collaborators Feb 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants