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 373f4ae739ee #37505

Closed
wants to merge 1 commit into from
Closed

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Feb 24, 2021

Original commit message:

[torque] Don't replace unmodified empty files

To improve incremental builds.

Bug: v8:7793
Change-Id: I6990a97e058d22d34acd1f609167cd30ca7518ad
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2596789
Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#72053}

Refs: v8/v8@373f4ae

Fixes: #37368
Refs: #36139 (comment)

Some of the .inc files written by torque are empty, i.e. they are
zero length. Actions in gyp generated makefiles are always run, and
torque would always write out zero length files which meant their
timestamps were newer than any previously compiled object file.

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Feb 24, 2021
@richardlau
Copy link
Member Author

This PR also addresses the coverage build issue (#37368):
https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/804/nodes=benchmark/console

17:02:24 ++ grep -B1 Lines coverage/index.html
17:02:24 ++ head -n1
17:02:24 ++ grep -o '[0-9\.]*'
17:02:24 + JSCOVERAGE=96.76
17:02:24 ++ grep -A3 Lines coverage/cxxcoverage.html
17:02:25 ++ grep style
17:02:25 ++ grep -o '[0-9]\{1,3\}\.[0-9]\{1,2\}'
17:02:25 + CXXCOVERAGE=89.2

@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

This seems to indeed fix the issue, tests are completing in in a reasonable time on GH Actions 👍

This has already landed upstream (v8/v8@373f4ae), we need to cherry-pick it or simply wait for the V8 8.9 update.

@richardlau
Copy link
Member Author

This seems to indeed fix the issue, tests are completing in in a reasonable time on GH Actions 👍

This has already landed in upstream (v8/v8@373f4ae), we just need to cherry-pick it or simply wait for the V8 8.9 update.

Let's do that (cherry-pick). I'll update this PR.

@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

Note that 8.9 is ready to land on master: #37330 (comment)

@richardlau
Copy link
Member Author

I'll backport anyway (gives me an opportunity to try out git node v8 backport ...). We can always close if made redundant by V8 8.9 landing.

@richardlau
Copy link
Member Author

I just checked and it doesn't seem like v8/v8@373f4ae or equivalent is in the current #37330.

@aduh95
Copy link
Contributor

aduh95 commented Feb 24, 2021

Oh you're right, it has landed in V8 8.9.258, and #37330 is using 8.9.255.19. Let's backport indeed.

@richardlau richardlau changed the title [WIP] deps: V8: stop torque updating unchanged empty files deps: V8: cherry-pick 373f4ae739ee Feb 24, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36342/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3801/ (✅)
Coverage CI: https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/805/nodes=benchmark/console

19:06:14 ++ grep -B1 Lines coverage/index.html
19:06:14 ++ head -n1
19:06:14 ++ grep -o '[0-9\.]*'
19:06:14 + JSCOVERAGE=96.76
19:06:14 ++ grep -A3 Lines coverage/cxxcoverage.html
19:06:14 ++ grep style
19:06:14 ++ grep -o '[0-9]\{1,3\}\.[0-9]\{1,2\}'
19:06:14 + CXXCOVERAGE=89.2

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 24, 2021
@nodejs-github-bot

This comment has been minimized.

Original commit message:

    [torque] Don't replace unmodified empty files

    To improve incremental builds.

    Bug: v8:7793
    Change-Id: I6990a97e058d22d34acd1f609167cd30ca7518ad
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2596789
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
    Cr-Commit-Position: refs/heads/master@{#72053}

Refs: v8/v8@373f4ae
@richardlau
Copy link
Member Author

richardlau commented Feb 24, 2021

Rebased to fixed common.gypi merge conflict after the V8 8.9 update.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36346/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3802/ (✅)
Coverage CI: https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/806/nodes=benchmark/console

00:09:18 ++ grep -B1 Lines coverage/index.html
00:09:18 ++ head -n1
00:09:18 ++ grep -o '[0-9\.]*'
00:09:18 + JSCOVERAGE=96.76
00:09:18 ++ grep -A3 Lines coverage/cxxcoverage.html
00:09:18 ++ grep style
00:09:18 ++ grep -o '[0-9]\{1,3\}\.[0-9]\{1,2\}'
00:09:18 + CXXCOVERAGE=89.2

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 25, 2021

@targos targos added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 25, 2021
@targos
Copy link
Member

targos commented Feb 25, 2021

fast-track?

@aduh95
Copy link
Contributor

aduh95 commented Feb 25, 2021

Landed in 448de26

@aduh95 aduh95 closed this Feb 25, 2021
aduh95 pushed a commit that referenced this pull request Feb 25, 2021
Original commit message:

    [torque] Don't replace unmodified empty files

    To improve incremental builds.

    Bug: v8:7793
    Change-Id: I6990a97e058d22d34acd1f609167cd30ca7518ad
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2596789
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
    Cr-Commit-Position: refs/heads/master@{#72053}

Refs: v8/v8@373f4ae

PR-URL: #37505
Fixes: #37368
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@richardlau richardlau deleted the torque branch February 25, 2021 11:02
targos pushed a commit that referenced this pull request Feb 28, 2021
Original commit message:

    [torque] Don't replace unmodified empty files

    To improve incremental builds.

    Bug: v8:7793
    Change-Id: I6990a97e058d22d34acd1f609167cd30ca7518ad
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2596789
    Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
    Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
    Cr-Commit-Position: refs/heads/master@{#72053}

Refs: v8/v8@373f4ae

PR-URL: #37505
Fixes: #37368
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coverage stats have plummeted
7 participants