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/v8: add missing #include "unicode/normlzr.h" #13040

Closed
wants to merge 1 commit into from
Closed

deps/v8: add missing #include "unicode/normlzr.h" #13040

wants to merge 1 commit into from

Conversation

ArchangeGabriel
Copy link

  • The following function from <unicode/normlzr.h> is used:
    normalize()

  • Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022

P.S.: This is my first PR in Node, and my first “real” PR in any project, so I don’t expect it to be alright from the start.

@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels May 15, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

@MylesBorins
Copy link
Contributor

hey @ArchangeGabriel thanks for submitting this PR

This patch most likely needs to be sent upstream to V8 first. We could then backport the V8 patch to each appropriate release stream. /cc @fhinkel who might be able to help with this (a patch on V8 and then a backport to Node!!!)

As a heads up, v4.x is in maintenance mode, it would not likely land for a while due to not being critical bug fix as v4 ships with icu: '56.1'. You can find out more about the LTS process here

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

-1 needs to be fixed in V8

edit:

this is not meaningful upstream to V8 so it makes sense to land it as a patch onto v6.x and backport to v4.x

@bnoordhuis
Copy link
Member

@MylesBorins This patch is not relevant upstream, they are switching (or have switched) to ICU 59.

@fhinkel
Copy link
Member

fhinkel commented May 16, 2017

LGTM. Yes, upstream V8 wouldn't be right. Thanks @ArchangeGabriel !

@MylesBorins
Copy link
Contributor

My apologies. I must have misunderstood something. Should this target master?

@bnoordhuis
Copy link
Member

I don't think so, master bundles ICU 59. This is just for v4.x and v6.x since they bundle ICU 58.

@ArchangeGabriel
Copy link
Author

master is not affected. This file is quite different in master, and already supports ICU 59. See #11753 for master and the v8 patches/links.

@ArchangeGabriel
Copy link
Author

@MylesBorins To be more clear maybe: upstream v8 is not 4.x or 6.x v8, you only do cherry-pick.

Upstream v8 doesn’t need patching, because the current version supports ICU 59 (since patches/links from my previous comment).

Upstream v8 patches cannot be backported, because node 4.x and 6.x v8 doesn’t need the same fixes as upstream v8 for ICU 59, since the code is quite different.

@MylesBorins
Copy link
Contributor

MylesBorins commented May 16, 2017 via email

@ArchangeGabriel
Copy link
Author

OK, I’ve targeted v4.x first because it’s the one I have to use, but the fix is the same for v6.x.

I suppose you should also wait for ARM and ppc-linux CI to finish before merging?

@MylesBorins
Copy link
Contributor

@ArchangeGabriel we generally waterfall from newer releases to older ones. Further v4.x is in maintenance, so a release won't be happening for quite a while for non critical bugs. If you are ok with it I'll update the issue to target v6.x and run CI again

@MylesBorins MylesBorins dismissed their stale review May 16, 2017 14:45

I was wrong

@MylesBorins
Copy link
Contributor

@nodejs/v8 how would we want to float a patch like this on V8 for v6.x? Should it bump patch number?

@targos
Copy link
Member

targos commented May 16, 2017

Should it bump patch number?

Yes

@ArchangeGabriel
Copy link
Author

@MylesBorins Yeah, sure. I fixed it on my side anyway, so this is mostly for other users/to be able to remove the patch from my build system. v6.x needs to be fixed too, I just didn’t open the v6.x PR in case this one would have required fixes.

@MylesBorins MylesBorins changed the base branch from v4.x-staging to v6.x-staging May 16, 2017 15:12
@MylesBorins
Copy link
Contributor

MylesBorins commented May 16, 2017

@ArchangeGabriel github was not able to smartly change target... I have a working branch (including V8 bump). Can you enable us to push to your branch so I can update?

@MylesBorins
Copy link
Contributor

MylesBorins commented May 16, 2017

You can find the option on the right side of the screen at the bottom
screen shot 2017-05-16 at 11 22 28 am

Specifically this

screen shot 2017-05-16 at 11 22 33 am

@ArchangeGabriel
Copy link
Author

This is already on.

@MylesBorins
Copy link
Contributor

odd...

@MylesBorins
Copy link
Contributor

Ahhh... I don't think we can force push.

here is the patch

MylesBorins@f00fe08

can you checkout v6.x-staging, cherry-pick, and force push over this branch. Sorry this is so convoluted

@ArchangeGabriel
Copy link
Author

I can redo my branch to be only the patch on v6.x, and then you could add your bump and any other change, if that’s easier for you.

@ArchangeGabriel
Copy link
Author

ArchangeGabriel commented May 16, 2017

Our messages crossed. I’m doing that right now (but could take some time since I don’t think I can do that from GitHub web, so I’ll have to clone the repo locally first).

@MylesBorins
Copy link
Contributor

@ArchangeGabriel whatever is easier for you. We only need to bump the patch number in deps/v8/include/v8-version.h. It should be part of the commit where the change is made

@ArchangeGabriel
Copy link
Author

Done, sorry for the delay.

* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
@MylesBorins MylesBorins added v6.x and removed v4.x labels May 18, 2017
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor

@nodejs/lts anyone else want to review this?

@gibfahn
Copy link
Member

gibfahn commented May 18, 2017

Maybe cc/ @srl295 ?

@MylesBorins MylesBorins self-assigned this May 18, 2017
@ArchangeGabriel
Copy link
Author

Small ping. I’d like to get this fixed in Argon and Boron so that I could package them without an out-of-tree patch. :)

@bnoordhuis
Copy link
Member

@ArchangeGabriel It has a couple of LGTMs, I expect it will land in the next v6.x patch release.

MylesBorins pushed a commit that referenced this pull request May 30, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins
Copy link
Contributor

Landed in d3ae2f0

I'll backport to v4.x-staging but can make no guarantees when, if at all, we will land it

MylesBorins pushed a commit that referenced this pull request May 30, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@ArchangeGabriel
Copy link
Author

Thanks! :) I’ll keep an eye on v4.x changes to see if that get merged, but I’ll just keep the patch in my tree else. ;)

MylesBorins pushed a commit that referenced this pull request Jun 6, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2017
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: nodejs/node#13022
PR-URL: nodejs/node#13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: #13022
PR-URL: #13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 25, 2017
MylesBorins added a commit that referenced this pull request Nov 6, 2017
Notable Changes:

* **crypto**:
  - update root certificates (Ben Noordhuis)
    #13279
  - update root certificates (Ben Noordhuis)
    #12402
* **deps**:
  - add support for more modern versions of INTL (Bruno Pagani)
    #13040
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade openssl sources to 1.0.2l (Daniel Bevenius)
    #13233

PR-URL: #16500
MylesBorins added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

* **crypto**:
  - update root certificates (Ben Noordhuis)
    #13279
  - update root certificates (Ben Noordhuis)
    #12402
* **deps**:
  - add support for more modern versions of INTL (Bruno Pagani)
    #13040
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade openssl sources to 1.0.2l (Daniel Bevenius)
    #13233

PR-URL: #16500
@ArchangeGabriel
Copy link
Author

@MylesBorins Thanks for having finally merged that patch in v4.x. :)

gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Nov 24, 2017
* The following function from <unicode/normlzr.h> is used:
     normalize()

* Until ICU 59, <unicode/normlzr.h> is indirectly included, but this changed with the 59 release. Adding this header has been the right thing to do for many years, so it is backwards compatible and fix compilation with recent ICU.

Refs: nodejs/node#13022
PR-URL: nodejs/node#13040
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants