-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
npm package untar not to use remote-cache #1974
base: main
Are you sure you want to change the base?
Conversation
# cache in most of the cases, and untar creates mutliple outputs and sometimes the number of the output can be massive | ||
# meaning it can takes much more time to upload the cache compared to the actual untar. | ||
# see https://github.com/aspect-build/rules_js/pull/1974 | ||
execution_requirements = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd believe this is a speedup for local builds, but I think it's a pessimization for remote builds, since you'd need to upload the results anyway to the remote cluster. Same exact reasoning as the execution requirements on bazel-lib CopyFile
applies here. So let's make this configurable if we do land it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remembered the original CopyFile/Directory execution requirements didn't play well with BWOB and was worried about the same thing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the problem is:
- if inputs are local, and subsequent actions that depend on the outputs will run local, you want this action to run local and skip writing the result to remote cache
- if inputs are remote, or subsequent actions are remote, you want this action to run remote and store result in the remote cache
So there isn't a reasonable default we can use for everyone. Perhaps a better answer is to document that you can use --modify_execution_info= NpmPackageExtract=+no-remote
if you find your builds are better tuned with that setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another possibility is to add that flag to a preset, like https://github.com/bazel-contrib/bazel-lib/blob/main/.aspect/bazelrc/javascript.bazelrc - this way more devs will discover it
It's pretty meaningless to use remote-cache for npm
untar
action. All npm packages are small in size I think?And even if there's a big tarball, it probably take similar time to extract the tarball vs downloading these files from remote-cache? Even if the remote-cache is a local implemented one.