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.

Sign up for GitHub

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

Jump to bottom

missing #include <unicode/ustring.h> #11753

Closed
srl295 opened this issue Mar 8, 2017 · 9 comments
Closed

missing #include <unicode/ustring.h> #11753

srl295 opened this issue Mar 8, 2017 · 9 comments
Assignees
@srl295
Labels
i18n-api Issues and PRs related to the i18n implementation.

Comments

@srl295
Copy link
Member

srl295 commented Mar 8, 2017

  • Subsystem: core/deps

An ICU header was missing from the #includes. This works on current ICU, but fails later when an indirect dependency changes.

diff --git a/deps/v8/src/runtime/runtime-i18n.cc b/deps/v8/src/runtime/runtime-i18n.cc
index 75e0952..c923c17 100644
--- a/deps/v8/src/runtime/runtime-i18n.cc
+++ b/deps/v8/src/runtime/runtime-i18n.cc
@@ -42,6 +42,7 @@
 #include "unicode/unistr.h"
 #include "unicode/unum.h"
 #include "unicode/uversion.h"
+#include "unicode/ustring.h"
 
 
 namespace v8 {
diff --git a/src/node_i18n.cc b/src/node_i18n.cc
index b337456..fab594c 100644
--- a/src/node_i18n.cc
+++ b/src/node_i18n.cc
@@ -43,6 +43,7 @@
 #include <unicode/ulocdata.h>
 #include <unicode/uvernum.h>
 #include <unicode/uversion.h>
+#include <unicode/ustring.h>
 
 #ifdef NODE_HAVE_SMALL_ICU
 /* if this is defined, we have a 'secondary' entry point.
@srl295 srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Mar 8, 2017
@srl295 srl295 self-assigned this Mar 8, 2017
@srl295
Copy link
Member Author

srl295 commented Mar 8, 2017

The second is a v8 patch.

@srl295 srl295 mentioned this issue Mar 8, 2017
Merged
2 tasks
srl295 added a commit to srl295/node that referenced this issue Mar 9, 2017
@srl295
src: add missing #include <unicode/ustring.h>
5efb15e 
* We use these functions that are declared in <unicode/ustring.h>

  u_strFromUTF8()
    u_strToUTF8()

* At present, <unicode/ustring.h> is indirectly included, but this will
likely change in future ICUs. Adding this header has been the right
thing to do for many years, so it is backwards compatible.

Fixes: nodejs#11753
PR-URL: nodejs#11754
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Mar 13, 2017
@srl295 @italoacasas
src: add missing #include <unicode/ustring.h>
032becd 
* We use these functions that are declared in <unicode/ustring.h>

  u_strFromUTF8()
    u_strToUTF8()

* At present, <unicode/ustring.h> is indirectly included, but this will
likely change in future ICUs. Adding this header has been the right
thing to do for many years, so it is backwards compatible.

Fixes: nodejs#11753
PR-URL: nodejs#11754
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
@srl295 @jungx098
src: add missing #include <unicode/ustring.h>
fddb23e 
* We use these functions that are declared in <unicode/ustring.h>

  u_strFromUTF8()
    u_strToUTF8()

* At present, <unicode/ustring.h> is indirectly included, but this will
likely change in future ICUs. Adding this header has been the right
thing to do for many years, so it is backwards compatible.

Fixes: nodejs#11753
PR-URL: nodejs#11754
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@srl295
Copy link
Member Author

srl295 commented Mar 27, 2017

v8 side: https://codereview.chromium.org/2738453005/ and https://codereview.chromium.org/2738503008/

@srl295 srl295 mentioned this issue Mar 27, 2017
Closed
@srl295 srl295 mentioned this issue Mar 27, 2017
Closed
2 tasks
@srl295
Copy link
Member Author

srl295 commented Mar 27, 2017 •
edited
Loading

  • v8 changes above still don't seem to have landed in node HEAD. Filed CrBug:6173

@jungshik
Copy link

https://chromium.googlesource.com/v8/v8/+/5.9.35 is the first v8 release with the v8-side fix. Can Node head roll v8 to that version or later, instead (while rolling ICU to 59) ?

@srl295
Copy link
Member Author

srl295 commented Mar 29, 2017

@jungshik will investigate that- the back port would allow older nodes to support newer icu though.

srl295 added a commit that referenced this issue Mar 30, 2017
@srl295
tools: add missing #include "unicode/putil.h"
b084907 
* we use u_setDataDirectory() in "unicode/putil.h"
* at present, this header is indirectly included,
  but this will change in ICU 59
* no impact on past ICUs.
* this is an exact analog to #11753

PR-URL: #12078
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Apr 10, 2017
@srl295 @italoacasas
tools: add missing #include "unicode/putil.h"
fdf76d5 
* we use u_setDataDirectory() in "unicode/putil.h"
* at present, this header is indirectly included,
  but this will change in ICU 59
* no impact on past ICUs.
* this is an exact analog to nodejs#11753

PR-URL: nodejs#12078
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@MylesBorins
Copy link
Contributor

@srl295 should this be backported to v6.x?

@srl295
Copy link
Member Author

srl295 commented Apr 18, 2017

@MylesBorins it doesn't hurt… it could backport to v0.12 for that matter… however, without the (trivial) v8 patch, it's not going to work in previous versions.

So yes, it would probably be a good idea to back port it.

@srl295
Copy link
Member Author

srl295 commented Apr 18, 2017

re v8 5.9.35: #11827

@srl295 srl295 mentioned this issue Apr 18, 2017
Closed
3 tasks
@refack refack mentioned this issue Apr 27, 2017
Closed
@srl295
Copy link
Member Author

srl295 commented May 8, 2017 •
edited
Loading

For the record: This issue was fixed by a v8 bump in 60d1aac specifically 60d1aac#diff-df47c9a27158d9fcec0511ec405f9172

@refack refack mentioned this issue May 14, 2017
Closed
@ArchangeGabriel ArchangeGabriel mentioned this issue May 16, 2017
Closed
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
@srl295 @MylesBorins
tools: add missing #include "unicode/putil.h"
cbd2c77 
* we use u_setDataDirectory() in "unicode/putil.h"
* at present, this header is indirectly included,
  but this will change in ICU 59
* no impact on past ICUs.
* this is an exact analog to #11753

PR-URL: #12078
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
@srl295 @MylesBorins
tools: add missing #include "unicode/putil.h"
74d5cba 
* we use u_setDataDirectory() in "unicode/putil.h"
* at present, this header is indirectly included,
  but this will change in ICU 59
* no impact on past ICUs.
* this is an exact analog to #11753

PR-URL: #12078
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@srl295 srl295

Labels
i18n-api Issues and PRs related to the i18n implementation.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@MylesBorins @srl295 @jungshik