V8 update script broken

[gengjiawen]

https://ci.nodejs.org/view/Node.js%20Daily/job/node-update-v8-canary/1435/console

13:55:51 + git-node v8 major --branch=lkgr --base-dir=/home/iojs/build/workspace/node-update-v8-canary --node-dir=/home/iojs/build/workspace/node-update-v8-canary/node --no-version-bump
13:55:52 [05:55:52] Update local V8 clone [started]
13:55:52 [05:55:52] Fetch V8 [started]
13:56:03 [05:56:03] Fetch V8 [completed]
13:56:03 [05:56:03] Update local V8 clone [completed]
13:56:03 [05:56:03] Major V8 update [started]
13:56:03 [05:56:03] Get current V8 version [started]
13:56:03 [05:56:03] Get current V8 version [completed]
13:56:03 [05:56:03] Checkout V8 branch [started]
13:56:06 [05:56:06] Checkout V8 branch [completed]
13:56:06 [05:56:06] Remove deps/v8 [started]
13:56:07 [05:56:06] Remove deps/v8 [completed]
13:56:07 [05:56:06] Clone branch to deps/v8 [started]
13:56:10 [05:56:10] Clone branch to deps/v8 [completed]
13:56:10 [05:56:10] Remove deps/v8/.git [started]
13:56:10 [05:56:10] Remove deps/v8/.git [completed]
13:56:10 [05:56:10] Update V8 DEPS [started]
13:56:10 [05:56:10] Update V8 DEPS [failed]
13:56:10 [05:56:10] → Cannot read property 'split' of undefined
13:56:10 [05:56:10] Major V8 update [failed]
13:56:10 [05:56:10] → Cannot read property 'split' of undefined
13:56:10 ✖ Cannot read property 'split' of undefined

cc @targos @ryzokuken

[gengjiawen]

Also cc @nodejs/node-core-utils An edge case in node-core-utils ?

This jenkins job runs latest node-core-utils from github master branch, start broken on July 24.

[mmarchini]

Did V8 add a new dependency or remove an old dependency? That could break things I think.

[mmarchini]

On a second thought (and after looking at V8 git history for today and yesterday), that's probably not the issue.

[codebytere]

hmm 🤔 not much has changed in that section of the codebase lately that i'm aware of but i can go have a poke

[gengjiawen]

On a second thought (and after looking at V8 git history for today and yesterday), that's probably not the issue.

It starts broken on July 24. I checked the node-core-utils v8 part, looks not touched recently.
More likely from V8. but I don't know 13:56:10 [05:56:10] → Cannot read property 'split' of undefined comes from since it lacks stacktrace.

[mmarchini]

Probably something changed on V8 which broke spectations we have on ncu.

[codebytere]

If i had to guess it's https://github.com/nodejs/node-core-utils/blob/88d9c618c3ccf60aed8c6832eb6a4b47eb5dd9e6/lib/update-v8/majorUpdate.js#L133 but i'll test locally

Edit: confirmed - it dies on that line

[codebytere]

Got it: nodejs/node-core-utils#465

[targos]

I worked on a fix two days ago but didn't have time to test it and open a PR yet:targos/node-core-utils@15bc3b0

[codebytere]

I'm happy to close mine out in favor of yours, or we can merge mine in the short term and then update to yours once we've verified things are shipshape? Up to yall :)

[codebytere]

@ryzokuken implicit there would be addressing small review feedback to ensure that's the case :)

[ryzokuken]

Oops, I just deleted my comment because it was obvious. Thanks.

[mmarchini]

@ryzokuken the changed code is only used on major bumps, so I think it would be safe to land @codebytere PR and make a release (to unbreak the daily v8 updated), and then follow up integrating @targos changes to allow for relative paths.

[ryzokuken]

Right. I feel it's highly unlikely that we would ever bump up to a version without those files, right?

[mmarchini]

There's the ongoing V8 8.5 bump (nodejs/node#34337) which might also be backported to Node.js v14. Either way, for the ongoing PR it's still possible to use a previous ncu version, and we should get a patch with a long-term fix landed before we start to backport to 8.5 v14.

[codebytere]

Should i ensure mine is backwards compatible then or is it fine as-is as a stopgap do we think? happy to try and make it a little cleaner if needed

[gengjiawen]

Should i ensure mine is backwards compatible then or is it fine as-is as a stopgap do we think? happy to try and make it a little cleaner if needed

If you can make it in a day I thinks it's okay. But longer may cause some issue, since node.js daily build broken more than 10 days. We got more time on V8 8.5 bump.

[mmarchini]

Let's keep this open until the long-term fix is applied :)

[vsemozhetbyt]

Is it normal that v15.0.0-v8-canary20200723fa696aa4d8 had "v8":"8.6.228.0" while the last v15.0.0-v8-canary202008071df3ac3d89 has "v8":"8.6.0.0" as per https://nodejs.org/download/v8-canary/index.json?

[targos]

Is it normal that v15.0.0-v8-canary20200723fa696aa4d8 had "v8":"8.6.228.0" while the last v15.0.0-v8-canary202008071df3ac3d89 has "v8":"8.6.0.0" as per https://nodejs.org/download/v8-canary/index.json?

That happens sometimes, when the HEAD commit of V8's lkgr branch wasn't tagged to a new version number yet.