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

intl: VS2015: workaround ICU VS2015 failure? #2279

Closed
srl295 opened this issue Jul 30, 2015 · 8 comments
Closed

intl: VS2015: workaround ICU VS2015 failure? #2279

srl295 opened this issue Jul 30, 2015 · 8 comments
Labels
i18n-api Issues and PRs related to the i18n implementation. windows Issues and PRs related to the Windows platform.

Comments

@srl295
Copy link
Member

srl295 commented Jul 30, 2015

ICU (which provides Intl) failed under VS2015 in the same way and for the same reason as openssl in #478. Should we try to workaround?

I already committed a fix to IcuBug:11689, scheduled for release in a couple of months with ICU 56. If we hit this issue earlier, it looks like we could just pull a copy of ufile.c from ICU trunk on top of the downloaded ICU.

Just filing this for reference.

@rvagg @orangemocha
@rvagg
Copy link
Member

rvagg commented Jul 31, 2015

argh, 2015 is live now, it'd be a shame to have this as the only blocker.

is it possible to say something like "to build with VS2015 you must disable icu completely" or is it broken no matter how we look at it?

@brendanashworth brendanashworth added the windows Issues and PRs related to the Windows platform. label Jul 31, 2015
@srl295
Copy link
Member Author

srl295 commented Jul 31, 2015

to build with VS2015 you must disable icu completely

This is true at the moment. Sorry, I missed the scope of this issue before.

@orangemocha
Copy link
Contributor

Working on a repro. Is the icu lib itself that fails to build or does it fail at link time? We could change the download process to overwrite ufile.c from with a version from trunk (but at that point it would be better to commit it in our repo somewhere), or we could hack-define those missing functions somewhere else either in node or with an additional file for the icu library.

@orangemocha
Copy link
Contributor

Getting this error:

..\..\deps\icu\source\io\ufile.c(70): error C2109: subscript requires array or
pointer type

..which is at compile time, so my idea of defining the missing functions somewhere else won't help.

We should probably hack the build to use a different version of ufile.c then.

@srl295
Copy link
Member Author

srl295 commented Jul 31, 2015

@orangemocha yes, that's the error. The particular function gets treated as "undefined assuming function returning int", and then somehow has a problem subscripting an int.

I/we/you can snag the ufile.c from basically here and overwrite it in the situation of variables.icu_ver_major < 56 and optionally && MSVC 2015 or just MSVC. I think this ufile.c will work for ICU 54 and 55. And it's already checked in to the 56 stream.

@srl295
Copy link
Member Author

srl295 commented Jul 31, 2015

@orangemocha thanks for the discussion. I will open a PR here for a workaround.

@srl295 srl295 removed the tsc-agenda label Jul 31, 2015
@srl295
Copy link
Member Author

srl295 commented Jul 31, 2015

oops, was looking for the Intl label!

This was referenced Jul 31, 2015
Closed
Closed
@silverwind silverwind added the i18n-api Issues and PRs related to the i18n implementation. label Aug 10, 2015
@srl295 srl295 mentioned this issue Aug 13, 2015
Merged
srl295 added a commit that referenced this issue Aug 14, 2015
@srl295 @joaocgreis
build: work around VS2015 issue in ICU <56
4c06515 
The particular ufile.c is from
http://bugs.icu-project.org/trac/changeset/37704
and should be OK for ICU 54 and 55.

Also, adds general mechanism for floating patches on top of ICU.

Fixes: #2279
PR-URL: #2283
Reviewed-By: João Reis <[email protected]>
@joaocgreis
Copy link
Member

Fixed by 4c06515.

srl295 added a commit that referenced this issue Aug 17, 2015
@srl295 @rvagg
build: work around VS2015 issue in ICU <56
3645dc6 
The particular ufile.c is from
http://bugs.icu-project.org/trac/changeset/37704
and should be OK for ICU 54 and 55.

Also, adds general mechanism for floating patches on top of ICU.

Fixes: #2279
PR-URL: #2283
Reviewed-By: João Reis <[email protected]>
srl295 added a commit to nodejs/node-v0.x-archive that referenced this issue Aug 24, 2015
@srl295 @joaocgreis
build: work around VS2015 issue in ICU <56
4f24563 
This change is a backport of
nodejs/node@4c06515.

Original commit message:

  The particular ufile.c is from
  http://bugs.icu-project.org/trac/changeset/37704
  and should be OK for ICU 54 and 55.

  Also, adds general mechanism for floating patches on top of ICU.

  Fixes: nodejs/node#2279
  PR-URL: nodejs/node#2283
  Reviewed-By: João Reis <[email protected]>

Fixes: #25792

PR-URL: #25804
Reviewed-By: João Reis <[email protected]>
jBarz pushed a commit to ibmruntimes/node that referenced this issue Nov 4, 2016
@srl295 @jBarz
build: work around VS2015 issue in ICU <56
476fce2 
This change is a backport of
nodejs/node@4c06515.

Original commit message:

  The particular ufile.c is from
  http://bugs.icu-project.org/trac/changeset/37704
  and should be OK for ICU 54 and 55.

  Also, adds general mechanism for floating patches on top of ICU.

  Fixes: nodejs/node#2279
  PR-URL: nodejs/node#2283
  Reviewed-By: João Reis <[email protected]>

Fixes: nodejs#25792

PR-URL: nodejs#25804
Reviewed-By: João Reis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
i18n-api Issues and PRs related to the i18n implementation. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@silverwind @joaocgreis @rvagg @srl295 @orangemocha @brendanashworth