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.
that link doesn't seem useful in this case
There was a problem hiding this comment.
I was thinking we should add to that doc?
There was a problem hiding this comment.
See the update to that doc
jbedard
force-pushed
the
lifecycle_hooks_use_default_shell_env
branch
3 times, most recently
from
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.
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.
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.
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.
this will be rarely used so I'm fine with the option that makes maintenance easier rather than the more ergonomic one.
jbedard
force-pushed
the
lifecycle_hooks_use_default_shell_env
branch
4 times, most recently
from
b1ffad4
to
c904028
Compare
jbedard
force-pushed
the
lifecycle_hooks_use_default_shell_env
branch
from
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.
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
jbedard
force-pushed
the
lifecycle_hooks_use_default_shell_env
branch
from
5785ed3
to
fe226e4
Compare
Depends on aspect-build/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