Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: automate v8 patch update #47594

Merged
merged 7 commits into from
Apr 26, 2023
Merged

tools: automate v8 patch update #47594

merged 7 commits into from
Apr 26, 2023

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Apr 17, 2023 •
edited
Loading

Refs: nodejs/security-wg#828
please review carefully, first time approaching v8
if you know a way of creating a patch without cloning v8 (which is very expensive) that would be great
@nodejs/security-wg

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Apr 17, 2023
@marco-ippolito marco-ippolito requested a review from targos April 17, 2023 16:52
@targos
Copy link
Member

targos commented Apr 17, 2023

Did you consider using the git node command? For the V8 clone, in the canary repo we use the GitHub actions cache

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Apr 19, 2023 •
edited
Loading

@targos I basically copied what git node v8 minor was doing, I tried with the command but I had some issue with folder structure. I guess we can cache the v8 clone, should we create a new action specificly for this?

@marco-ippolito marco-ippolito force-pushed the feat/automate-v8-update branch from 03b4a51 to f8da622 Compare April 21, 2023 10:28
@marco-ippolito
Copy link
Member Author

I have created a separate action to update v8, caching the git node v8 folder. @targos

targos
targos requested changes Apr 21, 2023
View reviewed changes
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
tools/dep_updaters/update-v8-minor.sh Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito force-pushed the feat/automate-v8-update branch from f912fa3 to 0a46fe1 Compare April 21, 2023 15:59
@marco-ippolito marco-ippolito requested a review from targos April 24, 2023 08:32
targos
targos reviewed Apr 24, 2023
View reviewed changes
.github/workflows/tools.yml Outdated Show resolved Hide resolved
tools/dep_updaters/update-v8-patch.sh Show resolved Hide resolved
@marco-ippolito @targos
fix: cache only with v8
cf9270a 
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@marco-ippolito
Copy link
Member Author

sorry for changing my mind a few times but due to the amount of dependencies I've decided to move it back to update-v8.yml

@marco-ippolito marco-ippolito force-pushed the feat/automate-v8-update branch from 97e3b47 to 936482c Compare April 24, 2023 09:27
targos
targos reviewed Apr 24, 2023
View reviewed changes
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
.github/workflows/update-v8.yml Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito force-pushed the feat/automate-v8-update branch from d115ed1 to a7c237c Compare April 24, 2023 09:57
@marco-ippolito marco-ippolito force-pushed the feat/automate-v8-update branch from a7c237c to 9e5b634 Compare April 24, 2023 10:00
@marco-ippolito marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Apr 26, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 26, 2023
@nodejs-github-bot nodejs-github-bot merged commit 71a776b into nodejs:main Apr 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 71a776b

@targos
Copy link
Member

targos commented Apr 26, 2023

It doesn't work: https://github.com/nodejs/node/actions/runs/4807223999/jobs/8555708692

yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
@marco-ippolito @yjl9903
tools: automate v8 patch update
4db3645 
PR-URL: nodejs#47594
Refs: nodejs/security-wg#828
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
@marco-ippolito @yjl9903
tools: automate v8 patch update
fb949e5 
PR-URL: nodejs#47594
Refs: nodejs/security-wg#828
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
@marco-ippolito @yjl9903
tools: automate v8 patch update
40fdcb7 
PR-URL: nodejs#47594
Refs: nodejs/security-wg#828
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request May 2, 2023
@marco-ippolito @targos
tools: automate v8 patch update
63e8c95 
PR-URL: #47594
Refs: nodejs/security-wg#828
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request May 2, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@marco-ippolito @danielleadams
tools: automate v8 patch update
a072bfc 
PR-URL: #47594
Refs: nodejs/security-wg#828
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@marco-ippolito @MoLow
tools: automate v8 patch update
4e0d19e 
PR-URL: nodejs#47594
Refs: nodejs/security-wg#828
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@UlisesGascon UlisesGascon UlisesGascon approved these changes

@targos targos targos approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@marco-ippolito @nodejs-github-bot @targos @mscdex @ShogunPanda @UlisesGascon