-
-
Notifications
You must be signed in to change notification settings - Fork 105
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: add lifecycle_hooks_use_default_shell_env to npm lifecycle hooks #1489
feat: add lifecycle_hooks_use_default_shell_env to npm lifecycle hooks #1489
Conversation
|
||
This defaults to False to reduce the negative effects of `use_default_shell_env`. | ||
|
||
Read more: [lifecycles](/docs/pnpm.md#lifecycles) |
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.
that link doesn't seem useful in this case
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 was thinking we should add to that doc?
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.
See the update to that doc
219849d
to
54cd703
Compare
Sorry maybe this should have been a draft for now, although the general idea can be reviewed now. |
@@ -8,6 +9,9 @@ npm = use_extension("@aspect_rules_js//npm:extensions.bzl", "npm") | |||
npm.npm_translate_lock( | |||
name = "npm", | |||
data = ["//:package.json"], | |||
lifecycle_hooks_use_default_shell_env = { | |||
"segfault-handler": "true", |
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.
Have we had to do this elsewhere where we want a dict of boolean values? In WORKSPACE it is a macro and the use of strings is hidden unlike 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.
a dict of boolean values seems like it ought to just be a list? I don't think the datatype has to match the other lifecycle_hooks_* necessarily
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.
That would work, would probably just require some (minor) changes to the utils to process these things. I would prefer keeping the same dict API as all the other hook params though, it's just this boolean thing makes it ugly in this case...
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.
this will be rarely used so I'm fine with the option that makes maintenance easier rather than the more ergonomic one.
b1ffad4
to
c904028
Compare
c904028
to
5785ed3
Compare
docs/pnpm.md
Outdated
@@ -275,6 +275,8 @@ which is equivalent to setting the `lifecycle_hooks` to an empty list for that p | |||
|
|||
You can set environment variables for hook build actions using the `lifecycle_hooks_envs` attribute of `npm_translate_lock`. | |||
|
|||
Some hooks may depend on environment variables specified depending on [use_default_shell_env](https://bazel.build/rules/lib/builtins/actions#run.use_default_shell_env) which may be enabled for hook build actions using the `lifecycle_hooks_use_default_shell_env` attribute of `npm_translate_lock`. |
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.
Worth mentioning that this feature depends on a minimum version of bazel-lib that adds use_default_shell_env
to run_binary
?. The MODULE.bazel of rules_js currently brings in 1.x bazel-lib that I'm not sure has that attribute.
Should also mention the minimum version in the docstrings of npm_translate_lock and npm_import if there is one
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.
🌮
5785ed3
to
fe226e4
Compare
Depends on bazel-contrib/bazel-lib@1c98ca9 if the feature is used, while maintaining support for bazel-lib <2.4.2 if not using this new feature.
Type of change
For changes visible to end-users
Test plan