deps: cherry-pick a51f429 from V8 upstream

[fhinkel]

Checklist

• make -j4 test (UNIX)
• commit message follows commit guidelines

Affected core subsystem(s)

deps V8

Description of change

deps: cherry-pick a51f429 from V8 upstream that fixex case-insensitive regex problem

[thefourtheye]

cc @nodejs/v8

[targos]

Can you cherry-pick the full commit (including the test change) ? I think the version bump can be squashed with it too.

[fhinkel]

Added test and squashed. Sorry about forgetting the test.

[targos]

LGTM

[bnoordhuis]

LGTM. CI:

https://ci.nodejs.org/job/node-test-pull-request/3385/
https://ci.nodejs.org/job/node-test-commit-v8-linux/226/

[ofrobots]

LGTM.

I am going to somewhat disagree with the opinion on squashing version bumps into the commit now that we are doing the bumps. The negatives I see are:

• if the commit needs to be later reverted, we would have to be diligent to add an extra commit to keep the version number monotonically increasing. It would be easy to miss this step.
• PRs often stay open for a long while in which case intervening backports to land with that particular version level. It would be preferable if the bump commit was a separate commit and the primary cherry-pick doesn't need editing at landing time.

[bnoordhuis]

@ofrobots What would you do differently?

if the commit needs to be later reverted, we would have to be diligent to add an extra commit to keep the version number monotonically increasing

That's no different when the version bump is a separate commit, isn't it? A revert is also a patch level bump.

Single commits are somewhat superior in that respect because most reverts are going to cause a merge conflict in v8-version.h that will forcefully remind the reverter to make the version bump.

(Solving it with tooling is still more superior, of course.)

PRs often stay open for a long while in which case intervening backports to land with that particular version level. It would be preferable if the bump commit was a separate commit and the primary cherry-pick doesn't need editing at landing time.

Can you explain why that bothers you? There is going to be a conflict that needs to be resolved anyway. Does it really matter in what commit that is?

[targos]

+1 for what @bnoordhuis said. Putting the version bump in the same commit will more often result in a conflict that will remind the person landing it to update the patch level.

[ofrobots]

The point about the merge conflict is a good one. The case where it won't occur is when the commit at the tip-of-tree is reverted. The version number going backwards in that case would be mildly unfortunate, but not the end of the world.

The reason I don't like editing the primary commit is because it makes cherry-picking to older Node branches harder. I think you can make the same merge conflict argument, which I agree will happen in most cases, but not all.

I personally would like some tooling to do the version bumps rather than humans. Until such tooling exists, I guess it is fine to pick a solution (squashing is fine) and see how it goes.

[MylesBorins]

@ofrobots I'd be interested in exploring adding some tooling to the repo to automate the process of backporting these commits. Is there any prior art in the chromium project that is grokable?

[ofrobots]

@thealphanerd +1. I have a TODO to write up follow-up proposal from nodejs/Release#111. The gist of the process I am thinking of is: 1) maintain patches in a V8 fork. 2) Have tooling to automate the updating of V8 into nodejs/node. The version bump could happen as part of the roll process.

[natorion]

To chime in: V8 has also moved away of updating the version in each merge. We have a bot/service running which takes care of this automatically for half a year now and it makes a lot of things easier (less merge race-conditions, reverts, enables cherry-picks, ...).

[fhinkel]

CI for ppcbe-ubuntu1404 failed, but we don't have the console output anymore. Could somebody run the CI again? Thanks!

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/4285/
CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/246/

[fhinkel]

Thanks,

FreeBSD failed, not sure what the problem is:

ln -fs out/Release/node node
./node: not found

https://ci.nodejs.org/job/node-test-commit-freebsd/nodes=freebsd10-64/3464/console

[addaleax]

/cc @nodejs/build

I’m pretty sure I’ve seen that before, so it’s probably unrelated.

FreeBSD CI again: https://ci.nodejs.org/job/node-test-commit-freebsd/3466/nodes=freebsd10-64/ (Edit: green)

[Trott]

Yes, we've definitely had FreeBSD builds fail like that. Subsequent re-run by @addaleax is green. I think this is good to land, yeah?

[ofrobots]

Landed as 60d46b4.