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

[v12.x] deps: revert whitespace changes on V8 #32605

Closed
wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

While landing some of the V8 upgrades on the v12 branch, something went
wrong and git made unecessary (and incorrect) whitespace changes to test
fixtures, which broke V8 tests. This was likely caused by the use of
git node land or a simiar tool. Revert those changes to fix our tests.
Since this only reverts unwanted changes on deps/v8, and it only affects
tests, don't bump v8_embedder_string.

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

While landing some of the V8 upgrades on the v12 branch, something went
wrong and git made unecessary (and incorrect) whitespace changes to test
fixtures, which broke V8 tests. This was likely caused by the use of
`git node land` or a simiar tool. Revert those changes to fix our tests.
Since this only reverts unwanted changes on deps/v8, and it only affects
tests, don't bump v8_embedder_string.
@nodejs-github-bot nodejs-github-bot added v12.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 1, 2020
@mmarchini
Copy link
Contributor Author

@mmarchini
Copy link
Contributor Author

Extra care is needed here when landing to prevent git from stripping the trailing spaces.

@MylesBorins
Copy link
Member

MylesBorins commented Apr 1, 2020

@mmarchini if / when I land I'll use my custom git alias

patchit-please = "!f() { curl -L $1.patch | git am -3; }; f"

git patchit-please https://github.com/nodejs/node/pull/32605 will land on head with no changes to whitespace

Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@MylesBorins MylesBorins changed the title deps: revert whitespace changes on V8 [v12.x] deps: revert whitespace changes on V8 Apr 1, 2020
@mmarchini mmarchini linked an issue Apr 1, 2020 that may be closed by this pull request
@nodejs-github-bot
Copy link
Collaborator

MylesBorins pushed a commit that referenced this pull request Apr 2, 2020
While landing some of the V8 upgrades on the v12 branch, something went
wrong and git made unecessary (and incorrect) whitespace changes to test
fixtures, which broke V8 tests. This was likely caused by the use of
`git node land` or a simiar tool. Revert those changes to fix our tests.
Since this only reverts unwanted changes on deps/v8, and it only affects
tests, don't bump v8_embedder_string.

PR-URL: #32605
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
@MylesBorins
Copy link
Member

V8-CI is green and the various CI failures we are seeing have 0 to do with the whitespace fixes that are present in the PR. I'm going to go ahead and land them so we can unblock other V8 PRs

landed in 784df12

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.

V8 tests failing on v12.16.0+
4 participants