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

Adding Node V14 Formula #63410

Closed
wants to merge 13 commits into from
Closed

Adding Node V14 Formula #63410

wants to merge 13 commits into from

Conversation

eveenendaal
Copy link
Contributor

@eveenendaal eveenendaal commented Oct 24, 2020

  • 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?
  • Is your test running fine brew test <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>)?

@BrewTestBot BrewTestBot added legacy Relates to a versioned @ formula new formula PR adds a new formula to Homebrew/homebrew-core python Python use is a significant feature of the PR or issue labels Oct 24, 2020
@SMillerDev
Copy link
Member

Does it meet all requirements in https://docs.brew.sh/Versions?

@eveenendaal
Copy link
Contributor Author

eveenendaal commented Oct 24, 2020

Yes. 14 is the next LTS version of node and meets all the version requirements.

Co-authored-by: Jason Karns <jason.karns@gmail.com>
@jasonkarns
Copy link
Contributor

jasonkarns commented Oct 24, 2020

I was about to submit a handful of code suggestions regarding npm, but I'll just ask it here instead.

Question for maintainers:
The node@12 formula does not use the homebrew npm as the main node formula does. Is that intentional? (I'm assuming it is, so that the versioned-node does indeed just use its bundled npm instead of sharing npm with the unversioned node.)

If that is indeed the case, then this PR should be adjusted to remove the homebrew-specific npm installation and should use the node@12 formula as a guide.

Separately, I'll add my (non-maintainer) 👍 to having v14 as a versioned formula, since v14 will be one of the LTS releases of node. It seems a helpful practice for brew to have the LTS releases available as versioned formula whenever the latest version of node is odd-numbered (ie, will not be supported longterm by nodejs).

@eveenendaal
Copy link
Contributor Author

I'm no node expert; just a user. The main node formula switched to v15, and no one had started a v14 pull request yet, so I got one started. It's just the last v14 commit from master with the modifications I saw in the other LTS formulas. I'm hoping the maintainers from the main formula can chip in. I'm not really sure what their intention is with NPM version or the python build environment. The build takes quite a bit of time, so it would be a bit of a learning curve for me to figure it out.

@Rylan12 Rylan12 mentioned this pull request Oct 24, 2020
4 tasks
@fxcoudert
Copy link
Member

From our docs:

A keg_only :versioned_formula should not post_install anything in the HOMEBREW_PREFIX that conflicts with or duplicates the main counterpart (or other versioned formulae). For example, a node@6 formula should not install its npm into HOMEBREW_PREFIX like the node formula does.

So basically all of your current post_install should be removed.

Also, to answer the other question:

Versioned formulae should not have resources that require security updates. For example, a node@6 formula should not have an npm resource but instead rely on the npm provided by the upstream tarball.

@derrabus
Copy link
Contributor

Thank you for working on this! I work on some projects that weren't ready for node 15 yet, so Homebrew's upgrade kind-of broke my local setup. Having a versioned formula for node 14 would be much appreciated.

Making the post_install look like the rest of the versioned node formula
@eveenendaal
Copy link
Contributor Author

From our docs:

A keg_only :versioned_formula should not post_install anything in the HOMEBREW_PREFIX that conflicts with or duplicates the main counterpart (or other versioned formulae). For example, a node@6 formula should not install its npm into HOMEBREW_PREFIX like the node formula does.

So basically all of your current post_install should be removed.

Also, to answer the other question:

Versioned formulae should not have resources that require security updates. For example, a node@6 formula should not have an npm resource but instead rely on the npm provided by the upstream tarball.

I've made that change. Thanks.

Eric Veenendaal added 3 commits October 26, 2020 07:13
Matching the other versioned node installs even more.
@derrabus
Copy link
Contributor

I've tested your formula locally and tried to link it instead of the node formula:

brew link --force --overwrite node@14
Linking /usr/local/Cellar/node@14/14.14.0... 3808 symlinks created

3808 seemed like an unreasonable high number of files to me. The node formula only creates seven symlinks:

brew link --dry-run node
Would link:
/usr/local/etc/bash_completion.d/npm
/usr/local/bin/node
/usr/local/include/node
/usr/local/share/doc/node
/usr/local/share/man/man1/node.1
/usr/local/share/systemtap
/usr/local/lib/dtrace/node.d

@eveenendaal
Copy link
Contributor Author

eveenendaal commented Oct 26, 2020

The install is only 3 lines long and is pretty much identical to the node 12 formula? Any idea what needs to change?

@eveenendaal
Copy link
Contributor Author

eveenendaal commented Oct 26, 2020

Interesting. The node@10 formula returns 4787.

brew link --dry-run node@10 | wc -l
4787

@derrabus
Copy link
Contributor

I don't really know how brew link works, sorry. But other than that, your formula works well. Thank you very much. 😃

@eveenendaal
Copy link
Contributor Author

@diogoazevedos It looks like you have quite a bit of experience maintaining the node@12 formula, do you happen to know the reason for the high number of linked files? Is it normal for node?

@billinghamj
Copy link
Contributor

billinghamj commented Oct 27, 2020

v14.15.0 will be the first LTS - v14.14.0 is not. Probably worth switching to/waiting for that: nodejs/node#35746 (will be later today)

@eveenendaal
Copy link
Contributor Author

The problem with waiting for v14.15.0 is that the master branch already moved to v15. Myself and I'm sure plenty of other people locked themselves into v14 since that was the next LTS branch and are now stuck with a broken build for a month or two. What's the harm/risk in setting up a node@14 formula now? The node@14 formula will be updated to v14.15.0 whenever that comes out.

I will personally guarantee you I will submit the 2 line pull request to upgrade to v14.15.0 whenever it comes out.

tl;dr. There is no risk/harm in setting up a v14 formula now (there is no dispute that v14 is the next LTS release and will be supported until 2023-04-30), but there is risk/harm in waiting for those that rely on homebrew for their node installs.

@diogoazevedos
Copy link
Contributor

It looks like you have quite a bit of experience maintaining the node@12 formula, do you happen to know the reason for the high number of linked files? Is it normal for node?

I've no idea why so many files are linked, but this is normal.

@fxcoudert
Copy link
Member

v14.15.0 will be the first LTS - v14.14.0 is not. Probably worth switching to/waiting for that: nodejs/node#35746 (will be later today)

If that is indeed in the next couple of days, let's do this: let's update this PR to 14.15.0 when it comes out, and we'll merge then.

stuck with a broken build for a month or two

I don't see where “a month or two” comes from.

Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
Formula/node@14.rb Outdated Show resolved Hide resolved
@eveenendaal
Copy link
Contributor Author

v14.15.0 will be the first LTS - v14.14.0 is not. Probably worth switching to/waiting for that: nodejs/node#35746 (will be later today)

If that is indeed in the next couple of days, let's do this: let's update this PR to 14.15.0 when it comes out, and we'll merge then.

stuck with a broken build for a month or two

I don't see where “a month or two” comes from.

It was just my pessimistic guess as to when the next minor release of the node would come out. Yes, I realize it was a bit hyperbolic and I'm sorry about that, but I'd like to switch back to v14 from v15 in my local setup as soon as possible. If it is later today, then yes; it wouldn't be much of a burden to wait.

Co-authored-by: Diogo Azevedo <diogoazevedos@gmail.com>
@SMillerDev
Copy link
Member

but I'd like to switch back to v14 from v15 in my local setup as soon as possible. If it is later today, then yes; it wouldn't be much of a burden to wait.

Brew really isn't trying to provide versions of everything so if you need a specific version a tool like Docker might be a better fit.

@eveenendaal
Copy link
Contributor Author

but I'd like to switch back to v14 from v15 in my local setup as soon as possible. If it is later today, then yes; it wouldn't be much of a burden to wait.

Brew really isn't trying to provide versions of everything so if you need a specific version a tool like Docker might be a better fit.

I get that, and I use docker for my local infrastructure for this specific reason. Believe me, I don't like installing libraries directly on my machine. I feel Node is a bit different and past precedent (a formula for node@10 and node@12) indicate that generally, the homebrew community has agreed.

Is the plan to remove node@10 and node@12 then?

@fxcoudert
Copy link
Member

Is the plan to remove node@10 and node@12 then?

Node 10 will be EOL in April 2021, so it would be deprecated then, and removed at some later point

@SMillerDev
Copy link
Member

The point was more "if your environment breaks on updates, you might want to avoid a rolling release package manager". LTS releases could be in brew for longer, but the focus for brew is really on the latest releases of all packages.

@martpie
Copy link

martpie commented Oct 27, 2020

Using brew as a package manager for Node.js is just extremely convenient, so I guess using brew pin node is something that could be used by people who need to be sure a rolling release does not introduce breaking changes in their setup.

@SMillerDev
Copy link
Member

brew extract node is a much better option there because it doesn't interfere with the workings of the rest of brew. It's just another formula.

@eveenendaal
Copy link
Contributor Author

Cool. I didn't know that existed. Thank you.

Side note though is the plan still to add merge this change in after v14.15.0 is released? or is the plan to stop adding node LTS versions and deprecate the existing ones as they reach end of life?

@fxcoudert
Copy link
Member

This versioned formula meets our requirements, so:

let's update this PR to 14.15.0 when it comes out, and we'll merge then

@billinghamj
Copy link
Contributor

It is now out :)

@eveenendaal
Copy link
Contributor Author

Finally! /sarcasm

@billinghamj Now I feel silly. My bad.

@fxcoudert
Copy link
Member

Thanks @eveenendaal for your pull request!

@BrewTestBot
Copy link
Member

:shipit: @fxcoudert has triggered a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy Relates to a versioned @ formula new formula PR adds a new formula to Homebrew/homebrew-core python Python use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants