validate commits to deps/v8

[targos]

We have a few rules about commits to our V8 copy, that are documented in the maintaining V8 guide. Some things could be easy to check:

• For backports / cherry-picks
  • Format of the commit title
  • Presence / correctness of the original commit message in the message
  • Presence of the Refs: github_url in the message
  • Has the embedder string been incremented?
• For regular (minor or major) updates
  • Format of the commit title

Those are just a few ideas. Let me know if it makes sense to implement them here.

[addaleax]

Let me know if it makes sense to implement them here.

I’d go with “yes”, for all of these :)

[Tiriel]

+1, could be implemented by checking the right label (again).

This makes me think, we're having more and more rules to be implemented if specific labels are present in the PR. Maybe it would be more convenient to have a "label checker" module in which we could add label checks as we need them

[joyeecheung]

@addaleax

I’d go with “yes”, for all of these :)

+1

@Tiriel

+1, could be implemented by checking the right label (again).

In this case I am afraid checking the labels is not gonna be enough, because an V8 PR might include commits that don't touch deps/v8 (e.g. updating build files, the platform, vm, or inspector). We probably will have to integrate the v3 API and look into the files. I can put together the infrastructure for that.

[joyeecheung]

So I've given it a bit of thought, turns out this should be the job of core-validate-commit. There is no need to "get the file changed" if the file is already on the user's disk, the tool just need to git show them to find out.

[targos]

Has the embedder string been incremented?

In addition of the verification, git node land could even automatically increment the embedder version if needed.

[joyeecheung]

@targos The easiest way to get this done is to operate on a per-PR basis i.e. do not mix V8 update commits in normal PRs otherwise it's hard to get the squashing right

[joyeecheung]

Also it might be easier if we just take the original commit id and generate the commit message, then compare it against the current one, given that V8 commits are very well-formatted.

[targos]

@joyeecheung

@targos The easiest way to get this done is to operate on a per-PR basis i.e. do not mix V8 update commits in normal PRs otherwise it's hard to get the squashing right

Perhaps we need a formal policy around that but the rules should be IMO:

• Commits that touch deps/v8 should not touch other directories
• Commits that touch deps/v8 must increment the embedder string (unless it's a V8 update, and in that case, v8_version.h is changed instead)

With those rules, we can't get the squashing wrong, I think.

I agree that it would be ideal to always change V8 in separate PRs, but I can understand that people don't want to wait >48h to submit their real work.

[joyeecheung]

@targos It's always hard to get the squashing right if it's not "squash everything to one commit", the user must explicitly tell the tool which commit they want to keep and because the nature of squashing using the sha is not very reliable (we can run it at the right timing but why even bother to figure out what timing is right instead of squashing everything and forget about that extra code needed?). Things are more complicated if we try to be clever and make guesses so it's just easier to keep it dumb and simple.

[joyeecheung]

By the way I think we can work around the 48h wait if the PR are submitted separately but reviewed together? I tend to submit multiple PRs where one includes a commit from another if I think it makes sense for commit A, B to be reviewed together but they don't really need to be landed together because one needs more discussion than the other. It also helps backporting changes that are not semver-major or minor because each PR can be labeled differently.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.