fix: handle malformed npm packages gracefully in extract action

[gregmagolan]

Fixes #1637.

Inspired by https://github.com/frc971/971-Robot-Code/blob/master/third_party/rules_js/0001-Fix-package-permissions.patch. Thanks @AustinSchuhBRT.

Similar to what is already done in npm_import.bzl:

rules_js/npm/private/npm_import.bzl

Line 489 in f52cc4c

chmod_args = ["chmod", "-R", "a+X", _EXTRACT_TO_DIRNAME]

pngjs@5.0.0, added to npm/private/test/package.json, is a package known to have a malformed tarball where the directories don't have execute/list permissions.

[aspect-workflows]

Test

All tests were cache hits

193 tests (100.0%) were fully cached saving 42s.

---

Test

e2e/bzlmod

All tests were cache hits

4 tests (100.0%) were fully cached saving 486ms.

---

Test

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 557ms.

---

Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 7s.

---

Test

e2e/npm_link_package

All tests were cache hits

2 tests (100.0%) were fully cached saving 1s.

---

Test

e2e/npm_link_package-esm

All tests were cache hits

2 tests (100.0%) were fully cached saving 923ms.

---

Test

e2e/npm_translate_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

---

Test

e2e/npm_translate_lock_empty

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

---

Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 271ms.

---

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 117ms.

---

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 251ms.

---

Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

---

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

---

Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 566ms.

---

Test

e2e/pnpm_lockfiles

All tests were cache hits

28 tests (100.0%) were fully cached saving 10s.

---

Test

e2e/pnpm_workspace

All tests were cache hits

8 tests (100.0%) were fully cached saving 3s.

---

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

6 tests (100.0%) were fully cached saving 2s.

---

Test

e2e/repo_mapping

All tests were cache hits

2 tests (100.0%) were fully cached saving 453ms.

---

Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 605ms.

---

Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 187ms.

---

Buildifier      Format

[jbedard]

I'm still curious how frequent this is and if it's better to just patch the bad package instead? I don't think we should be writing workarounds for other peoples bugs within rules_js unless it is very common (like circular deps :/).

[gregmagolan]

I'm still curious how frequent this is and if it's better to just patch the bad package instead? I don't think we should be writing workarounds for other peoples bugs within rules_js unless it is very common (like circular deps :/).

I know of two packages so rare but impossible to know how many unless you check all npm packages on the npm registry. The failure mode is so bad that user's would have no idea that the problem is a missing x bit on directories within the npm package. IMO the defence-in-depth here with an inexpensive chmod is the right solution. My guess is the package managers do the same thing tho I haven't looked at their code to verify.

Circular deps don't feel like a bug in npm packages but rather a property of the registry which allows circular deps. Unless Bazel is in the loop, circular deps don't matter to the package managers.

[jbedard]

Maybe circular deps is a bad example then, I just mean something very command that we can't ignore. Where this seems so rare I wonder if it's better to just patch or open PRs for the bad packages 🤷

However with the use of $1 ... $2 and args I'm not so worried anymore 👍

[gregmagolan]

Even if you fixed the package at head you won't be able to fix bad tarballs in the registry for versions already published 🤷‍♂️

[jbedard]

Error in add: Cannot add directories to Args#add since they may expand to multiple values. Either use Args#add_all (if you want expansion) or args.add(directory.path) (if you do not).

I guess we can't do that. Up to you if you want to try doing .path only for directories vs everything 🤷

[gregmagolan]

I guess we can't do that. Up to you if you want to try doing .path only for directories vs everything 🤷

Lame. It should just expand to the path of the directory. I'll just special case that one.