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

Refloat cherry-picks from #18196 #18360

Closed
wants to merge 3 commits into from
Closed

Refloat cherry-picks from #18196 #18360

wants to merge 3 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jan 24, 2018 •
edited
Loading

#18196 landed shortly before #17489. The latter ends up dropping the V8 cherry-picks contained in the former. This PR cherry-picks them back.

/cc @MylesBorins @apapirovski @targos

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

@ofrobots
deps: cherry-pick 814577e from upstream V8
e5bbf90 
Original commit message:

    [tracing] allow embedders to provide own tracing timestamps

    Make it possible for embedders to provide their own tracing timetamps by
    providing an overridable virtual function on V8's tracing controller.

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I727e633cb7f63d4b41c2e427ecca3c9174c90bfe
    Reviewed-on: https://chromium-review.googlesource.com/847690
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Fadi Meawad <[email protected]>
    Commit-Queue: Ali Ijaz Sheikh <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#50489}

Refs: v8/v8@814577e
Refs: nodejs#17349
PR-URL: nodejs#18196
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
@ofrobots
deps: cherry-pick c3bb73f from upstream V8
ee5a37b 
Original commit message:

    [tracing] implement TRACE_EVENT_ADD_WITH_TIMESTAMP

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Icb3cf7b7f96704e1eaa4c5fbf773b94b70cddc85
    Reviewed-on: https://chromium-review.googlesource.com/861302
    Reviewed-by: Fadi Meawad <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Ali Ijaz Sheikh <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#50549}

Refs: v8/v8@c3bb73f
Refs: nodejs#17349
PR-URL: nodejs#18196
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Jan 24, 2018
@ofrobots ofrobots mentioned this pull request Jan 24, 2018
Closed
6 tasks
@MylesBorins
Copy link
Contributor

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

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 25, 2018
@kfarnung kfarnung mentioned this pull request Jan 25, 2018
Closed
@MylesBorins
Copy link
Contributor

Since this change is arguably changing ABI do we need to bump module version? or is it close enough to the 6.4 land that we can shrug it off?

@ofrobots
Copy link
Contributor Author

ABI changes on master are okay. ABI changes on release branches would have been problematic.

@ofrobots ofrobots added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 25, 2018
@MylesBorins
Copy link
Contributor

@ofrobots we do arguably bump the module version number on master every time though

@kfarnung
Copy link
Contributor

If I'm understanding the issue correctly it wasn't even possible to build node after the 6.4 changes landed. If that's the case then I don't think ABI compatibility is a problem here.

@ofrobots
Copy link
Contributor Author

ofrobots commented Jan 25, 2018 •
edited
Loading

@MylesBorins it makes sense to bump the MODULE_VERSION each time a new V8 is picked up; but I don't think that locks us into a precise V8 ABI during the lifetime of that version of V8 on master. I think the idea is to signal some level of compatibility of the ABI through the MODULE_VERSION (e.g. the bump indicates that it is not compatible with master+V8-6.3), but at the same time, we don't guarantee that ABI won't change until it is released.

@joyeecheung joyeecheung mentioned this pull request Jan 25, 2018
Closed
2 tasks
@MylesBorins
Copy link
Contributor

V8-CI is failing

v8tests.cctest/test-bytecode-generator/ClassFields --nohard-abort

Should we back out 6.4 and figure out the fix separately?

V8-CI on master for comparison

https://ci.nodejs.org/job/node-test-commit-v8-linux/1178/

@ofrobots
Copy link
Contributor Author

That test failure suggests that some V8 commit was landed with --whitespace=fix. Some of the V8 tests are sensitive to whitespace. Let me look at the commits in #17489.

@MylesBorins
Copy link
Contributor

OH NO! that was totally me when I landed the original PR. ugh I'm sorry. Since nothing else has landed on master should we force push?

@ofrobots
Copy link
Contributor Author

Yep, 4c4af64 contains the change to ClassFields.golden. @MylesBorins did you land that with --whitespace=fix by any chance?

@jasnell
Copy link
Member

jasnell commented Jan 25, 2018

Force push window is only about 10 minutes. Please don't. I've rebased things since this landed.

@jasnell
Copy link
Member

jasnell commented Jan 25, 2018

In other words, please go with a new corrective commit rather than a force push.

@MylesBorins
Copy link
Contributor

@jasnell I'll submit a new PR to fix this

@ofrobots
Copy link
Contributor Author

Personally, I don't think the V8-CI is absolutely critical to be green. I think we should fix the whitespace issue in ClassFields.golden in a follow up.

@MylesBorins
Copy link
Contributor

I'll make a patch and we can wrap it into this PR

@MylesBorins
Copy link
Contributor

This commit is the result of reverting the upgrade, applying the upgrade again, and squashing all 20 commits

MylesBorins@a082513

@ofrobots
deps: V8: reintroduce missing whitespace in test
20c1fce 
4c4af64 accidentally dropped the significant whitespace from this test
when it was landed. Add the whitespace back.

Refs: nodejs#17489
PR-URL: nodejs#18360
@ofrobots
Copy link
Contributor Author

Added to the PR. New V8-CI is at https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1179/, but I am not going to wait for it, it is more important to fix the Node CI. Can I get a quick LGTM on this last commit: 20c1fce.

@kfarnung
Copy link
Contributor

LGTM

@ofrobots
Copy link
Contributor Author

ofrobots commented Jan 25, 2018 •
edited
Loading

Thanks. Landed as 990959d...142d623.

@ofrobots ofrobots closed this Jan 25, 2018
ofrobots added a commit that referenced this pull request Jan 25, 2018
@ofrobots
deps: cherry-pick 814577e from upstream V8
a1c5ddd 
Original commit message:

    [tracing] allow embedders to provide own tracing timestamps

    Make it possible for embedders to provide their own tracing timetamps by
    providing an overridable virtual function on V8's tracing controller.

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I727e633cb7f63d4b41c2e427ecca3c9174c90bfe
    Reviewed-on: https://chromium-review.googlesource.com/847690
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Fadi Meawad <[email protected]>
    Commit-Queue: Ali Ijaz Sheikh <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#50489}

Refs: v8/v8@814577e
Refs: #17349
PR-URL: #18196
Refs: #18360
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
ofrobots added a commit that referenced this pull request Jan 25, 2018
@ofrobots
deps: cherry-pick c3bb73f from upstream V8
b064403 
Original commit message:

    [tracing] implement TRACE_EVENT_ADD_WITH_TIMESTAMP

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Icb3cf7b7f96704e1eaa4c5fbf773b94b70cddc85
    Reviewed-on: https://chromium-review.googlesource.com/861302
    Reviewed-by: Fadi Meawad <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Ali Ijaz Sheikh <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#50549}

Refs: v8/v8@c3bb73f
Refs: #17349
PR-URL: #18196
Refs: #18360
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
ofrobots added a commit that referenced this pull request Jan 25, 2018
@ofrobots
deps: V8: reintroduce missing whitespace in test
142d623 
4c4af64 accidentally dropped the significant whitespace from this test
when it was landed. Add the whitespace back.

Refs: #17489
PR-URL: #18360
Reviewed-By: Kyle Farnung <[email protected]>
@ofrobots ofrobots deleted the reland-18196 branch January 25, 2018 01:17
@ofrobots ofrobots mentioned this pull request Jan 25, 2018
Merged
ofrobots pushed a commit to ofrobots/node that referenced this pull request Jan 31, 2018
@MylesBorins @ofrobots
deps: V8: resolve remaining whitespace diff
70277d6 
Some whitespace was lost when nodejs#17489 landed. While I restored the one
file causing the V8-CI to fail, I missed the remaining changes from
Myles. This changes restores all whitespace differences with upstream.

PR-URL: nodejs#18366
Refs: nodejs#18360
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ofrobots @MayaLekova
deps: cherry-pick 814577e from upstream V8
242a7d5 
Original commit message:

    [tracing] allow embedders to provide own tracing timestamps

    Make it possible for embedders to provide their own tracing timetamps by
    providing an overridable virtual function on V8's tracing controller.

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: I727e633cb7f63d4b41c2e427ecca3c9174c90bfe
    Reviewed-on: https://chromium-review.googlesource.com/847690
    Reviewed-by: Yang Guo <[email protected]>
    Reviewed-by: Fadi Meawad <[email protected]>
    Commit-Queue: Ali Ijaz Sheikh <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#50489}

Refs: v8/v8@814577e
Refs: nodejs#17349
PR-URL: nodejs#18196
Refs: nodejs#18360
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ofrobots @MayaLekova
deps: cherry-pick c3bb73f from upstream V8
16bbf05 
Original commit message:

    [tracing] implement TRACE_EVENT_ADD_WITH_TIMESTAMP

    Bug:
    Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
    Change-Id: Icb3cf7b7f96704e1eaa4c5fbf773b94b70cddc85
    Reviewed-on: https://chromium-review.googlesource.com/861302
    Reviewed-by: Fadi Meawad <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Ali Ijaz Sheikh <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#50549}

Refs: v8/v8@c3bb73f
Refs: nodejs#17349
PR-URL: nodejs#18196
Refs: nodejs#18360
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ofrobots @MayaLekova
deps: V8: reintroduce missing whitespace in test
7e0ffad 
4c4af64 accidentally dropped the significant whitespace from this test
when it was landed. Add the whitespace back.

Refs: nodejs#17489
PR-URL: nodejs#18360
Reviewed-By: Kyle Farnung <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MylesBorins @MayaLekova
deps: V8: resolve remaining whitespace diff
6aa65db 
Some whitespace was lost when nodejs#17489 landed. While I restored the one
file causing the V8-CI to fail, I missed the remaining changes from
Myles. This changes restores all whitespace differences with upstream.

PR-URL: nodejs#18366
Refs: nodejs#18360
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfarnung kfarnung kfarnung approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. semver-major PRs that contain breaking changes and should be released in the next major version. 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
@ofrobots @MylesBorins @kfarnung @jasnell @apapirovski @addaleax @nodejs-github-bot