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

build: add libicu i18n support #6559

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Adds a --with-icu-path= switch to the configure script. Requires that the user
checks out the copy of libicu that's bundled with chromium to a fixed directory.
It's still a little rough around the edges but it works.

Fixes #6371.

Suggested reviewers: @tjfontaine @trevnorris

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit bnoordhuis/node@fc081ff has the following error(s):

  • Commit message line too long: 2
  • Commit message line too long: 3

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@@ -16,6 +16,15 @@ Unix/Macintosh:
make
make install

With libicu i18n support:
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm considering rewording this to "With libicu i18n support (UNSUPPORTED):"

Choose a reason for hiding this comment

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

agreed.

Adds a --with-icu-path= switch to the configure script.  Requires that
the user checks out the copy of libicu that's bundled with chromium to
a fixed directory.  It's still a little rough around the edges but it
works.

Fixes nodejs#6371.
@trevnorris
Copy link

+1 from me.

@tjfontaine
Copy link

LGTM, can we use a git example in the readme instead of svn?

@bnoordhuis
Copy link
Member Author

can we use a git example in the readme instead of svn?

Not really. It only works with the libicu that's bundled with chromium. You can't do partial checkouts with git and the full chromium tree is a whopping 12 GB.

@trevnorris
Copy link

@davglass Could you try this patch and see if it fits your needs?

@davglass
Copy link

davglass commented Dec 3, 2013

Ping @rohiniwork, can you check out this patch?

@rohiniwork
Copy link

This worked for me, except for the I had to run configure with the gyp path

./configure --with-icu-path=deps/v8/third_party/icu46/icu.gyp

This is because v8.gyp assumes the gyp file is specified, not just the directory
'<(icu_gyp_path):icui18n'

Thanks,
--rohini

@davglass
Copy link

davglass commented Dec 4, 2013

Same here, I had to change the configure line like above. If we update the README with that bit I'd be happy that this is merged :)

@trevnorris
Copy link

This was merged in 9e32a7d

@trevnorris trevnorris closed this Dec 6, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants