Skip to content

fix: handle malformed npm packages gracefully in extract action#1794

Merged
gregmagolan merged 2 commits into
mainfrom
fix_1637__rbe
Jun 9, 2024
Merged

fix: handle malformed npm packages gracefully in extract action#1794
gregmagolan merged 2 commits into
mainfrom
fix_1637__rbe

Conversation

@gregmagolan

@gregmagolan gregmagolan commented Jun 8, 2024

Copy link
Copy Markdown
Member

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:

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

aspect-workflows Bot commented Jun 8, 2024

Copy link
Copy Markdown

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

@gregmagolan gregmagolan marked this pull request as draft June 8, 2024 22:53
@gregmagolan gregmagolan force-pushed the fix_1637__rbe branch 2 times, most recently from af18f93 to 09104a9 Compare June 8, 2024 23:50
@gregmagolan gregmagolan force-pushed the fix_1637__rbe branch 4 times, most recently from b108cb7 to d044659 Compare June 9, 2024 00:06
@gregmagolan gregmagolan requested a review from jbedard June 9, 2024 00:07
@gregmagolan gregmagolan marked this pull request as ready for review June 9, 2024 00:07
@gregmagolan gregmagolan enabled auto-merge (squash) June 9, 2024 00:12
Comment thread npm/private/npm_package_store.bzl
Comment thread npm/private/test/snapshots/bzlmod/npm_defs.bzl
@gregmagolan gregmagolan requested a review from jbedard June 9, 2024 01:08
@gregmagolan gregmagolan disabled auto-merge June 9, 2024 01:12
Comment thread npm/private/npm_package_store.bzl Outdated
@jbedard

jbedard commented Jun 9, 2024

Copy link
Copy Markdown
Member

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

gregmagolan commented Jun 9, 2024

Copy link
Copy Markdown
Member Author

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

jbedard commented Jun 9, 2024

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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

jbedard commented Jun 9, 2024

Copy link
Copy Markdown
Member
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

Copy link
Copy Markdown
Member Author

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.

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.

[Bug]: NpmPackageExtract extraction of malformed packages causes errors in RBE/Remote Cache

2 participants