Skip to content

Revert V8 MSVC and deadlock workarounds #58187

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Revert V8 MSVC and deadlock workarounds #58187

wants to merge 3 commits into from

Conversation

targos
Copy link
Member

@targos targos commented May 6, 2025

Hopefully we don't need them anymore after #58047

@targos targos added v8 engine Issues and PRs related to the V8 dependency. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v23.x labels May 6, 2025
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels May 6, 2025
@bnoordhuis
Copy link
Member

Is the first commit (re-enable sparkplug) intended for v24? Asking for a friend.

@targos
Copy link
Member Author

targos commented May 6, 2025

I would say yes, but now that you ask, is it risky?

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66644/

@bnoordhuis
Copy link
Member

I would say yes, but now that you ask, is it risky?

After thinking about it for a bit: probably not; node's sparkplug problems were self-inflicted.

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6534/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6534/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6534/

@targos targos added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 7, 2025
@nodejs-github-bot nodejs-github-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 May 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58187
✔  Done loading data for nodejs/node/pull/58187
----------------------------------- PR info ------------------------------------
Title      Revert V8 MSVC and deadlock workarounds (#58187)
Author     Michaël Zasso <[email protected]> (@targos)
Branch     targos:revert-deadlock-v8 -> nodejs:main
Labels     build, v8 engine, author ready, needs-ci, dont-land-on-v20.x, dont-land-on-v22.x, dont-land-on-v23.x
Commits    3
 - Revert "deps: disable V8 concurrent sparkplug compilation"
 - Revert "deps: always define V8_EXPORT_PRIVATE as no-op"
 - Revert "deps: patch V8 to support compilation with MSVC"
Committers 1
 - Michaël Zasso <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 06 May 2025 08:03:38 GMT
   ✔  Approvals: 4
   ✔  - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/58187#pullrequestreview-2817293262
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/58187#pullrequestreview-2820026777
   ✔  - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/58187#pullrequestreview-2821675859
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/58187#pullrequestreview-2823009665
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-05-06T11:41:03Z: https://ci.nodejs.org/job/node-test-pull-request/66644/
- Querying data for job/node-test-pull-request/66644/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 58187
From https://github.com/nodejs/node
 * branch                  refs/pull/58187/merge -> FETCH_HEAD
✔  Fetched commits as abf1908bd05d..4c2902b2fb06
--------------------------------------------------------------------------------
Auto-merging common.gypi
[main a0eca360ac] Revert "deps: disable V8 concurrent sparkplug compilation"
 Author: Michaël Zasso <[email protected]>
 Date: Tue May 6 10:00:25 2025 +0200
 2 files changed, 2 insertions(+), 2 deletions(-)
Auto-merging common.gypi
[main f4aa6749d8] Revert "deps: always define V8_EXPORT_PRIVATE as no-op"
 Author: Michaël Zasso <[email protected]>
 Date: Tue May 6 10:00:52 2025 +0200
 2 files changed, 4 insertions(+), 4 deletions(-)
Auto-merging common.gypi
[main 34a5681c1c] Revert "deps: patch V8 to support compilation with MSVC"
 Author: Michaël Zasso <[email protected]>
 Date: Tue May 6 10:02:02 2025 +0200
 14 files changed, 26 insertions(+), 89 deletions(-)
 delete mode 100644 deps/v8/src/heap/base/asm/arm64/push_registers_masm.S
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Revert "deps: disable V8 concurrent sparkplug compilation"

This reverts commit 57699fffb8a387a3edeafd3418a369907b6b8246.


PR-URL: #58187

Reviewed-By: Ben Noordhuis <[email protected]>

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Luigi Pinca <[email protected]>


[detached HEAD bdf0f2de00] Revert "deps: disable V8 concurrent sparkplug compilation"

Author: Michaël Zasso <[email protected]>

Date: Tue May 6 10:00:25 2025 +0200

2 files changed, 2 insertions(+), 2 deletions(-)

Rebasing (3/6)

Rebasing (4/6)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Revert "deps: always define V8_EXPORT_PRIVATE as no-op"


This reverts commit ffadf3561a26dee89aa4601cd660f38636ba2cfb.


PR-URL: #58187

Reviewed-By: Ben Noordhuis <[email protected]>

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Luigi Pinca <[email protected]>


[detached HEAD 2317464000] Revert "deps: always define V8_EXPORT_PRIVATE as no-op"

Author: Michaël Zasso <[email protected]>

Date: Tue May 6 10:00:52 2025 +0200

2 files changed, 4 insertions(+), 4 deletions(-)

Rebasing (5/6)

Rebasing (6/6)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Revert "deps: patch V8 to support compilation with MSVC"


This reverts commit 0f98039268ee76cf0bf9d4337b096f952136937f.


PR-URL: #58187

Reviewed-By: Ben Noordhuis <[email protected]>

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Rafael Gonzaga <[email protected]>

Reviewed-By: Luigi Pinca <[email protected]>


[detached HEAD b8a333fff4] Revert "deps: patch V8 to support compilation with MSVC"

Author: Michaël Zasso <[email protected]>

Date: Tue May 6 10:02:02 2025 +0200

14 files changed, 26 insertions(+), 89 deletions(-)

delete mode 100644 deps/v8/src/heap/base/asm/arm64/push_registers_masm.S

Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/14901818290

@targos targos 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 May 8, 2025
@targos targos added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label May 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in abf1908...e5e8eaa

nodejs-github-bot pushed a commit that referenced this pull request May 8, 2025
@targos @nodejs-github-bot
Revert "deps: disable V8 concurrent sparkplug compilation"
32a8cc0 
This reverts commit 57699ff.

PR-URL: #58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request May 8, 2025
@targos @nodejs-github-bot
Revert "deps: always define V8_EXPORT_PRIVATE as no-op"
f2138b5 
This reverts commit ffadf35.

PR-URL: #58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request May 8, 2025
@targos @nodejs-github-bot
Revert "deps: patch V8 to support compilation with MSVC"
e5e8eaa 
This reverts commit 0f98039.

PR-URL: #58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos deleted the revert-deadlock-v8 branch May 8, 2025 08:29
targos added a commit that referenced this pull request May 16, 2025
@targos
Revert "deps: disable V8 concurrent sparkplug compilation"
e75ecf8 
This reverts commit 57699ff.

PR-URL: #58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos added a commit that referenced this pull request May 16, 2025
@targos
Revert "deps: always define V8_EXPORT_PRIVATE as no-op"
e3193ee 
This reverts commit ffadf35.

PR-URL: #58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos added a commit that referenced this pull request May 16, 2025
@targos
Revert "deps: patch V8 to support compilation with MSVC"
f54a7a4 
This reverts commit 0f98039.

PR-URL: #58187
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@github-actions github-actions bot mentioned this pull request May 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@targos @nodejs-github-bot @bnoordhuis @jasnell @lpinca @RafaelGSS