dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.09k stars 1.56k forks source link

Allow environment variables to be set within a process #28160

Open nex3 opened 7 years ago

nex3 commented 7 years ago

To clarify: In this issue I'm referring to the OS's notion of environment variables, not the thing you pass in to a Dart process via -D.

Currently Dart has no means of modifying the set of environment variables within a process. This turns out to be important for integrating the test package with the Bolt interactive runner. Specifically, we need a way of setting environment variables within a specific section of the program. My ideal API for this would be:

T withEnvironment<T>(T callback(), Map<String, String> newValues);

where callback() runs in a zone such that:

I could live with an API that only sets these variables on Isolate boundaries, though, such as a Map<String, String> osEnvironment argument to Isolate.spawnUri() that has no effect on non-VM platforms.

lrhn commented 7 years ago

It should be possible to add a function using Posix setenv (or equivalent) to update the process environment exposed by Platform.environment.

It probably won't be possible to do that only for some code running in the process - that's just not the abstraction level that the environment lives at.

nex3 commented 7 years ago

It probably won't be possible to do that only for some code running in the process - that's just not the abstraction level that the environment lives at.

But it could be. Dart code accesses the environment purely through the dart:io APIs, and those APIs don't necessarily have to correspond exactly with the underlying C APIs. You could have a Zone variable that Platform.environment, Process.*, and Isolate.spawn* check in pure Dart.

lrhn commented 7 years ago

It's definitely possible, but then we are inventing a new concept that isn't a per-process environment. Nothing wrong with that, but that's not what Platform.environment is. We'd still want a way to access the actual process environment, so we might as well keep Platform.environment for that, and then add the new concept as something separate (and better designed for its job).

nex3 commented 7 years ago

I think it was you who told me once that if Isolates can't do everything that subprocesses can, that's a library issue that should be fixed :wink:. We have a concrete situation now with an important tool (Bolt) where they want to load code at runtime using isolates, but they need that code to have a different view of the environment than the parent isolate. They could use subprocesses for this, but in every other way isolates are perfect, whereas subprocesses take longer to start and are less efficient to communicate with.

It's also important that the environment logic be invisible. The code that's environment-sensitive may not even be aware of the context it's running in; it's just starting a process and assuming that the right environment will be passed through. We need something to enable that pattern, not a new layer on top that no existing code is using.

lrhn commented 7 years ago

I think it was you who told me once that if Isolates can't do everything that subprocesses can, that's a library issue that should be fixed 😉

Damn you, past-me, for giving away all the good arguments!

It is true, I said that, and it also suggests that this is really an isolate problem, not an environment problem as such. It's an "environment override" on isolate spawn that is really needed (and only for Isolate.spawnUri).

nex3 commented 7 years ago

I'd be fine if this could only be set on Isolate.spawnUri().

nex3 commented 6 years ago

Can we move on this? It's important for Bolt and thus most of our internal customers.

zanderso commented 6 years ago

It should be possible to add an entry for Platform.environment to IOOverrides. Spawned processes would use the overridden Platform.environment by default. We could then use a new osEnvironment argument to Isolate.spawnUri to seed IOOverrides.global in the new Isolate (if dart:io exists). The details could be hairy---it looks like the environment argument to Isolate.spawnUri might not even be implemented, yet...

lrhn commented 6 years ago

There is no environment argument to Isolate.spawnUri because isolates used to work on platforms that had no dart:io. I'd refer that dart:io had its own spawnIsolate function that does the same as spawnUri and allows the Process environments to be overridden, rather than making dart:isolate deal with dart:io specific features.

If Isolate only exists on the stand-alone VM anyway, keeping the distinction might not be as important, but I'm wary of assuming that no future platform will have dart:isolate without dart:io.

zanderso commented 6 years ago

Just to clarify: There is an environment argument to Isolate.spawnUri whose entries are supposed to be in the new Isolate's String.fromEnvironment. See https://github.com/dart-lang/sdk/blob/master/sdk/lib/isolate/isolate.dart#L302. There is currently nothing like osEnvironment, though. Whether the feature is added to dart:isolate or in a new call in dart:io the implementation will look roughly the same. I'd be interested in hearing peoples' thoughts about where in dart:io the call should go. Maybe Process.spawnIsolate?

nex3 commented 6 years ago

What's the benefit to adding this to IOOverrides rather than making Platform.environment mutable?

I'm not totally convinced that it's worth adding isolate-local environment variables once they can be set, since it's always possible to just add a wrapper to the isolate that does that manually. If we do decide it's worth doing, though, I strongly believe we should add a parameter to the existing spawn functions. Splitting them in two when we have no actual or planned platforms that support isolates without dart:io seems like YAGNI, especially since we can (and have in the past) have isolates implement some parameters and throw on others.

zanderso commented 6 years ago

What's the benefit to adding this to IOOverrides rather than making Platform.environment mutable?

The ability of Isolates to interfere with each other by way of dart:io is generally something we'd like to decrease rather than increase.

nex3 commented 6 years ago

Why not just have the map be per-isolate?

zanderso commented 6 years ago

I'd find it pretty confusing if the default behavior was that reading Platform.environment read from the global environment for the process, but writing to it only wrote to an Isolate-local one. If you have to go through a different API (IOOverrides) to make changes, I think that is a sufficiently strong signal that the process's global environment isn't going to be modified.

nex3 commented 6 years ago

What about this:

nex3 commented 5 years ago

Dart Sass needs this feature now as well. We need to test the behavior of the library when the SASS_PATH environment variable is set.

davidmorgan commented 5 years ago

I just ran into this too; the Isolate.spawnProcess(..., environment: {...}) API would be sufficient for my needs, but the VM handily tells me: UnimplementedError: environment. Could I ask what platforms it is implemented for, please--and if any more are coming?

Thanks.

zanderso commented 5 years ago

It looks like the environment parameter to Isolate.spawnUri() was never implemented on the VM: https://github.com/dart-lang/sdk/commit/232a9d687e9abf3a315e9b5c980334c2313f94a8#diff-bdfa815e10abb469ed0659539811630e

Solido commented 5 years ago

@davidmorgan Any news on your concern ? just hit the same.

davidmorgan commented 5 years ago

@Solido haven't heard anything, sorry.

bsutton commented 3 years ago

Just a vote for some action on this one.

I maintain a library DCli which is designed to help build cli apps. https://pub.dev/packages/dcli

I also have a project nginx-le which is a docker container for nginx which automates lets encrypt certificates. https://pub.dev/packages/nginx_le

nginx-le spawns isolates which perform such actions as log rotation and acquisition and renewal of lets encrypt certs.

The lets encrypt isolate within nginx-le spawns the lets encrypt process which requires environment variables be set to control its behaviour.

When the DCli library is initialised it takes a copy of Platform.environment and allows users to set/get environment variables via a global property 'env' env['USER'] env['USER'] = 'bsutton';

When DCli is used to spawn a process it automatically passes the contents of 'env' to the process.

nginx-le uses DCli env[] = method to set environment variables.

The problem I encountered was when nginx-le spawns an isolate to call the lets encrypt process any environment variables set via DCli (in the main isolate) are (of course) not inherited via the lets encrypt isolate.

As such I've had to build a crude mechanism which jsonises the environment variables, passes them to the isolate as a string and have the isolate deserialize the json back into the DCli environment.

Having a means to control the initial environment of the isolate would overcome this issue.

I'm using IsolateRunner.spawn().

chrisnorman7 commented 3 years ago

Hi, +1 from me too.

I'm trying to use a DLL from within Flutter, and the DLL relies on a second DLL. If I could just modify the PATH environment variable, I could make that happen with Flutter assets. As it goes, I've not found a solution yet.

Cheers,

Chris