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

deep merge spawnOptions.env with kernelSpec.env #40

Open
haraldschilly opened this issue Feb 8, 2019 · 2 comments
Open

deep merge spawnOptions.env with kernelSpec.env #40

haraldschilly opened this issue Feb 8, 2019 · 2 comments

Comments

@haraldschilly
Copy link
Contributor

This ticket is about refining the behavior for setting the environment of a newly started kernel. Right now, as it can be seen in this code reference the various options are shallow-merged. I.e. if there is a kernel-configured environment and a custom one passed in via the spawnOptions object, spawnOptions.env overwrites everything.

My idea about this is -- at least optionally, such that existing behavior isn't broken -- to merge these env configurations before they're passed to execa, i.e. a "deeper merge". In particular, this would allow to overwrite a single environment variable from the kernel config file, while the other ones are kept as they are. I'm for example thinking about a case, where variables like LD_LIBRARY_PATH and PATH are configured in the kernel, but I want to tweak PATH via spawnOptions and keep LD_LIBRARY_PATH as it is.

Since at least I find this confusing, I'm just clarifying this by an example:

> env = { PATH : 1, VAR : 2}
{ PATH: 1, VAR: 2 }
> spawnOptions = {env : { PATH : 3}}
{ env: { PATH: 3 } }
> Object.assign({} , {env : env} , spawnOptions)
{ env: { PATH: 3 } }                  // VAR:2 gone!

merging env one level deeper preserves VAR

> Object.assign({}, env, spawnOptions.env)
{ PATH: 3, VAR: 2 }
@BenRussert
Copy link
Member

Interesting points. I think your idea makes sense if you wanted to make a PR.

I am glad you are bringing up how this works as I wondered about this when we switched to execa. At the time, I opened #34 because I was unclear on how execa handles this and if it would work for us.

See also: #24 #14

@captainsafia
Copy link
Member

@haraldschilly If you're interested in opening a PR to make this modification, you'll need to do so in the nteract monorepo. We're actually deprecating this package and moving the logic here to the fs-kernels package. The code has been moved to this file.

@haraldschilly haraldschilly changed the title (optionally) deep merge spawnOptions.env with kernelSpec.env deep merge spawnOptions.env with kernelSpec.env Feb 5, 2020
haraldschilly added a commit to haraldschilly/spawnteract that referenced this issue Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants