-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add patch_tool argument to npm_translate_lock #2048
base: main
Are you sure you want to change the base?
Add patch_tool argument to npm_translate_lock #2048
Conversation
53dba40
to
6274d2e
Compare
6274d2e
to
42542d1
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.
thanks!
patch(rctx, patch_args = rctx.attr.patch_args, patch_directory = _EXTRACT_TO_DIRNAME) | ||
patch( | ||
rctx, | ||
patch_tool = rctx.path(rctx.attr.patch_tool) if rctx.attr.patch_tool else "patch", |
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.
we should consider ALWAYS using a hermetic patch implementation. I recall we looked into the one Bazel itself uses, but it wasn't exposed for repository rules to use? Maybe @gregmagolan remembers. Can be in a later PR though.
Hm, I don't understand why this isn't triggering our GitHub Actions CI. Is it rebased on a really old base commit? |
Oh right. I had this on top of v2.1.0 which is what we're still currently using. Let me rebase. |
96c9754
to
9b797d8
Compare
We encountered a bug with MacOS's implementation with
patch
, likely due to it being based on an ancient version ofpatch
.One downstream effect of
patch
being broken locally is that it leads to remote cache misses since our remote cache warming machines are using a non-broken implementation ofpatch
.This PR adds support for specifying a custom patch tool so that we can use a non-broken implementation.
Changes are visible to end-users: yes
Test plan
Manual testing; please provide instructions so we can reproduce:
npm.npm_translate_lock