fix: traverse extracted package using the find utility

plobsing commented 3 months ago

Recursive chmod would work fine, but we want to skip the top-level directory. Using a shell glob to enumerate the first-level children, as was done with https://github.com/aspect-build/rules_js/pull/1894, runs into difficulties when importing large packages: we can run afoul of ARG_MAX. This is observed, for instance, when extracting @material-ui/icons:

Extracting npm package @material-ui/icons@4.11.3_1407018657 failed: (Exit 126): bash failed: error executing NpmPackageExtract command (from target //web/web-ui:.aspect_rules_js/node_modules/@material-ui+icons@4.11.3_1407018657/pkg) /bin/bash -c '$1 --extract --no-same-owner --no-same-permissions --strip-components 1 --file $2 --directory $3 && chmod -R a+X $3/*' '' external/aspect_bazel_lib~~toolchains~bsd_tar_darwin_arm64/tar ... (remaining 2 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
: /bin/chmod: Argument list too long

The number of characters in the enumeration of all top-level entries in the package's archive is 492 KiB:

$ tar tf aspect_rules_js\~\~npm\~webui_npm__at_material-ui_icons__4.11.3_1407018657/package.tgz | wc -c
  504139

This is a sizeable fraction of the ARG_MAX (1024 KiB on MacOS):

$ sysctl kern.argmax
kern.argmax: 1048576

While the relative filenames are still under the limit, we go over once we prefix each filename with package_store_directory.

Changes are visible to end-users: no

Test plan

Covered by existing test cases
Manual testing; please provide instructions so we can reproduce:
- Confirmed the failure that motivated https://github.com/aspect-build/rules_js/pull/1894 is still fixed in a Buildkite-based CI that was observed to be affected by the issue when running an unpatched version of aspect_rules_js v2.0.0.

aspect-workflows[bot] commented 3 months ago

Test

All tests were cache hits

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

Test

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 631ms.

Test

e2e/gyp_no_install_script

All tests were cache hits

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

Test

e2e/js_image_oci

All tests were cache hits

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

Test

e2e/npm_link_package

All tests were cache hits

3 tests (100.0%) were fully cached saving 925ms.

Test

e2e/npm_link_package-esm

All tests were cache hits

3 tests (100.0%) were fully cached saving 925ms.

Test

e2e/npm_translate_lock

All tests were cache hits

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

Test

e2e/npm_translate_lock_empty

All tests were cache hits

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

Test

e2e/npm_translate_lock_multi

All tests were cache hits

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

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

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

Test

e2e/npm_translate_lock_replace_packages

All tests were cache hits

3 tests (100.0%) were fully cached saving 770ms.

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

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

Test

e2e/npm_translate_package_lock

All tests were cache hits

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

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

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

Test

e2e/package_json_module

All tests were cache hits

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

Test

e2e/pnpm_lockfiles

All tests were cache hits

40 tests (100.0%) were fully cached saving 16s.

Test

e2e/pnpm_workspace

All tests were cache hits

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

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

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

Test

e2e/repo_mapping

All tests were cache hits

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

Test

e2e/rules_foo

All tests were cache hits

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

Test

e2e/vendored_node

All tests were cache hits

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

Buildifier Format

plobsing commented 3 months ago

Withdrawing proposal. find is not universally available. https://github.com/aspect-build/rules_js/pull/1904 is a better proposal.

aspect-build / rules_js