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

update to node v7.0.0 #6261

Closed
wants to merge 1 commit into from
Closed

Conversation

matuzalemsteles
Copy link
Contributor

Update to node v7.0.0

@zmwangx zmwangx closed this in 1836df4 Oct 25, 2016
@zmwangx
Copy link
Contributor

zmwangx commented Oct 25, 2016

Merged as 1836df4. Thank you for your contribution to Homebrew!

For the record, npm can be updated to 3.10.9 and icu4c can be updated to 58.1, but I don't think those are necessary, and sticking to a slightly older version might even be safer at this point. npm can update itself anyway.

@JLHwung
Copy link
Contributor

JLHwung commented Oct 26, 2016

@zmwangx I think we'd better track upstream resource dependencies. Here is a list of resource dependencies PR:

As of Oct. 26th, both deps updates have not been merged to 7.x. So

sticking to a slightly older version

is a decision from upstream and we will bump dependencies once the upstream does.

@srl295
Copy link
Contributor

srl295 commented Oct 26, 2016

@JLHwung ICU 58.1 doesn't merge into the existing node codebase on Windows due to a build issue. But it should land in 7.x shortly as the referenced issue notes.

Looks like homebrew is still using ICU as "static linked" into node and not trying to leverage homebrew's icu4c formula. I'm still entirely not sure why you go to the extra work of dealing with ICU specially in the node formula when you have a perfectly good icu4c formula. C runtimes or something.

@zmwangx
Copy link
Contributor

zmwangx commented Oct 26, 2016

I'm still entirely not sure why you go to the extra work of dealing with ICU specially in the node formula when you have a perfectly good icu4c formula. C runtimes or something.

804c188
Homebrew/legacy-homebrew#44147

@zmwangx
Copy link
Contributor

zmwangx commented Oct 26, 2016

On 10.12,

$ brew linkage node
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /usr/lib/libSystem.B.dylib
  /usr/lib/libc++.1.dylib

so the original argument may not stand. May need testing on 10.10.

@DomT4
Copy link
Member

DomT4 commented Oct 26, 2016

As of the latest release Node finally builds against libc++ rather than libstdc++, so the formula could go back to using Homebrew's icu4c optionally, if the remaining maintainers deemed it desirable.

@ilovezfs
Copy link
Contributor

@DomT4 the reason being to avoid the icu4c vendored resource?

@DomT4
Copy link
Member

DomT4 commented Oct 27, 2016

@ilovezfs That would be an upside, yeah. Using a Homebrew icu4c with --with-intl=system-icu saves a touch of build time against the current --with-full-icu option (~5:30m Vs ~7:02m currently).

The bigger question really is whether that option would burst into flames if Homebrew updates icu4c out-of-sync with Node, which would have happened already if the icu4c formula wasn't outdated currently.

  • Edit - Tidied up first sentence for clarity.

@ilovezfs
Copy link
Contributor

If the icu4c formula is used, it sounds like maybe depends_on "icu4c" should be the default to make sure it gets tested properly during both node CI and icu4c CI.

@DomT4
Copy link
Member

DomT4 commented Oct 27, 2016

My personal viewpoint is that there's never been enough interest to merit dumping the semi-heavy dependency onto the node formula by complete default, especially since upstream themselves don't provide full locale support (they only ship the small-icu component). That said, it's your decision now, I only really dropped by to help explain the original change 🙈.

Edit - Wording, again.

@ilovezfs
Copy link
Contributor

ilovezfs commented Oct 27, 2016

@DomT4 another option would be to make sure at least one of the bottled reverse dependencies builds with depends_on "node" => "with-icu4c" and depends_on "icu4c", which should cause it to be semi tested.

@srl295
Copy link
Contributor

srl295 commented Oct 27, 2016

Hi again @DomT4 , and thanks.

The bigger question really is whether that option would burst into flames if Homebrew updates icu4c out-of-sync with Node

It shouldn't fail if Homebrew's ICU is too old … For that matter, node upstream (basically me) could actually make sure that the ICU underneath was a sufficient minimum version. You'd have to dig pretty far back to break it at this point, though. But I'll file nodejs/Intl#35

As far as a newer ICU, which Homebrew is building separately from node, we [http://userguide.icu-project.org/design#TOC-ICU-API-compatibility](try not to break things there). The main issues come in actually building ICU.

If either of these needed mitigation (too old or too new), I'd think node.rb could pin icu4c to a specific version.

especially since upstream themselves don't provide full locale support (they only ship the small-icu component).

…which is only and entirely for mitigating download size.

edit anyway, hope this helps.

@DomT4
Copy link
Member

DomT4 commented Oct 29, 2016

It shouldn't fail if Homebrew's ICU is too old

Barring all the remaining Homebrew maintainers getting into the same bus which then falls off a cliff I can't foresee a situation where Homebrew's ICU is too old, to be honest. It can lag a little but eventually someone braves the latest update within a month of release or so.

As far as a newer ICU, which Homebrew is building separately from node, we http://userguide.icu-project.org/design#TOC-ICU-API-compatibility. The main issues come in actually building ICU.

This was the point of concern in my earlier comment, yeah. Homebrew pouring say icu4c version 58.1 and node linking to that (even optionally) and it blowing up because there's no support from Node upstream for breaking changes on the ICU side yet.

@srl295
Copy link
Contributor

srl295 commented Oct 29, 2016

I continuously test node against HEAD Icu to keep that from happening
specifically, just because it's easy to test that way. We have been pretty
good about not breaking anything, not counting a few things that were
already broken, and that with good notice.

On Saturday, October 29, 2016, Dominyk Tiller [email protected]
wrote:

It shouldn't fail if Homebrew's ICU is too old

Barring all the remaining Homebrew maintainers getting into the same bus
which then falls off a cliff I can't foresee a situation where Homebrew's
ICU is too old, to be honest. It can lag a little but eventually someone
braves the latest update within a month of release or so.

As far as a newer ICU, which Homebrew is building separately from node, we
http://userguide.icu-project.org/design#TOC-ICU-API-compatibility. The
main issues come in actually building ICU.

This was the point of concern in my earlier comment, yeah. Homebrew
pouring say icu4c version 58.1 and node linking to that (even optionally)
and it blowing up because there's no support from Node upstream for
breaking changes on the ICU side yet.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6261 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0Ms0Kyz3pbVln0HNwKEXwAoXOpP4k6ks5q43QJgaJpZM4KgFt8
.

@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.

6 participants