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

deps: backport char16ptr.h to 57 #19658

Closed
wants to merge 2 commits into from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Mar 28, 2018

  • char16ptr.h is appended to the ICU 57 unistr.h
  • this way, no v8 code change is needed.

Fixes: #19656

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 28, 2018
@srl295 srl295 self-assigned this Mar 28, 2018
@nodejs-github-bot nodejs-github-bot added i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Mar 28, 2018
@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018

note that #19624 is also needed

- char16ptr.h is appended to the ICU 57 unistr.h
- this way, no v8 code change is needed.

Fixes: nodejs#19656
@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018

^ doesn't actually work (why did it seem to work locally?)… my 'floating patch' mechanism doesn't support header file patching.

@srl295 srl295 added the wip Issues and PRs that are still a work in progress. label Mar 28, 2018
@bnoordhuis
Copy link
Member

For my curiosity: is there a reason we try to support old versions of ICU? It doesn't work when you try to link against the system ICU (logical) so it seems like the only use case is people downloading an old tarball manually and extracting that to deps/icu - but why would anyone do that?

@srl295
Copy link
Member Author

srl295 commented Mar 28, 2018

Can test with:

configure --with-icu-source=https://ssl.icu-project.org/files/icu4c/57.1/icu4c-57_1-src.zip --download=all

@bnoordhuis good point… my point was for system-icu. But, floating patches against icu don't affect an icu that's already built and part of the system.

but why would anyone do that

The only benefit for doing "that" would be to verify compatibility (see #19657 ). But I think more importantly, the benefit of this PR is misguided: Fixing v8 would actually allow linking against the older system-icu.

@srl295 srl295 removed the wip Issues and PRs that are still a work in progress. label Mar 28, 2018
@srl295
Copy link
Member Author

srl295 commented Apr 3, 2018

#19710 actually solves the issue instead of hacking around it. The only benefit to this PR is that it fixes the floating patch mechanism to support patching header files (below). I'll close this for now.

diff --git a/configure b/configure
index df59b59e207..13fca229bf6 100755
--- a/configure
+++ b/configure
@@ -34,6 +34,8 @@ import shlex
 import subprocess
 import shutil
 import string
+import distutils
+from distutils import dir_util
 
 # If not run from node/, cd to node/.
 os.chdir(os.path.dirname(__file__) or '.')
@@ -1354,6 +1356,10 @@ def configure_intl(o):
     print('See the README.md.')
     # .. and we're not about to build it from .gyp!
     sys.exit(1)
+  # If we're not using the in-tree ICU, copy over any patched files
+  if with_icu_source and os.path.isdir('tools/icu/patches/%s' % icu_ver_major):
+    print('Copying ICU patches from tools/icu/patches/%s to %s' % (icu_ver_major, icu_full_path))
+    distutils.dir_util.copy_tree('tools/icu/patches/%s' % icu_ver_major, icu_full_path)
   # map from variable name to subdirs
   icu_src = {
     'stubdata': 'stubdata',

@srl295 srl295 closed this Apr 3, 2018
@srl295 srl295 deleted the icu-backport-char16ptr branch April 3, 2018 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v8: unnecessary call to icu::toUCharPtr requires ICU 58+
4 participants