deps: V8: bump v8_embedder_string for 0e21c1e637bf #31096
Conversation
0e21c1e has landed without a proper v8_embedder_string bump, this is a follow-up fix. Refs: 0e21c1e Refs: nodejs#31007
nodejs-github-bot
added
the
build
There was a problem hiding this comment.
This LGTM and I’d prefer this approach over a revert, but tbh, I’m good with just leaving this alone and doing nothing.
(Maybe to expand on that: The commit just before the landed on already bumps the embedder string. There isn’t any point in re-incrementing it, as far as I can tell.)
Shouldn't each individual backport commit bump the embedder string? |
|
@BridgeAR Afaik, the only purpose of the embedder string is to identify a specific modified version of V8. If two V8 changes land right next to each other, why do we need to bump the string twice? |
|
Sounds like this has become unnecessary. I'm closing it, but of course feel free to re-open if you think I'm wrong to close it. |
|
For clarity I reopened this PR because I think simply bumping the embedder string is a better course of action than reverting and relanding the other PR |
|
The revert being #31098 |
I closed it because I thought there was consensus on the third option: Do nothing because the string got bumped elsewhere in the meantime. (At least that was my interpretation of #31096 (review) and #31096 (comment) but maybe I'm misunderstanding something.) |
|
We have generally bumped the string for every commit. Just because they came in the same PR doesn't mean they are treated as a single atomic change for the bump At least that is how I've understood it to have been done up until this point. We can choose to do nothing, but I think the bump in a separate commit is the correct action. Either of those are better than revert and reland imho |
BridgeAR
added
the
author ready
0e21c1e has landed without a proper v8_embedder_string bump, this is a follow-up fix. PR-URL: #31096 Refs: 0e21c1e Refs: #31007 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
Landed in 9b0cbcf 🎉 I went ahead and landed it. That way it's consistent with the way we handled it so far. |
0e21c1e has landed without a proper v8_embedder_string bump, this is a follow-up fix. PR-URL: #31096 Refs: 0e21c1e Refs: #31007 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
0e21c1e has landed without a proper
v8_embedder_stringbump, this is a follow-up fix.Refs: 0e21c1e
Refs: #31007
The alternative would be to revert 0e21c1e (PR: #31098) and re-land #31007. Filing both to speed up things.