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

deps: update V8 to 7.8 #29493

Closed
wants to merge 15 commits into from
Closed

deps: update V8 to 7.8 #29493

wants to merge 15 commits into from

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Sep 8, 2019

ETA: Oct 22nd, 2019

/cc @targos @refack 🎉

ryzokuken and others added 14 commits September 8, 2019 21:55
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 7.8.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
PR-URL: nodejs#28016
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Original commit message:

    [testrunner] delete ancient junit compatible format support

    Testrunner has ancient support for JUnit compatible XML output.

    This CL removes this old feature.

    [email protected],[email protected],[email protected]
    CC=​[email protected]

    Bug: v8:8728
    Change-Id: I7e1beb011dbaec3aa1a27398a5c52abdd778eaf0
    Reviewed-on: https://chromium-review.googlesource.com/c/1430065
    Reviewed-by: Jakob Gruber <[email protected]>
    Reviewed-by: Michael Starzinger <[email protected]>
    Commit-Queue: Tamer Tas <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#59045}

Refs: v8/v8@bd019bd

PR-URL: nodejs#26685
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#26685
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Fixes a compilation issue on some platforms

PR-URL: nodejs#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This should be semver-patch since actual invocation is version
conditional.

PR-URL: nodejs#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
`std::atomic_init<size_t>` is not implemented on all platforms.

PR-URL: nodejs#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Refs: https://developercommunity.visualstudio.com/content/problem/512352/compiler-doesnt-finish-142027508.html

PR-URL: nodejs#28016
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
PR-URL: nodejs#27375
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#28016
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann (רפאל פלחי) <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
until c6196ad7a2d601a4e1fdb313bfe2ec727fd67f7a

Co-authored-by: Ujjwal Sharma <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]>
@ryzokuken ryzokuken self-assigned this Sep 8, 2019
@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 Sep 8, 2019
@targos
Copy link
Member

targos commented Sep 8, 2019

We need to find a better message for the last commit 😅

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen
Copy link
Member

on macOS: https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/28796/console

00:57:40 ../deps/v8/src/compiler/js-heap-broker.cc:4116:23: error: constructor for 'v8::internal::compiler::GlobalAccessFeedback' must explicitly initialize the const member 'cell_or_context_'
00:57:40 GlobalAccessFeedback::GlobalAccessFeedback(FeedbackSlotKind slot_kind)
00:57:40                       ^
00:57:40 ../deps/v8/src/compiler/processed-feedback.h:83:35: note: 'cell_or_context_' declared here
00:57:40   base::Optional<ObjectRef> const cell_or_context_;

@ryzokuken
Copy link
Contributor Author

@gengjiawen http://logs.libuv.org/node-build/2019-09-11#16:20:08.434

@rvagg
Copy link
Member

rvagg commented Sep 11, 2019

I think we might be a bit stuck on this one. In BUILDING.md we have Xcode 8 as our minimum. That machine has 8 on it ("version: 8.2.0.0.1.1480973914"). If this isn't going to compile there then I think there are two obvious choices (maybe there's non-obvious ones?):

  1. Mark this semver-major and change our BUILDING.md support matrix to match the new minimum required by this. Will need @nodejs/build buy-in but I don't imagine it'll be too controversial.
  2. Work on fixing the code to compile with the current compiler. It might not be too hard to do, there may even be just that one place with a problem. Getting a local config with Xcode 8 might be the trickier bit but maybe we can give you access to a build machine to do it on? Open an issue in nodejs/build requesting ssh access to a specific type of machine and we should be able to organise that if you want to try it.

@bnoordhuis
Copy link
Member

Is it even reasonable to support xcode 8? If you submit a bug report to Apple, it's not like they're going to fix it, they'll just tell you to upgrade to xcode 10.

(In reality, they're not going to tell you anything because Apple but you know what I mean.)

@gengjiawen
Copy link
Member

Is it even reasonable to support xcode 8? If you submit a bug report to Apple, it's not like they're going to fix it, they'll just tell you to upgrade to xcode 10.

(In reality, they're not going to tell you anything because Apple but you know what I mean.)

Let's set Xcode requirement to 10 ?

@ryzokuken
Copy link
Contributor Author

@bnoordhuis

Is it even reasonable to support xcode 8? If you submit a bug report to Apple, it's not like they're going to fix it, they'll just tell you to upgrade to xcode 10.

Exactly.

(In reality, they're not going to tell you anything because Apple but you know what I mean.)

🤷‍♂ Didn't want to say it out loud, but...

@rvagg

Mark this semver-major and change our BUILDING.md support matrix to match the new minimum required by this. Will need @nodejs/build buy-in but I don't imagine it'll be too controversial.

I think I'd personally prefer this, since me and @targos originally decided to do this also, and IIRC, V8 7.8 isn't going to land on older release lines anyway.

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 12, 2019
@mmarchini
Copy link
Contributor

From our BUILDING.md:

Node.js does not support a platform version if a vendor has expired support for it. In other words, Node.js does not support running on End-of-Life (EoL) platforms. This is true regardless of entries in the table below.

Is xcode still supported by Apple? If it's not, according to our BUILDING.md we don't officially support it either.

(If Apple doesn't support xcode 8 anymore but we do, we should change that paragraph as well as the table)

@mhdawson
Copy link
Member

Mark this semver-major and change our BUILDING.md support matrix to match the new minimum required by this. Will need @nodejs/build buy-in but I don't imagine it'll be too controversial.

+1 from me but the additional work/thing to figure out is how we continue to test with the older xcode for earlier releases (which I assume we need to given that we would be saying this is SemVer major). It might be time to also update the version we build on (ie start building on 10.12 with an updated xcode version and leave the existing machines as is).

Given that we are 10 days from the SemVer cutoff for 13.x we might have to wait until 14.x (ie do in master after 13.x is cut) as it could take some time to agree/get all of the required pieces in place.

@sam-github
Copy link
Contributor

sam-github commented Sep 17, 2019

As I understand the current situation, unless someone steps up to do the OS X infrastructure work, it looks like V8 7.8 may not make it into 13.x? If so, that should be called out in the TSC agenda, @nodejs/tsc EDIT: remove the label if I misunderstood!

@Trott
Copy link
Member

Trott commented Sep 17, 2019

As I understand the current situation, unless someone steps up to do the OS X infrastructure work, it looks like V8 7.8 may not make it into 13.x? If so, that should be called out in the TSC agenda, @nodejs/tsc EDIT: remove the label if I misunderstood!

@sam-github Do you mean merely as an informational item? Or is there an action/decision the TSC needs to take here?

@sam-github
Copy link
Contributor

@Trott I think (just my thoughts), that if a deadline is likely to be missed, the TSC should be aware of it. If no one is concerned enough, or able, to do anything about it, that's OK, just so long as its not a surprise.

@mhdawson
Copy link
Member

We discussed in the TSC meeting:

  • Myles will follow up with V8 team to see if there is any chance to get a patch so that we can still
    land in 13.x before the SemVer major cutoff

There was also the suggestion that we update the minimum for 13.x even though we won't be able to test/build with the newer xcode until later.

@nodejs/build I think updating the minimum now makes sense, it gives us greater flexibility in 13.x and is what we'd want to do, we just can actually do it in time to build/test with it. Please let me know if you agree or not. Otherwise I'll submit a PR to make that change, I'm assuming we'd want to update it to xCode 10 ?

@rvagg
Copy link
Member

rvagg commented Sep 18, 2019

@mhdawsom 👍 let's make the change to 10

@mhdawson
Copy link
Member

PR to bump minimum xCode version: #29622

@gengjiawen
Copy link
Member

So is it possible to set build toolchain upgrade as semver-minor. In the end, it won't affect Node.js user, only core devs. Consider V8 7.8 will go to 12.x too ?

@rvagg
Copy link
Member

rvagg commented Sep 20, 2019

In the end, it won't affect Node.js user

A large number of Node.js users compile their own binaries, a large number of intermediates (like package managers) do too. Those are our users for the purpose of semer-major changes to toolchain. Some very large companies have toolchains that take source and compile down to their own binaries using their own rules and if we start breaking toolchain requirements mid-major then we break their pipelines.

mhdawson added a commit to mhdawson/io.js that referenced this pull request Sep 20, 2019
Update the minimum Xcode version for macOS to 10. We'll
need this level to incorporate future V8 versions.

Refs: nodejs#29493
mhdawson added a commit that referenced this pull request Sep 20, 2019
Update the minimum Xcode version for macOS to 10. We'll
need this level to incorporate future V8 versions.

Refs: #29493

PR-URL: #29622
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 24, 2019
4 tasks
@MylesBorins
Copy link
Contributor

Hey all, I've got an alternative PR that is building on OSX 🎉

@ryzokuken would you be open to closing this in lieu of #29694?

@mhdawson
Copy link
Member

@sam-github do you think we can remove the tsc-agenda from this one?

@ryzokuken
Copy link
Contributor Author

@MylesBorins would absolutely love to, thanks so much, and sorry I couldn't give enough love to this.

@ryzokuken ryzokuken closed this Sep 25, 2019
@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 25, 2019
rvagg added a commit to rvagg/io.js that referenced this pull request Oct 20, 2019
This reverts commit 9f830f3.

Ref: nodejs#29622
Ref: nodejs#29493

Node.js build infrastructure is not prepared to release 13.x on anything
but Xcode 8 on macOS 10.11.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. 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
Development

Successfully merging this pull request may close these issues.