Uh oh!
There was an error while loading. Please reload this page.
tools: add update script for googletest - #47482
Conversation
nodejs-github-bot
commented
Apr 8, 2023
Review requested:
|
marco-ippolito
commented
Apr 8, 2023
Im not sure about updating the gyp file, its too complex to automate it we should probably do it manually |
tniessen
commented
Apr 8, 2023
I tend to agree with that in general. However, in this case, if the script does make a mistake in producing the gyp file, we still have our regular review process as well as Github Actions and Jenkins CI as safeguards, and we can still fix the gyp file manually if necessary. A mistake that is not caught by any of these safeguards could just as well be the result of manual work. In other words, we can take the generated gyp file as a suggestion, which hopefully will be correct in most cases, thus usually removing the need to update it manually. When we or our automated tests notice a problem with the suggested change, we can still resort to updating it manually. I'll try and see what happens with previous versions of googletest (i.e., if the script would have made mistakes in the past). That being said, I'd be happy to switch to a more reliable approach if we can come up with one. |
anonrig
left a comment
There was a problem hiding this comment.
I like this approach. Recently, I read about a similar blog post about how Github updates its Ruby version every week by following main branch. We can always revert back or interfere with the upgrade process and I believe this would be a valuable lesson/experience we could benefit as the whole Node.js project.
marco-ippolito
left a comment
There was a problem hiding this comment.
LGTM, lets try with this approach if we are happy we can extend it to zlib and also try to automate some other dependencies gyp
4782ef4 to
9304143Compare9304143 to
4c1af7bCompare4c1af7b to
a31c52eComparetniessen
commented
Apr 18, 2023
Appeased lint-sh. PTAL @marco-ippolito. |
Uh oh!
There was an error while loading. Please reload this page.
a31c52e to
f2eae6bComparetniessen
commented
Apr 19, 2023
Rebased again and updated for #47591. |
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828
f2eae6b to
635b183Comparetniessen
commented
Apr 19, 2023
@marco-ippolito Is my understanding correct that the GitHub action will keep updating the same PR instead of opening new ones if a new update is found before the PR is merged? That would be great to reduce noise in this case, if we could just keep the PR open regardless of how often the action runs. |
marco-ippolito
commented
Apr 19, 2023
if there is a pr for |
richardlau
commented
Apr 19, 2023
According to https://github.com/gr2m/create-or-update-pull-request-action#how-it-works it should update the existing PR (more specifically, it pushes to the same branch as the previously opened PR for the dependency). I've seen this happen on some of the Undici PRs (e.g. #46502). |
Oh that's great then we are good on this |
tniessen
commented
Apr 19, 2023
I like that not creating additional PRs reduces notification noise, but it unfortunately seems to unnecessarily keep old commits and also doesn't update the PR title or description. |
richardlau
commented
Apr 19, 2023
Updating the title/body can be controlled by the |
tniessen
commented
Apr 19, 2023
Thanks, I saw that in the docs right after I left the previous comment and opened #47621. Regardless, this PR needs to be re-approved after the last force-push :) |
nodejs-github-bot
commented
Apr 19, 2023
Landed in 4956271 |
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828 PR-URL: #47482 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828 PR-URL: #47482 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while. The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future. Refs: nodejs/security-wg#828 PR-URL: nodejs#47482 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
I haven't done one of these before, so please review carefully.
GoogleTest follows the Abseil Live at Head philosophy, and rarely creates tags or GitHub releases, so instead, follow Google's recommendation and update to the upstream HEAD every once in a while.
The tricky bit is properly updating googletest.gyp, and this script might fail doing so in the future.
Refs: nodejs/security-wg#828