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

build: fix VS2015 ICU/Intl breakage <56 (v0.12) #25804

Closed
wants to merge 1 commit into from

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Aug 3, 2015

rebased to v0.12

probably fixes #25792

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

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

backport from: nodejs/node#2283

  • includes responses to comments in from io.js version
  • also apply patch to ICU 55 (if/when that is merged)

@srl295
Copy link
Member Author

srl295 commented Aug 3, 2015

@orangemocha

@srl295 srl295 changed the title build: work around VS2015 issue in ICU <56 build: work around VS2015 issue in ICU <56 (v0.12) Aug 3, 2015
@srl295 srl295 changed the title build: work around VS2015 issue in ICU <56 (v0.12) build: fix VS2015 ICU/Intl breakage <56 (v0.12) Aug 3, 2015
probably fixes nodejs#25792

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

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

backport from: nodejs/node#2283

* includes responses to comments in from io.js version
* also apply patch to ICU 55 (if/when that is merged)
@srl295 srl295 force-pushed the vs2015-icufix-v012 branch from 74d9849 to 3e46d6d Compare August 4, 2015 16:24
@srl295
Copy link
Member Author

srl295 commented Aug 4, 2015

As of 3e46d6d this is parallel to the io.js side

@srl295 srl295 self-assigned this Aug 13, 2015
@srl295
Copy link
Member Author

srl295 commented Aug 13, 2015

@joaocgreis do you want to take this as part of your work?

@joaocgreis
Copy link
Member

@srl295 I've just submitted this: #25857 , enabling VS2015 in v0.10. When it lands, the plan is to merge v0.10 into v0.12. It would be nice to have this already landed, and VS2015 support would be complete. So, LGTM!

@jasnell jasnell added the v0.12 label Aug 16, 2015
@joaocgreis
Copy link
Member

@srl295 I think it would be good to reorganize the commit message, and then I can take care of landing this. There is no official guide for commit messages for ports that I know of, so I've been adapting the one specific for v8. I would amend the commit message to something like:

build: work around VS2015 issue in ICU <56

This change is a backport of
nodejs/node@4c06515a2f13c9b0890b374bb3ab3c0740c282e2.

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

Feel free to change/add anything in my suggestion. If you're ok with it as it is, I can also take care of this and land, let me know.

@srl295
Copy link
Member Author

srl295 commented Aug 24, 2015

@joaocgreis the message looks good to me.

srl295 added a commit that referenced this pull request Aug 24, 2015
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]>
@joaocgreis
Copy link
Member

@joaocgreis joaocgreis closed this Aug 24, 2015
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
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 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.

4 participants