Add :node special dependency #1408

torifat · 2016-10-30T13:44:24Z

reitermarkus · 2016-10-30T14:09:39Z

Library/Homebrew/compat/requirements.rb

@@ -17,3 +17,4 @@
 TuntapDependency           = TuntapRequirement
 X11Dependency              = X11Requirement
 ConflictsWithBinaryOsxfuse = NonBinaryOsxfuseRequirement
+NodeRequirement            = NodeRequirement


There is no need to add a compatibility layer for a new requirement.

Ok, thanks for pointing it out 😄

MikeMcQuaid · 2016-11-02T18:03:57Z

Library/Homebrew/requirements.rb

+  fatal true
+  default_formula "node"
+  satisfy { which("node") }
+end


Please add this to a separate node_requirement.rb file and, like the JavaRequirement, support using a version argument. This should perhaps be compulsory like the `RubyRequirement.

Ok, Thanks 👍

This should perhaps be compulsory like the RubyRequirement.

I wouldn't recommend this, personally.

torifat · 2016-11-02T20:36:06Z

@MikeMcQuaid I'm not good at Ruby. Can you please guide me here? I have implemented the version checking and it's working.

But, brew still trying to install node even if return value of satisfy build_env: false do is true.

DomT4 · 2016-11-02T20:49:34Z

But, brew still trying to install node even if return value of satisfy build_env: false do is true.

Are you pouring the bottle of a formula to check? If so, that's expected and correct behaviour. To avoid the default_formula you'd need to build from source, brew install blah -s, etc.

torifat · 2016-11-02T20:52:58Z

@DomT4 ah, thanks. So, the current code should work fine. Though I forgot to remove require "language/node" which is not needed there anymore.

Update: It's working 😄 Thanks again.

torifat · 2016-11-03T00:50:16Z

Any idea why this test fails only in Jenkins?

NodeRequirementTests#test_satisfied [/Users/brew/Jenkins/pr-brew/version/any_sierra/Library/Homebrew/test/test_node_requirement.rb:29]:
Expected #<NodeRequirement: "node" [] version="4.0">
 to be satisfied?.

Has been addressed.

torifat · 2016-11-05T15:12:26Z

@MikeMcQuaid Can you have a look here, please?

MikeMcQuaid · 2016-11-05T15:15:26Z

Any idea why this test fails only in Jenkins?

Likely because there's no Node installed there. You may need to do some mocking.

As @DomT4 mentions above some things that are worth considering on this once it's 💚:

can something that uses npm install from node at build-time use iojs or a (very) different node version at runtime?
as-is all bottled formulae using depends_on :node will end up being depends_on "node" by default. This could be worked around if 1) is resolved.

torifat · 2016-11-05T15:25:35Z

Likely because there's no Node installed there. You may need to do some mocking.

But, I have mocked node and it works locally & in travis too.

https://github.com/Homebrew/brew/pull/1408/files#diff-595241f18c3635cda19d357a78dc5c52R6

As long as it meets the version requirement it should work.

Daniel15 · 2016-11-05T23:33:06Z

can something that uses npm install from node at build-time use iojs or a (very) different node version at runtime?

This works most of the time, the main exception is when using native modules, where it only works if you install the package on a version of Node.js that's binary (ABI) compatible with the version you use at runtime. In that case, I think generally some package installed using npm 6.x should run on any other 6.x version of npm.

The vast majority of npm packages are non-native packages that just contain JavaScript code so it doesn't matter which version of Node.js you used to install them.

Also iojs is completely unsupported now, so it's not worth worrying about it.

MikeMcQuaid · 2016-11-06T18:03:36Z

In that case, I think generally some package installed using npm 6.x should run on any other 6.x version of npm.

Ok. We'll need to figure out a way to encode that into this requirement.

The vast majority of npm packages are non-native packages that just contain JavaScript code so it doesn't matter which version of Node.js you used to install them.

Good to know.

Also iojs is completely unsupported now, so it's not worth worrying about it.

The voice of a non-packager 😉. As we're just looking for a node in $PATH we will likely find some people/systems with weird stuff like iojs still around so we'll need to detect and handle these cases.

torifat · 2016-11-06T18:12:20Z

@MikeMcQuaid which node should work for iojs too.

There's no version conflict too:

And, finally I mocked node in test but it's failing in Jenkins 😢 Working in both local & travis.

https://github.com/Homebrew/brew/pull/1408/files#diff-595241f18c3635cda19d357a78dc5c52R6

MikeMcQuaid · 2016-11-06T18:19:15Z

And, finally I mocked node in test but it's failing in Jenkins 😢 Working in both local & travis.

Let's not worry about that until the PR handles the other issues above. When it's working there I'll help fix the issues.

ilovezfs · 2016-11-14T13:39:27Z

Are we sure we want yet another one of these special requirements given the difficulties we've had with the others?

MikeMcQuaid · 2016-11-15T09:09:47Z

@ilovezfs I think this may be one where we can handle bottles sensible due to this being interpreted with vendored dependencies rather than using linkage. This PR is still a bit off, though, and there's a bunch of questions to be answered and implemented.

torifat · 2016-11-15T09:13:59Z

@MikeMcQuaid I tried to answer all the questions you asked and also pursed @Daniel15 to answer too.

Ok. We'll need to figure out a way to encode that into this requirement.

Any suggestions regarding how we can do that?

bunch of questions to be answered and implemented.

I'm not sure which questions are still unanswered.

MikeMcQuaid · 2016-11-15T09:17:44Z

As long as it meets the version requirement it should work.

We'll need confirmation that it does work and has been tested with a couple of different depends_on "node" formulae.

Any suggestions regarding how we can do that?

I'm not sure. Are you saying both the Node and NPM version matter at runtime?

torifat · 2016-11-15T09:26:26Z

We'll need confirmation that it does work and has been tested with a couple of different depends_on "node" formulae.

I can do that for a few popular formulae.

I'm not sure. Are you saying both the Node and NPM version matter at runtime?

Only node version matters. npm is mostly backward compatible.

IMO brew shouldn't handle npm install or yarn install. Formulae tarball should comes with node_modules like yarn tarball.

MikeMcQuaid · 2016-11-15T09:30:16Z

I can do that for a few popular formulae.

Thanks! Report back in here and try with node, homebrew/versions/node4-lts, homebrew/versions/node6-lts and homebrew/versions/iojs formulae.

npm is mostly backward compatible.

Can you elaborate on "mostly"?

IMO brew shouldn't handle npm install or yarn install. Formulae tarball should comes with node_modules like yarn tarball.

Check out the e.g. yarn.rb and jsonlint.rb formulae. We're open to doing it in a different way but may need some formula PRs for that. Thanks!

gsklee · 2017-01-10T06:30:04Z

Is something blocking further progress of this MR?

tonytonyjan · 2017-01-11T20:49:52Z

wonder if there is any progress

MikeMcQuaid · 2017-01-11T21:04:50Z

There's no news or progress. If there was it'd be posted on here. If anyone would like to try and create their own PR: please do.

sjmueller · 2017-01-13T02:21:29Z

Why is this taking so long? Right now yarn from homebrew is broken for lots and lots of people who use nvm. Simply check if node or nvm is installed and skip as necessary. Even if it doesn't handle every case, surely it will improve from what's out there now...

ilovezfs · 2017-01-13T02:30:51Z

Sorry I don't understand. The requirement system is only intended to provide non-mandatory flexibility. The default formula should always be sufficient to build any formulae, so the urgency here is pretty low unless you're doing something peculiar.

DomT4 · 2017-01-13T03:05:16Z

Having used nvm and Homebrew together for quite some time in the past I'm fairly sure the most "broken" I ever found things was the minor irritation of having two Nodes installed. As ILZ points out, you'll still have to remember to stick an -s on brew install xyz where applicable to avoid that happening even if this is merged.

MikeMcQuaid · 2017-01-13T05:55:19Z

Why is this taking so long?

Because it's neither the maintainers nor contributors paid employment to work on Homebrew or submit a PR to fix this. This PR has changes requested that need addressed and it's failing CI.

If you'd like to create your own competing PR, we'd welcome it. This document should help and we're happy to walk you through anything else.

Thanks!

Simply check if node or nvm is installed and skip as necessary. Even if it doesn't handle every case, surely it will improve from what's out there now...

If it were a simple problem to solve, it would have been solved already.

sjmueller · 2017-01-13T07:51:32Z

So are you saying there is a way right now to install yarn via homebrew without installing a second copy of node (when using nvm)?

Daniel15 · 2017-01-13T07:54:27Z

@sjmueller I no longer have a Mac so I can't test this, but the --ignore-dependencies flag should work

jeanregisser · 2017-01-13T08:19:24Z

Yes this flag works, but the next time you do a brew upgrade and there's an update for yarn, it will install it with the node deps.

ilovezfs · 2017-01-13T09:23:28Z

@sjmueller as @DomT4 said above, "the most 'broken' I ever found things was the minor irritation of having two Nodes installed."

MikeMcQuaid · 2017-01-13T14:51:29Z

@sjmueller two current options:

Stop caring about having two nodes on your system
Install Yarn through NPM or other means

Daniel15 · 2017-01-13T16:52:20Z

If you don't want to use Homebrew to install Yarn, there's an installation shell script you can use. I don't think it's bad to have two Node.js versions installed though, particularly if you just use nvm for your own apps and the "system" / Homebrew-installed Node.js for utilities. In the end, I don't think it really matters which Node.js version is used for Yarn, as long as it works properly? Sent from my phone.

…

On Jan 13, 2017 6:51 AM, "Mike McQuaid" ***@***.***> wrote: @sjmueller <https://github.com/sjmueller> two current options: 1. Stop caring about having two nodes on your system 2. Install Yarn through NPM or other means — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1408 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFnHczsjomD3GZDIqRpPT1x8Ltn5ge-ks5rR493gaJpZM4KkYPk> .

torifat · 2017-01-13T16:59:08Z

Sorry, I'm migrating to another country. So, currently not getting enough time to work on this. Anyone interested should take the lead.

DomT4 · 2017-01-13T17:02:25Z

If the primary reason most people want this is to avoid dual installations that's a slippery slope towards:

depends_on :rust
depends_on :go
depends_on :node
depends_on :swift
depends_on :mono
depends_on :maven
depends_on :opam

And so on. I can see an argument for it but I also see endless new possibilities for people to break their local builds in interesting ways and then expect Homebrew to know how to fix those use cases, and quickly. Homebrew has already seen quite a bit of that with the :ruby and to a slightly lesser degree :perl requirements.

I think it would be useful if the argument gets fleshed out a bit more than "Having two of these is annoying", especially when a plain Homebrew node installation clocks in at <50MB, which even on the smallest SSDs Apple ships these days is a sneeze of disk space.

sjmueller · 2017-01-13T20:15:16Z

In my experience it's been quite a hassle having two versions of node competing on the same system. When something goes wrong, which version is running? Did this particular terminal instance use a different node because a path variable was changed? If I'm hunting down a global dependency that I linked locally, am I looking in the right dir?

Much easier to troubleshoot when nvm is given autonomy to manage node versions, resulting in a single source of truth.

MikeMcQuaid · 2017-01-14T00:39:19Z

@sjmueller A solution has been provided for that. Let's keep the discussion off this PR now, thanks,.

michael-yx-wu · 2017-01-30T19:46:40Z

Would it also be possible to make this work for folks who use nodenv instead of nvm to manage node versions?

DomT4 · 2017-01-31T13:54:45Z

A requirement won't care where it's coming from as long as the node and npm executables are found. But someone would still need to complete any remaining work here, and convince the remaining maintainers there's an inconvenience beyond a sneeze of disk space.

ilovezfs · 2017-01-31T14:00:53Z

@DomT4 Wait, I'm confused. So you're not volunteering to implement :rust, :go, :node, :swift, :mono, :maven, and :opam? I had my hopes up!

DomT4 · 2017-01-31T14:01:49Z

@ilovezfs All of those being a thing might actually be enough to push me to find an alternative package manager, and I used to help maintain this one 😆.

ilovezfs · 2017-01-31T14:02:51Z

I think you and fink will get along swimmingly.

DomT4 · 2017-01-31T14:04:05Z

Come on now, I didn't say I hated myself and felt the deep need for self-punishment 😉. (Mike's going to tell us off for dragging this off-topic any second, heh).

MikeMcQuaid · 2017-03-19T16:50:03Z

Closing as this has stalled.

SimenB · 2017-03-31T07:07:04Z

For now avoid using the :node requirement and just demonstrate how npm install isn't required, thanks!

Homebrew/homebrew-core#11881

reitermarkus previously requested changes Oct 30, 2016

View reviewed changes

bfontaine mentioned this pull request Nov 1, 2016

Allow some way of specifying "Node.js-ish" dependency Homebrew/homebrew-core#6458

Closed

torifat mentioned this pull request Nov 1, 2016

Homebrew should not install node if nvm is already installed yarnpkg/yarn#1116

Closed

MikeMcQuaid requested changes Nov 2, 2016

View reviewed changes

torifat force-pushed the feature/dependency-node branch 2 times, most recently from 1ec0125 to e9789e3 Compare November 2, 2016 18:58

Add :node special dependency

4587305

torifat force-pushed the feature/dependency-node branch from a28308c to 4587305 Compare November 2, 2016 20:26

torifat force-pushed the feature/dependency-node branch 2 times, most recently from 4bbeec1 to 8b70786 Compare November 3, 2016 13:43

Add test for node_requirement

937da25

torifat force-pushed the feature/dependency-node branch from 8b70786 to 937da25 Compare November 3, 2016 14:14

antonakv mentioned this pull request Nov 30, 2016

error: the requested upstream branch 'antonakv/cask-repair_update-deadbeef' does not exist vitorgalvao/tiny-scripts#52

Closed

MikeMcQuaid mentioned this pull request Dec 1, 2016

Brew can not find installed dependencies if they are installed in nonstandard way #1601

Closed

MikeMcQuaid closed this Mar 19, 2017

SimenB mentioned this pull request Apr 17, 2017

0.23.2 is missing on npm yarnpkg/yarn#3148

Closed

Homebrew locked and limited conversation to collaborators May 14, 2017

Add :node special dependency #1408

Add :node special dependency #1408

Conversation

torifat commented Oct 30, 2016 • edited Loading

reitermarkus Oct 30, 2016

Choose a reason for hiding this comment

torifat Oct 30, 2016

Choose a reason for hiding this comment

MikeMcQuaid Nov 2, 2016

Choose a reason for hiding this comment

torifat Nov 2, 2016

Choose a reason for hiding this comment

DomT4 Nov 2, 2016

Choose a reason for hiding this comment

torifat commented Nov 2, 2016

DomT4 commented Nov 2, 2016

torifat commented Nov 2, 2016 • edited Loading

torifat commented Nov 3, 2016 • edited Loading

torifat commented Nov 5, 2016

MikeMcQuaid commented Nov 5, 2016

torifat commented Nov 5, 2016

Daniel15 commented Nov 5, 2016 • edited Loading

MikeMcQuaid commented Nov 6, 2016

torifat commented Nov 6, 2016

MikeMcQuaid commented Nov 6, 2016

ilovezfs commented Nov 14, 2016

MikeMcQuaid commented Nov 15, 2016

torifat commented Nov 15, 2016

MikeMcQuaid commented Nov 15, 2016

torifat commented Nov 15, 2016

MikeMcQuaid commented Nov 15, 2016

gsklee commented Jan 10, 2017

tonytonyjan commented Jan 11, 2017

MikeMcQuaid commented Jan 11, 2017

sjmueller commented Jan 13, 2017

ilovezfs commented Jan 13, 2017

DomT4 commented Jan 13, 2017 • edited Loading

MikeMcQuaid commented Jan 13, 2017

sjmueller commented Jan 13, 2017

Daniel15 commented Jan 13, 2017

jeanregisser commented Jan 13, 2017

ilovezfs commented Jan 13, 2017

MikeMcQuaid commented Jan 13, 2017

Daniel15 commented Jan 13, 2017 via email

torifat commented Jan 13, 2017

DomT4 commented Jan 13, 2017

sjmueller commented Jan 13, 2017

MikeMcQuaid commented Jan 14, 2017

michael-yx-wu commented Jan 30, 2017

DomT4 commented Jan 31, 2017

ilovezfs commented Jan 31, 2017

DomT4 commented Jan 31, 2017

ilovezfs commented Jan 31, 2017

DomT4 commented Jan 31, 2017

MikeMcQuaid commented Mar 19, 2017

SimenB commented Mar 31, 2017 • edited Loading

torifat commented Oct 30, 2016 •

edited

Loading

torifat commented Nov 2, 2016 •

edited

Loading

torifat commented Nov 3, 2016 •

edited

Loading

Daniel15 commented Nov 5, 2016 •

edited

Loading

DomT4 commented Jan 13, 2017 •

edited

Loading

SimenB commented Mar 31, 2017 •

edited

Loading