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

Revert "deps: V8: cherry-pick 687d865fe251" #31098

Closed
wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Dec 26, 2019

This reverts commit 0e21c1e.

It landed without a proper v8_embedder_string bump, reverting to re-land #31007 cleanly after that.

Refs: 0e21c1e
Refs: #31007

Possible alternative actions:

  1. revert and re-land -- this approach
  2. just bump the v8_embedder_string separately (PR: deps: V8: bump v8_embedder_string for 0e21c1e637bf #31096)
  3. do nothing at all (as @addaleax mentioned, v8_embedder_string has been just updated by another V8 patch).

Either of those seems fine to me, opening all the possible ones to speed up things.

This reverts commit 0e21c1e.

It landed without a proper v8_embedder_string bump, reverting
to re-land cleanly.

Refs: 0e21c1e
Refs: nodejs#31007
@ChALkeR ChALkeR added v8 engine Issues and PRs related to the V8 dependency. revert PRs that revert previously landed PRs. labels Dec 26, 2019
@ChALkeR ChALkeR requested review from addaleax and BridgeAR December 26, 2019 02:47
@MylesBorins
Copy link
Contributor

I personally prefer the approach in #31096 of just bumping the string... it is what I've personally done before in the past.

Won't block this landing though.

@addaleax
Copy link
Member

I’m closing this out because there seems to be consensus that either #31096 or doing nothing is preferred.

@addaleax addaleax closed this Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
revert PRs that revert previously landed PRs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants