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

v8: update v8 patch version number (v7.x) #12542

Closed

Conversation

bnoordhuis
Copy link
Member

We (including yours truly) have been less than stellar with bumping
the patch level for every change to deps/v8. This commit rectifies
that and covers the following commits:

2bbee49 v8: fix build errors with g++ 7
f882f47 deps: cherry-pick 79aee39 from upstream v8
9f73df5 deps: cherry-pick 22858cb from V8 upstream
a735c16 deps: backport ec1ffe3 from upstream V8
6130d54 deps: backport 8dde6ac from upstream V8

This is a chery-pick if you consider reducing the context to -C2
a cherry-pick; WordIsSmi has been renamed to TaggedIsSmi upstream.

Original commit message:

    [builtins] Fix pointer comparison in ToString builtin.

    This fixes the bogus {Word32Equal} comparison in the ToString
    builtin implementing Object.prototype.toString to be a pointer-size
    {WordEqual} comparison instead. Comparing just the lower half-word
    is insufficient on 64-bit architectures.

    [email protected]
    TEST=mjsunit/regress/regress-crbug-664506
    BUG=chromium:664506

    Review-Url: https://codereview.chromium.org/2496043003
    Cr-Commit-Position: refs/heads/master@{nodejs#40963}

Fixes: nodejs#12411
PR-URL: nodejs#12412
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h.  We cannot easily back-port
those changes due to their size and invasiveness.

Fixes: nodejs#10388
PR-URL: nodejs#12392
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
We (including yours truly) have been less than stellar with bumping
the patch level for every change to deps/v8.  This commit rectifies
that and covers the following commits:

2bbee49 v8: fix build errors with g++ 7
f882f47 deps: cherry-pick 79aee39 from upstream v8
9f73df5 deps: cherry-pick 22858cb from V8 upstream
a735c16 deps: backport ec1ffe3 from upstream V8
6130d54 deps: backport 8dde6ac from upstream V8
@nodejs-github-bot nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 20, 2017
@jasnell
Copy link
Member

jasnell commented Apr 20, 2017

I'm wondering if there's a way we can automate this somehow... such as having the github bot check for the patch level bump if the commit touches deps/v8 at all... or having a pre-push git hook that checks for it.

@bnoordhuis
Copy link
Member Author

I've been thinking about that. A PR-time check would be annoying because version numbers tend to be conflict magnets. A push-time check would be workable most of the time but not when doing housekeeping; i.e., there should be an opt-out.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@bnoordhuis
Copy link
Member Author

I'm wondering if there's a way we can automate this somehow...

To be clear I wasn't disagreeing it's a good idea. Case in point: I forgot to bump the patch level when I merged #12460...

@evanlucas evanlucas force-pushed the v7.x-staging branch 2 times, most recently from d4ceb59 to 69a8053 Compare May 3, 2017 12:56
@bnoordhuis
Copy link
Member Author

I'll close this, it's out of date. On the topic of automation, maybe we should do the patch level bump at release time (and only when patches were landed, of course.) Good idea, bad idea?

@bnoordhuis bnoordhuis closed this May 15, 2017
@gibfahn
Copy link
Member

gibfahn commented May 19, 2017

On the topic of automation, maybe we should do the patch level bump at release time (and only when patches were landed, of course.) Good idea, bad idea?

If it's still a manual process I suspect it'll still get forgotten during the release.

A PR-time check would be annoying because version numbers tend to be conflict magnets.

Maybe a lint rule? That way you could ignore it if you knew it wasn't correct (and you could rerun just before landing the commit to make sure it was okay).

@bnoordhuis
Copy link
Member Author

Releasers follow a script, don't they? That said, a lint rule seems like a good idea.

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.

6 participants