Open targos opened 6 years ago
Let me know if it makes sense to implement them here.
I’d go with “yes”, for all of these :)
+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
@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.
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.
Has the embedder string been incremented?
In addition of the verification, git node land
could even automatically increment the embedder version if needed.
@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
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.
@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:
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.
@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.
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.
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.
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:
Refs: github_url
in the messageThose are just a few ideas. Let me know if it makes sense to implement them here.