Skip to content

deps: revert whitespace changes on V8#32587

Closed
mmarchini wants to merge 1 commit intonodejs:masterfrom
mmarchini:revert-v8-whitespace-changes
Closed

deps: revert whitespace changes on V8#32587
mmarchini wants to merge 1 commit intonodejs:masterfrom
mmarchini:revert-v8-whitespace-changes

Conversation

@mmarchini
Copy link
Copy Markdown
Contributor

@mmarchini mmarchini commented Mar 31, 2020

While landing the upgrade to V8 8.1, something went wrong and git made
unecessary (and incorrect) whitespace changes to test fixtures, which
broke V8 tests. Revert those changes to fix our tests.

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

@mmarchini
deps: revert whitespace changes on V8
3b341a4 
While landing the upgrade to V8 8.1, something went wrong and git made
unecessary (and incorrect) whitespace changes to test fixtures, which
broke V8 tests. Revert those changes to fix our tests.
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Mar 31, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Mar 31, 2020 •
edited by mmarchini
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30289/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2982/

@mmarchini
Copy link
Copy Markdown
Contributor Author

mmarchini commented Mar 31, 2020

Not sure if we should bump v8_embedder_string here here, since it's reverting a git mistake and not making changes relatives to V8 tree. cc @nodejs/v8-update wdyt?

addaleax
addaleax approved these changes Mar 31, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

Did you use git node land? It’s really time to get that tool away from manually downloading patches…

@mmarchini
Copy link
Copy Markdown
Contributor Author

mmarchini commented Mar 31, 2020

@addaleax yes, I used git node land (but I might have messed up something before landing with git rebase). I didn't know git node land had problems with major V8 updates.

MylesBorins
MylesBorins approved these changes Mar 31, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@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

@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Mar 31, 2020

I think we should fastrack this

@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 72 hours to land. label Mar 31, 2020
@targos
Copy link
Copy Markdown
Member

targos commented Mar 31, 2020

Not sure if we should bump v8_embedder_string here here, since it's reverting a git mistake and not making changes relatives to V8 tree.

IMO we shouldn't

@mmarchini
Copy link
Copy Markdown
Contributor Author

mmarchini commented Mar 31, 2020

If anyone wants to land it feel free to do so. I won't be able to do it before later today or tomorrow morning.

mmarchini added a commit that referenced this pull request Apr 1, 2020
@mmarchini
deps: revert whitespace changes on V8
f6dcd63 
While landing the upgrade to V8 8.1, something went wrong and git made
unecessary (and incorrect) whitespace changes to test fixtures, which
broke V8 tests. Revert those changes to fix our tests.

PR-URL: #32587
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@mmarchini
Copy link
Copy Markdown
Contributor Author

mmarchini commented Apr 1, 2020

Landed on f6dcd63

(I didn't use git node land this time to avoid messing with white spaces again)

@mmarchini mmarchini closed this Apr 1, 2020
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
@mmarchini @BethGriggs
deps: revert whitespace changes on V8
77542a5 
While landing the upgrade to V8 8.1, something went wrong and git made
unecessary (and incorrect) whitespace changes to test fixtures, which
broke V8 tests. Revert those changes to fix our tests.

PR-URL: #32587
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@MylesBorins MylesBorins MylesBorins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-track PRs that do not need to wait for 72 hours to land. 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.

7 participants

@mmarchini @nodejs-github-bot @MylesBorins @targos @jasnell @addaleax @cjihrig