-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
tools: automate uvwasi dependency update #47509
tools: automate uvwasi dependency update #47509
Conversation
Review requested:
|
d67fbe8
to
8d32af9
Compare
a44d55e
to
8bf8292
Compare
|
||
* Copy over the contents of `include` and `src` to the corresponding | ||
directories. | ||
* Check if any additional files have been added and need to be added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to check this in the update script and to generate a warning/info message if there are new files found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a valid point, maybe @marco-ippolito already faced something alike in other scripts, I'll ask him!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the first attempt of automating gyp file update #47482, let's see how it goes and if its reliable we can add it to other dependencies.
For the time being I'd add a warn like:
echo "Make sure to update the deps/uvwasi/uvwasi.gyp if any significant changes have occurred upstream"
in the bash script
a5572a3
to
28d5827
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
f74bbe8
to
9563b6e
Compare
9563b6e
to
d39e562
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc: @nodejs/security-wg |
Landed in 5fa84e8 |
Refs: nodejs/security-wg#828 PR-URL: #47509 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Refs: nodejs/security-wg#828 PR-URL: #47509 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Refs: nodejs/security-wg#828 PR-URL: nodejs#47509 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Refs: nodejs/security-wg#828