Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

node: add support for statically linking icu4c #44147

Closed
wants to merge 1 commit into from

Conversation

chrmoritz
Copy link
Contributor

This PR changes the Node.js formula's behavior from optionally dynamically linking against a brewed icu4c to statically linking to a resource staged icu4c by default to avoid issues with different c++ standard libraries, see #36681. A resource is needed, because otherwise the configure script would download icu4c itself (after passing an additional configure argument), which is against the formula guidelines.

For previous discussion see #44110 and #43973 (comment) (and the following comments).

By default Node.js will now be build with 'small-icu' support, which matches the current distributed Node.js binaries. This means the full Intl API is supported, but only the English locale is bundled to reduce file size (see also nodes Intl/icu docs). For getting support other locales you can either build --with-full-icu or load additional locales at runtime as described here.

A test for testing Intl support was added too.


deprecated_option "enable-debug" => "with-debug"
deprecated_option "with-icu4c" => "with-full-icu"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we should keep this or not:

  • argument for keeping this: no side effect for someone who has previously build node --with-icu4c and is relying on non English locales (through he could add other locales at runtime to a small-icu build as mentioned above)
  • argument against keeping this: someone who needs only the English local and has previously build node --with-icu4c would have an unnecessary big node installation and would have to build it from source instead of downloading a smaller bottle

@chrmoritz
Copy link
Contributor Author

I think it would be good to backport this to the iojs (and maybe node 0.12) formula too, once this is merged. What's the current plan for the iojs formula since it's now in maintenance? Should it stay in the main repo or should it be moved to homebrew-versions to the other older node releases currently in LTS/maintance.

Also npm 3.x is now stable. What is the plan for this? Upgrading to is asap or stay with the npm 2.x release line for the Node.js 4.x releases and upgrade to npm 3.x with the Node.js 5.0 release to match upstream? I'm personally +0 for waiting for the Node.js 5.0 release for consistency with upstream releases, although I'm currently using npm 3 already.

@bcomnes
Copy link
Contributor

bcomnes commented Sep 21, 2015

It was my understanding that the goal was to stay consistent with the latest node installers, so that would mean waiting till the installer ships npm3 before the formula here has it. People who know what they are doing are free to npm i npm@latest -g which only takes a moment.

As for back porting this to iojs, I can put together some proposal PRs that would share the staging code between the two formula so that there is less duplication.

Not sure how to handle the depreciate option.

Otherwise, LGTM!

@chrmoritz
Copy link
Contributor Author

The approve of this PR is to handle icu4c consistent with the latest node installers, but in the past the npm in the node formula was not alway consistent with upstream releases (thats why it downloads the npm sources as a resource instead of using the npm sources shipped with node).
Thats why I asking for the maintainers opinions about how to handle npm 3 in homebrew.
As I'm mentioned above I'm too slightly in favor of waiting for the Node.js 5 release before bumping npm to 3.x.

Porting this to the iojs formula should be really straight forward. We just have to use this formula and replace all references to nodejs 4 with the current iojs and add a conflict with node.

@DomT4
Copy link
Member

DomT4 commented Sep 21, 2015

About to go catch some 💤, but short and sweet answers:

  • I'm fine with the option being deprecated as such. If people were using icu4c before it seems reasonable they'd want the full ICU experience.
  • I'm fine with backporting it to iojs although I'd like to move iojs to Versions at this point or in the near future, ideally.
  • I don't really care about staying in line with the upstream node installations and what they ship in terms of npm, but I think we should hold off on npm3 until it does ship with a Node release in this case, unless some security problem arises that won't be fixed in npm2.

@DomT4
Copy link
Member

DomT4 commented Sep 21, 2015

Merged in ab0a4ef. Thank you for your contribution to Homebrew @chrmoritz!

@DomT4 DomT4 closed this in ab0a4ef Sep 21, 2015
@srl295
Copy link
Contributor

srl295 commented Oct 8, 2015

Late to the party, but FYI you can pass --with-icu-source=<path-to-source>.zip as a parameter to node's configure which might be less fragile than passing in the file path. What you committed assumes a specific ICU version will be picked up by node, master has already been bumped to ICU 56

@chrmoritz
Copy link
Contributor Author

From reading your configure script the only thing --with-icu-source=<path-to-source>.zip would do is to extract the contents of the icu.zip to deps/icu. This is exactly the same thing resource("icu4c").stage buildpath/"deps/icu" is doing with the only difference thats it's done on homebrews side in ruby. Please correct me if I missed another feature of the with-icu-source parameter.

Regarding the ICU version: I think the plan is to stay aligned with the upstream ICU version (bump the ICU resource to 56.1 with the node 4.2 upgrade).
BTW: The homebrew policy to do all resource downloading and checksum checking on homebrews side is the reason why ICU is a resource in this formula.

@srl295
Copy link
Contributor

srl295 commented Oct 9, 2015 via email

@chrmoritz chrmoritz deleted the node4icu branch October 25, 2015 19:09
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 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.

None yet

4 participants