Skip to content
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

feat: extract npm package tars into virtual store as action using tar toolchain from bazel-lib #1538

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

gregmagolan
Copy link
Member

@gregmagolan gregmagolan commented Mar 22, 2024

Should mostly fix #1412. I say mostly since this PR only applies for npm package with no lifecycle hooks and no patches. This is the majority of packages. This PR changes these so they will no longer use a CopyDirectory action with a source directory input to copy into the virtual store. Instead they will use a tar toolchain to extract the package .tgz directly into the virtual store.

Packages with lifecycle hooks and patches will be handled in follow-up PRs. For lifecycle hooks, the lifecycle hook binary will need to run tar --extract itself. This should be fairly straight forward. For packages with patches, we'll likely need a patch toolchain in bazel-lib so we have a hermetic patch we can call after tar --extract in some "extract and patch" action in both npm_package_store and the lifecycle hook binary.


Type of change

  • New feature or functionality (change which adds functionality)

Test plan

  • Covered by existing test cases

@gregmagolan gregmagolan force-pushed the extract_npm_tar branch 6 times, most recently from 3d9cf8d to 9b17a2c Compare March 23, 2024 06:20
@jbedard
Copy link
Member

jbedard commented Mar 23, 2024

Can tar within _download_and_extract_archive also use the toolchain?

@gregmagolan
Copy link
Member Author

Can tar within _download_and_extract_archive also use the toolchain?

It could but I'd rather change one thing at a time and do that in a follow-up. tar happens to ship on darwin, linux and Windows so its safe to call tar in a repo rule. Not causing any problems in the repo rule atm tho switching to use the toolchain would allow us to remote the _is_gnu_tar check since we pass a gnu tar specific flag to tar --extract.

@jbedard
Copy link
Member

jbedard commented Mar 23, 2024

It could but I'd rather change one thing at a time and do that in a follow-up.

Yeah 👍

I just want to make sure it's considered because the tar call in _is_gnu_tar is a perf issue on cold builds atm.

npm/private/npm_import.bzl Outdated Show resolved Hide resolved
MODULE.bazel Outdated Show resolved Hide resolved
npm/private/npm_package_store.bzl Outdated Show resolved Hide resolved
npm/private/npm_package_store.bzl Outdated Show resolved Hide resolved
e2e/gyp_no_install_script/MODULE.bazel Outdated Show resolved Hide resolved
@gregmagolan gregmagolan force-pushed the extract_npm_tar branch 5 times, most recently from 2855f93 to 1afd514 Compare March 27, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug]: Flaky build failure: npm package directory copy fails with "No such file or directory"
3 participants