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.

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

deps: revert whitespace changes on V8 #32587

Closed
wants to merge 1 commit into from
Closed

deps: revert whitespace changes on V8 #32587

wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

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
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
Contributor Author

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
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
Contributor Author

@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
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
Contributor

I think we should fastrack this

@MylesBorins MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 31, 2020
@targos
Copy link
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
Contributor Author

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 <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@mmarchini
Copy link
Contributor Author

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 <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@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

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mmarchini @nodejs-github-bot @MylesBorins @targos @jasnell @addaleax @cjihrig