(v6.x backport) v8: fix build errors with g++ 7

[kasicka]

This is patch from fedora rawhide to fix gcc 7 build errors, same as 2a2a556

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

Affected core subsystem(s)

[ArchangeGabriel]

Patch seems to work :), but I have no knowledge to say whether this is the correct fix.

However @kasicka to get that patch merged you’ll have to work on the commit guidelines I think. “Fix gcc7 build errors” is not a good commit message (you should rather say what you changed and then explain this was causing issue with GCC 7; and also prefix the commit message with deps/v8: so that we know where the change when), and also you will have to bump the patch level of v8. For both things, you can take a look at my PR that would have been called “Fix icu 59 build errors”.

[gibfahn]

Is this a backport of #12392? A backport was requested in #12392 (comment), so assuming this is the same fix it should land as a backport.

The backporting guide is here, basically you just cherry-pick 2bbee49 to v6.x-staging (which I think you've already done, but you should keep the commit message exactly as it was). So at this point you can just change the commit message to:

v8: fix build errors with g++ 7 This is a local patch because upstream fixed it differently by moving large chunks of code out of objects.h. We cannot easily back-port those changes due to their size and invasiveness. Fixes: https://github.com/nodejs/node/issues/10388 PR-URL: https://github.com/nodejs/node/pull/12392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]> 

and it should all be good.

[ArchangeGabriel]

@gibfahn Now that you’ve said it, yes, this definitively is a backport, but it’s not a cherry-pick of 2bbee49 since as @MylesBorins said in #12392 (comment), this wasn’t landing cleanly: indeed, while the two first files are almost identic, the third one was apparently a bit different (https://github.com/nodejs/node/pull/13574/files#diff-e6fb745db6e94b37a831f44722419e06 vs https://github.com/nodejs/node/pull/12392/files#diff-e6fb745db6e94b37a831f44722419e06).

[gibfahn]

I think a cherry-pick where you had to do some extra stuff still counts as a backport, fundamentally you're making the same change for the same reasons, it's just not identical because the original code looks different.

[ArchangeGabriel]

I think you wanted to say still counts as a “cherry-pick”. And I mostly agree with you, it’s just not exactly a git cherry-pick. ;)

[MylesBorins]

@ArchangeGabriel while I appreciate you coming in here to help out, that being said nitpicking about language around "cherry-picking" is not exactly helpful, especially as you are trying to correct someone who has helped draft our policy.

edit: I want to reaffirm that I genuinely appreciate trying to engage and help us with the review process. Just wanted to point out specific behavior I didn't find productive

[gibfahn]

I think you wanted to say still counts as a “cherry-pick”.

FWIW I did mean backport. You're right, it's not a cherry-pick. In Node core if it cherry-picks cleanly we just cherry-pick it directly, it's only if changes have to be made that we ask someone to raise a backport. So git cherry-pick + changes == a backport PR.

[ArchangeGabriel]

@MylesBorins@gibfahn Sorry to both of you, I didn’t understood you were talking of the meaning of cherry-pick and backport w.r.t. your policy (while I was referring their general meaning in VCS-based dev). But @MylesBorins is right, this was mostly useless noise from my part here.

Now what matters is that this is indeed a backport of #12392, and thus @kasicka should update the commit message as asked for this to get merged.

[kasicka]

The commit message was changed already on Friday.

[ArchangeGabriel]

@kasicka Hum, maybe you forgot to push it then?

[gibfahn]

@kasicka The PR title was updated, but the commit message wasn't updated, see here:

If you could change the commit message to this and push to your branch that'd be great, otherwise someone can fix it on landing.

[kasicka]

Pushing to wrong branch, my bad.

[gibfahn]

cc/ @nodejs/v8 (to make sure this is okay)

Assuming it's fine we can just land this on v6.x-staging, @nodejs/lts LMK if there's an issue with doing that.

[bnoordhuis]

nitpicking about language around "cherry-picking" is not exactly helpful

It's not nitpicking. I'm meticulous in distinguishing between back-ports and cherry-picks and so should you (and everyone else.)

[kasicka]

Can this get merged?

[gibfahn]

Can this get merged?

Yep, it'll get landed as part of the work to prepare the next 6.x release (which should happen over the next day or two).

It's not waiting for anything from you.

[gibfahn]

CI: https://ci.nodejs.org/job/node-test-commit/10632/

[gibfahn]

Landed in 11c7e01

[pabigot]

@gibfahn Sorry for my confusion, but where did this land? I was hoping to get a format-patch version to fix this in Yocto's meta-nodejs. The referenced commit 11c7e01 isn't in v6.x-staging or v6.x AFAICT, and though I can see the commit through the link in the "landed" comment neither fetching into my clone nor a fresh clone produce it. Maybe it's under some non-head ref?

[gibfahn]

@pabigot it landed in 11c7e0164a (the staging branch had to be rebased, and this hasn't yet gone into a release.

[pabigot]

@gibfahn Thanks; I hadn't accounted for a rebase.