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

node 8.9.0 + npm 5.5.1 #20110

Closed
wants to merge 3 commits into from
Closed

node 8.9.0 + npm 5.5.1 #20110

wants to merge 3 commits into from

Conversation

chrmoritz
Copy link
Contributor

@chrmoritz chrmoritz commented Oct 31, 2017

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This PR updates node to version 8.9.0 - the new LTS Carbon release - and npm in all node formulae to 5.5.1 (the version shipped with 8.9.0).

The actual node 9 release will follow later today.

@ilovezfs
Copy link
Contributor

Your npx fix didn't make it into 5.5.1? :(

@chrmoritz
Copy link
Contributor Author

Npm 5.5.1 was released like a month ago, which was before my fix.

@ilovezfs
Copy link
Contributor

Pity.

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Oct 31, 2017

Also with node v9 released later today we have to float nodejs/node@0ea8ff3deb and nodejs/node@237067d54e over npm too (like node upstream does) for node 9 support (see nodejs/node#16509).

Is it enough to only float these node 9 specific patches in the node v9 formula or do we want to float these over the node@{4,6,8} formulae too (to have the exact same copy of npm in every formula)? If the later it may be worth starting to float it now as a preparation for the node v9 release.

@ilovezfs ilovezfs closed this in 993f0d6 Oct 31, 2017
@ilovezfs
Copy link
Contributor

ilovezfs commented Oct 31, 2017

Thanks @chrmoritz!

exact same copy of npm

this.

I'd be tempted to wait for a comptible npm as the patching sounds ridiculous for a so-called release.

@ilovezfs
Copy link
Contributor

node 9 is now out.

@chrmoritz
Copy link
Contributor Author

Node 9 is out now: https://nodejs.org/en/blog/release/v9.0.0/
Waiting for a stable version of npm, which is out of the box compatible with node v9 could take some weeks and I think that most homebrew node users don't want to wait so long for a formula with node v9.

@ilovezfs There isn't support for patches in resources in homebrew, right? What would the the best approve to patch npm? Put the patch in a second resource and then call system "patch", ... to apply the patch to the npm sources. Or we could just use the prepatched npm sources from the node 9 tarball inside deps/npm to build our npm version?

@ilovezfs
Copy link
Contributor

Put the patch in a second resource and then call system "patch", ... to apply the patch to the npm sources.

This.

elm.rb
74: system "patch", "-p1", "-i", Pathname.pwd/"287.patch", "-d",

llvm.rb
199: system "patch", "-p1", "-i", Pathname.pwd/"324f93b5e30.patch", "-d", buildpath/"tools/lldb"

osquery.rb
130: system "patch", "-p1", "-i", "patch-thrift-0.10.diff"

riak.rb
60: system "patch", "-p1", "-i", buildpath/"6.patch", "-d", "deps/hyper"

reapr.rb
61: system "patch", "-p1", "-i", Pathname.pwd/"64275b4.patch", "-d",

@DomT4
Copy link
Member

DomT4 commented Oct 31, 2017

Waiting for a stable version of npm, which is out of the box compatible with node v9 could take some weeks

Life is fun 🙃.

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Oct 31, 2017

@ilovezfs Unfortunately its not even that easy in our case, because we would need to patch our resource("npm").cached_download too:

bootstrap = buildpath/"npm_bootstrap"
bootstrap.install resource("npm")
system "node", bootstrap/"bin/npm-cli.js", "install", "-ddd", "--global",
"--prefix=#{libexec}", resource("npm").cached_download

So we would need to:

  • bootstrap.install resource("npm")
  • apply the 2 patches npm/npm@4ca6958 and npm/npm@f7eb94a
  • npm pack --ignore-scripts it to create our own patched npm tarball
  • use this tarball instead of resource("npm").cached_download for the installation

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Oct 31, 2017

Alternatively chrmoritz@81d6e3e is a node 9 formula with the unstable npm canary npmc-5.5.1-canary.4 as npm, which already contains the minizlib update and my npx patch (but still warns about node 9 being unsupported because of missing npm/npm@f7eb94a).

@chrmoritz
Copy link
Contributor Author

@ilovezfs What do you think about shipping a canary version of npm (see comment above) for node 9 an upgrade to an non-canary npm version as soon as a node 9 compatible version is available as npm@next?

The formula linked above in chrmoritz@81d6e3e is working fine for me as long as you don't try to update npm itself (which is currently broken for every npm version patched for node 9 out there, because no stable official version does support node 9 yet).

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 1, 2017

I'm still leaning toward waiting on a new npm release. This situation doesn't make much sense to me when npm update doesn't even work.

@DomT4
Copy link
Member

DomT4 commented Nov 1, 2017

as long as you don't try to update npm itself

FWIW, if we went down this path I think we would literally have to write a temporary wrapper around npm that bailed hard if you tried to execute npm install -g npm, which is... silly. Support requests are already getting filed against both node and npm upstream. I kinda agree we have to wait on npm here, for better or worse.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 1, 2017

Yeah I sympathize with their chicken-and-egg situation but I don't think we need to get sucked into it.

chrmoritz referenced this pull request Nov 2, 2017
robohack pushed a commit to robohack/homebrew-core that referenced this pull request Nov 3, 2017
Closes Homebrew#20110.

Signed-off-by: ilovezfs <ilovezfs@icloud.com>
@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 7, 2017

@chrmoritz @DomT4 and now 9.1.0 is out 🙈

@DomT4
Copy link
Member

DomT4 commented Nov 7, 2017

What is going on with npm upstream? Seems to have been an almost one-person show on the commit side of things for a few months now.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 7, 2017

@DomT4 maybe we can just start shipping node with yarn.

@DomT4
Copy link
Member

DomT4 commented Nov 7, 2017

I really need to spend some more time getting to know yarn. I've just kinda stuck with npm resolutely because of how much time I've plowed into getting it into a semi-stable state within Homebrew over the years, heh.

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Nov 7, 2017

@DomT4 There are more than 1 person working on the npm cli, but you have to look deeper into npms dependencies which are developed by other npm cli maintainers. Also the way we handle npm in homebrew is way more stable than the was we handle yarn atm.

Npm@5.3.0 or lower will work with node 9 and npm@5.6+ (currently in canary) will work with that node 9 too. In theory we could just ship node 9 with one of these version.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 7, 2017

I think until people can do npm install npm@latest without things going 💥 we need to hold the line.

@DomT4
Copy link
Member

DomT4 commented Nov 7, 2017

Also the way we handle npm in homebrew atm is way more stable than the was we handle yarn atm.

Yeah, no disagreement here. I don't actually use yarn from Homebrew for this reason, I've stuck to installing it via npm.

In theory we could just ship node 9 with one of these version.

My concern is mostly around people not being able to update their npm versions or even try to without breaking all the things 😅. I don't know how we prevent that situation in a reasonable manner. As for the canary version, I feel like that's inviting trouble to roll out to users en mass. I still think waiting is the best thing to do here, but it's not exactly a grab bag of great options.

@chrmoritz
Copy link
Contributor Author

On the one hand I can understand your concerns about breaking npm self upgrades. On the other hand we keep a working npm version in libexec for exactly the same reasons, which could be used to easily restore npm by running brew postinstall node. We also never prevented people from doing other stupid npm installation destroying things like npm i -g npm@1 or npm rm -g npm or npm update -g npm in the past.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 7, 2017

npm rm -g npm

except that's exactly the advice being given to people with broken npm: npm/npm#19019 (comment)

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Nov 7, 2017

@ilovezfs This issue you are linking to is related to Windows user with a PATH setup were a local copy of npm (5.4-5.5) comes before the patched copy of npm installed by the official node installer. Removing the local copy of npm will lead to the patched and working copy of npm from the node installer to be picked up.

This is totally unrelated to our setup, where running npm rm -g npm would just delete the copy of npm from HOMEBREW_PREFIX leaving only the copy of npm in libexec intact (but not in the user PATH).

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 7, 2017

@chrmoritz I know. And do you want to be the one to explain that to each Homebrew user that breaks their setup doing that?

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Nov 8, 2017

I've proposed a formula with node 9.1.0 and a patched version of npm 5.5.1 (with node 9 compatibility which persists self upgrades) in #20431.

@ilovezfs
Copy link
Contributor

ilovezfs commented Nov 9, 2017

We're waiting for a compatible npm as I already said.

@Homebrew Homebrew locked and limited conversation to collaborators Nov 9, 2017
@Homebrew Homebrew unlocked this conversation Nov 28, 2017
@ilovezfs
Copy link
Contributor

@chrmoritz @DomT4 5.6.0 (pre-release) is now out.

@DomT4
Copy link
Member

DomT4 commented Nov 28, 2017

Those are some nice features as well. Hopefully that's cut stable soon and things can move ahead fairly rapidly.

@ilovezfs
Copy link
Contributor

Maybe we should put node 9.2.0 and npm 5.6.0 in a devel spec?

@DomT4
Copy link
Member

DomT4 commented Nov 28, 2017

Seems reasonable enough, although I'd personally be surprised if upstream sit on the release for too long given the incompatibility with the newest node. At least, hopefully. It'd be nice to finally be done with this issue.

@ilovezfs
Copy link
Contributor

The planned actual release date is December 7th npm/npm#19019 (comment)

@DomT4
Copy link
Member

DomT4 commented Nov 28, 2017

That's not too bad a wait, but if you want to move ahead and set up a temporary devel block I've no objections to that 👍.

@ilovezfs
Copy link
Contributor

#21128

@ilovezfs
Copy link
Contributor

brew install --devel node will now give you node 9.2.0 + npm 5.6.0

@chrmoritz
Copy link
Contributor Author

@ilovezfs npm@5.6.0 is now marked as stable npm@latest, see npm info npm dist-tags

@DomT4
Copy link
Member

DomT4 commented Dec 8, 2017

+ npm@5.6.0
added 27 packages, removed 11 packages and updated 38 packages in 15.565s

Yup. Decent timing given new node releases are imminent due to the OpenSSL vulnerabilities disclosed & fixed very recently.

@ilovezfs
Copy link
Contributor

ilovezfs commented Dec 8, 2017

So how do we feel about upgrading to node 9 with 5.6.0 for stable now vs. waiting?

@ilovezfs
Copy link
Contributor

ilovezfs commented Dec 8, 2017

For your consideration: 9.2.1

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
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