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: cherry-pick dfcdf7837e23 #36573

Closed
wants to merge 1 commit into from
Closed

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Dec 18, 2020

Coverage for nullish coalescing was broken in v8, and eating more characters than one would expect:

Screen Shot 2020-12-18 at 2 47 17 PM

This patch fixes it:

Screen Shot 2020-12-18 at 2 49 07 PM

Original commit message:

[coverage] fix greedy nullish coalescing

The SourceRangeScope helper was consuming too many characters, instead
explicitly create SourceRange, based on scanner position.

Bug: v8:11231
Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
Reviewed-by: Toon Verwaest <[email protected]>
Commit-Queue: Toon Verwaest <[email protected]>
Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

Related Issues

Fixes: #36619

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Dec 18, 2020
@bcoe bcoe requested a review from Trott December 18, 2020 22:50
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2020
@targos
Copy link
Member

targos commented Dec 19, 2020

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2020
@nodejs-github-bot
Copy link
Collaborator

@bcoe
Copy link
Contributor Author

bcoe commented Dec 20, 2020

@targos it looks like the v8 build is failing across the board?

@targos
Copy link
Member

targos commented Dec 21, 2020

@targos it looks like the v8 build is failing across the board?

It looks like a CI flake. Trying again: https://ci.nodejs.org/job/node-test-commit-v8-linux/3625/

Edit: again after cleaning the workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/3627/

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

@richardlau
Copy link
Member

@targos it looks like the v8 build is failing across the board?

It looks like a CI flake. Trying again: https://ci.nodejs.org/job/node-test-commit-v8-linux/3625/

Edit: again after cleaning the workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/3627/

Looking at https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/ I don't think this is a flake -- it looks like the build is broken. The timing of when the build broke appears to line up with 0e96dc1#diff-696862ddcbd7859bd9800af118c1a6e409c4f49f9c7c23d9025040f22c7f12c7 -- is it possible the script (tools/dev/v8gen.py) doesn't like empty arguments ("")?

@richardlau
Copy link
Member

@targos it looks like the v8 build is failing across the board?

It looks like a CI flake. Trying again: https://ci.nodejs.org/job/node-test-commit-v8-linux/3625/
Edit: again after cleaning the workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/3627/

Looking at https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1604-intel-64,v8test=v8test/ I don't think this is a flake -- it looks like the build is broken. The timing of when the build broke appears to line up with 0e96dc1#diff-696862ddcbd7859bd9800af118c1a6e409c4f49f9c7c23d9025040f22c7f12c7 -- is it possible the script (tools/dev/v8gen.py) doesn't like empty arguments ("")?

Looks so:

$ python2 tools/dev/v8gen.py "x64.release" --no-goma
$ python2 tools/dev/v8gen.py "x64.release" --no-goma ""
usage: v8gen.py [-h] {gen,list} ...
v8gen.py: error: unrecognized arguments:
$ 

@richardlau richardlau mentioned this pull request Dec 21, 2020
2 tasks
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 22, 2020

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 24, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36573
✔  Done loading data for nodejs/node/pull/36573
----------------------------------- PR info ------------------------------------
Title      deps: V8: cherry-pick dfcdf7837e23 (#36573)
Author     Benjamin E. Coe  (@bcoe)
Branch     bcoe:fix-nullish -> nodejs:master
Labels     V8 Engine, build
Commits    1
 - deps: V8: cherry-pick dfcdf7837e23
Committers 1
 - Benjamin Coe 
PR-URL: https://github.com/nodejs/node/pull/36573
Fixes: https://github.com/https://github.com/nodejs/node/issues/
Refs: https://github.com/v8/v8/commit/dfcdf7837e23cc0da31f9b2d4211f856413d13af
Reviewed-By: Michaël Zasso 
Reviewed-By: Rich Trott 
Reviewed-By: Michael Dawson 
Reviewed-By: Jiawen Geng 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36573
Fixes: https://github.com/https://github.com/nodejs/node/issues/
Refs: https://github.com/v8/v8/commit/dfcdf7837e23cc0da31f9b2d4211f856413d13af
Reviewed-By: Michaël Zasso 
Reviewed-By: Rich Trott 
Reviewed-By: Michael Dawson 
Reviewed-By: Jiawen Geng 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last V8 CI on 2020-12-22T03:13:19Z: https://ci.nodejs.org/job/node-test-commit-v8-linux/3630/
   ℹ  Last Full PR CI on 2020-12-22T06:04:26Z: https://ci.nodejs.org/job/node-test-pull-request/35035/
- Querying data for job/node-test-pull-request/35035/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Fri, 18 Dec 2020 22:49:34 GMT
   ✔  Approvals: 4
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/36573#pullrequestreview-555931323
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36573#pullrequestreview-556378221
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/36573#pullrequestreview-556517155
   ✔  - Jiawen Geng (@gengjiawen): https://github.com/nodejs/node/pull/36573#pullrequestreview-558419166
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 36573
From https://github.com/nodejs/node
 * branch                  refs/pull/36573/merge -> FETCH_HEAD
✔  Fetched commits as 2c8751cb855a..fba1e5f86bfa
--------------------------------------------------------------------------------
[master bd35b3b3b2] deps: V8: cherry-pick dfcdf7837e23
 Author: Benjamin Coe 
 Date: Fri Dec 18 10:40:07 2020 -0800
 3 files changed, 28 insertions(+), 7 deletions(-)
   ✔  Patches applied
--------------------------------------------------------------------------------
   ⚠  Found Refs: https://github.com/v8/v8/commit/dfcdf7837e23cc0da31f9b2d4211f856413d13af, skipping..
--------------------------------- New Message ----------------------------------
deps: V8: cherry-pick dfcdf7837e23

Original commit message:

[coverage] fix greedy nullish coalescing

The SourceRangeScope helper was consuming too many characters, instead
explicitly create SourceRange, based on scanner position.

Bug: v8:11231
Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
Reviewed-by: Toon Verwaest <[email protected]>
Commit-Queue: Toon Verwaest <[email protected]>
Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: #36573
Fixes: https://github.com/https://github.com/nodejs/node/issues/<issue_number>
Reviewed-By: Michaël Zasso [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: Jiawen Geng [email protected]

[master a2698e1924] deps: V8: cherry-pick dfcdf7837e23
Author: Benjamin Coe [email protected]
Date: Fri Dec 18 10:40:07 2020 -0800
3 files changed, 28 insertions(+), 7 deletions(-)
✖ a2698e192446ae6606095100adab24da799b3647
✖ 18:7 Fixes must be a GitHub URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 17:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/442076143

@gengjiawen
Copy link
Member

Related Issues
Fixes: https://github.com/nodejs/node/issues/<issue_number>

@bcoe looks you forget write the issue number, which lead to commit failure.

@bcoe bcoe added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 24, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2020
@github-actions
Copy link
Contributor

Landed in 3b0ecfc...33d99b6

@github-actions github-actions bot closed this Dec 24, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 24, 2020
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: #36573
Fixes: #36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jan 8, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: nodejs#36573
Fixes: nodejs#36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: #36573
Fixes: #36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos pushed a commit to targos/node that referenced this pull request Jan 25, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: nodejs#36573
Fixes: nodejs#36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Feb 7, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: nodejs#36573
Fixes: nodejs#36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Feb 11, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: nodejs#36573
Fixes: nodejs#36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Feb 11, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: nodejs#36573
Fixes: nodejs#36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit that referenced this pull request May 16, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: #36573
Fixes: #36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
Original commit message:

    [coverage] fix greedy nullish coalescing

    The SourceRangeScope helper was consuming too many characters, instead
    explicitly create SourceRange, based on scanner position.

    Bug: v8:11231
    Change-Id: I852d211227abacf867e8f1ab3e3ab06dbdba2a9b
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2576006
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#71765}

Refs: v8/v8@dfcdf78

PR-URL: #36573
Fixes: #36619
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[coverage]: nullish coalescing was consuming too many characters
7 participants