Skip to content

feat: Support hermetic patching#1453

Open
LaurenceTews wants to merge 1 commit into
aspect-build:2.xfrom
LaurenceTews:Hermetic_patch
Open

feat: Support hermetic patching#1453
LaurenceTews wants to merge 1 commit into
aspect-build:2.xfrom
LaurenceTews:Hermetic_patch

Conversation

@LaurenceTews

@LaurenceTews LaurenceTews commented Jan 22, 2024

Copy link
Copy Markdown

Allows Bazel native patch to be used with npm_import(). This removes the existing dependency on patch when enabled. The proviso is that patch diff headers must have an unstripped 'package/' prefix when use_native_patch is true. The dirname 'package' is the de facto standard for npm packages, though this isn't enforced.

Type of change

  • New feature or functionality (change which adds functionality)
  • Documentation (updates to documentation or READMEs)

For changes visible to end-users

Suggested release notes are provided below:

  • use_native_patch flag added to npm_import, allowing for hermetic patching.

Test plan

Manual testing steps:

  • set BAZEL_SH var to a nonexistent path.
  • Create valid patch. Patch diff header should contain package prefix, e.g. "diff --git package/lib/font.coffee ..."
  • verify that npm_import() with patch applied fails "Error applying patch". System 'patch' cannot be found.
  • verify that npm_import() with patch applied and use_native_patch set to 'True' succeeds.

@LaurenceTews

Copy link
Copy Markdown
Author

@gregmagolan Just wanted to gauge if there's any interest in this feature? I know Windows is a lesser used platform, but I'm sure there's teams out there (like mine) who would find this useful.

@matthewjh

Copy link
Copy Markdown

@gregmagolan any thoughts on this MR? I tried to run a rules_js project on Windows, and it blew up while resolving NPM packages due to the "patch" dependency.

@smocherla-brex

smocherla-brex commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

Ran into this issue as well on our Macs which have an older version of patch which is stricter than the one on Linux. Would be nice to use the native patch implementation as opt-in as this PR does. (although for our usecase a note in the docs for npm_translate_lock regarding this would also suffice)

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants