Skip to content
This repository has been archived by the owner on Jan 16, 2025. It is now read-only.

Adds iojs formula meeting requests by @mikemcquaid #36193

Closed
wants to merge 3 commits into from

Conversation

bcomnes
Copy link
Contributor

@bcomnes bcomnes commented Jan 24, 2015

I know there are some existing PRs, but this one tries to meet all the requests made in #35853 (comment)

This will probably need editing for message style. Apologies if this isn't what the maintainers are currently looking for.

class Iojs < Formula
homepage "https://iojs.org/"
url "https://iojs.org/dist/v1.0.4/iojs-v1.0.4.tar.xz"
version "1.0.4"
Copy link
Member

Choose a reason for hiding this comment

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

This might be redundant from the url. If it is, it should show up under brew audit.

@DomT4
Copy link
Member

DomT4 commented Jan 24, 2015

Can't comment on what the maintainers want here, but aside the few tiny nits I highlighted, the submission itself is great in terms of style, thanks :)


To intall `npm` and have it use `iojs`, install `node` and add
iojs to the front of your path:
export PATH=#{HOMEBREW_PREFIX}/opt/iojs/bin:$PATH
Copy link
Member

Choose a reason for hiding this comment

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

Potentially export PATH=#{Formula["iojs"].opt_bin}:$PATH :)

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 24, 2015

Thanks @DomT4, all good points fixing now.

@DomT4
Copy link
Member

DomT4 commented Jan 24, 2015

Thanks for the quick fixes, Appreciate that a lot!


depends_on :python => :build


Copy link
Member

Choose a reason for hiding this comment

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

Blank line here needs to be removed. Otherwise, Bot reports all is good.

==> brew audit iojs --strict                         FAILED
==> brew style iojs
== /usr/local/Library/Formula/iojs.rb ==
C: 12:  1: Extra blank line detected.

1 file inspected, 1 offense detected

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 25, 2015

Should this have a depends_on "npm" so that a copy of npm gets installed via node?

@DomT4
Copy link
Member

DomT4 commented Jan 25, 2015

It'd have to be a depends_on "node" rather than npm, but I don't think it's needed. There may be someone out there who wants to use iojs without npm and if we stuck in a node dep we'd almost be forcing one onto them, alas.

@MikeMcQuaid
Copy link
Member

Great work here @bcomnes and @DomT4. Just merged.

@DomT4
Copy link
Member

DomT4 commented Jan 25, 2015

Thanks all. Good to get this in; Appreciate the sheer amount of work everyone poured into it.

@MikeMcQuaid MikeMcQuaid mentioned this pull request Jan 25, 2015
@rvagg
Copy link

rvagg commented Jan 25, 2015

Note that that we are currently patching npm released in io.js to properly download source (headers) for native addon compiles. The general npm distribution won't do this properly and therefore won't compile any native addons. We are working on a fix for this but it's not there yet.

@rvagg
Copy link

rvagg commented Jan 25, 2015

e.g. try npm install buffertools as a quick test for this to see it fail

@MikeMcQuaid
Copy link
Member

@rvagg This isn't using the npm provided in iojs.

@rvagg
Copy link

rvagg commented Jan 25, 2015

that's my point

@MikeMcQuaid
Copy link
Member

@rvagg Sorry, I'm not sure how we're expected to resolve this? Should I just revoke this formula until you say it's ready to be used with an unpatched npm?

@MikeMcQuaid
Copy link
Member

@rvagg Also, did I miss in the previous PR where someone had mentioned this?

@rvagg
Copy link

rvagg commented Jan 25, 2015

just pointing it out, that's all, #35853 has appropriately set my expectations about resolutions

@MikeMcQuaid
Copy link
Member

@rvagg That's not helpful; I'm doing my best to try and get your software working in Homebrew. From that PR:

npm works the same under each of them, so there's no reason to have two different npm installs
@aredridel

Speaking as the npm CLI team lead, we draw no distinction between joyent/node and iojs/io.js. We support them both equally.
@othiym23

@rvagg Did you not think it might be worth correcting these statements before this got merged? Might have been more productive than tweeting about bikeshedding. If you're not interested in helping me work out a solution to this then I'm going to revoke iojs shortly.

@MikeMcQuaid
Copy link
Member

Clarified the situation in 8fa3572. Given comments in #35853 I suspect this may make iojs currently useless for people but I'm not really sure what else to do. Sorry for my snark and thanks for the clarification @rvagg.

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 25, 2015

It seems like the question now becomes, is there a single npm that can adequately serve both node and iojs based on PATH ordering? I'll poke around and see what I can find out.

@aredridel
Copy link
Contributor

npm runs equally well on both; if a user changes which 'node' runs (be it version or switching between iojs and node) then binary addons will need to be rebuilt by npm rebuild on that module, but npm itself will stay running.

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 25, 2015

@aredridel thats what I thought to but this was just clarified:

we are currently patching npm released in io.js to properly download source (headers)
@rvagg

So it seems there are differences between the versions of npm distributed between node and iojs

Also, I believe having the current formula is progress, and it works to some degree with the non-patched npm version, although it will be critical to find a fully compatible solution. Looking for a simple solution.

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 25, 2015

To clarify whats going on:

s/npm/node-gyp/ - it's node-gyp that's doing the downloading
@bnoordhuis

iojs npm isn't whats patched, the node-gyp dependency is, and this patch has been submitted upstream nodejs/node-gyp#564

Here is the forked node-gyp: https://github.com/iojs/io.js/tree/v1.x/deps/npm/bin/node-gyp-bin

Next questions: Is it possible to apply that patch to npm's node-gyp? Or would that break compatibility with node?

@MikeMcQuaid
Copy link
Member

@bcomnes I'd rather hold off until that PR is merged and we've got a new npm release we can use for both.

@bcomnes
Copy link
Contributor Author

bcomnes commented Jan 26, 2015

Seems fair. I'll probe around and keep an ear out.

@bcomnes bcomnes deleted the iojs branch January 29, 2015 16:24
@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.

5 participants