On Wed, Mar 13, 2013 at 09:43:59PM +0100, Lars T. Kyllingstad wrote:
> On Wednesday, 13 March 2013 at 20:26:44 UTC, Steven Schveighoffer wrote:
[...]
> >But with something similar to Dennis' idea, we have a possible way to do that without making a copy of the current environment into an AA and adding:
> >
> >struct EnvironmentArg
> >{
> >     this(string[string] env, bool useParent=false) { this.env =
> >env;
> >this.useParent = useParent;}
> >     this(bool useParent) {this.useParent = useParent;}
> >     string[string] env;
> >     bool useParent;
> >}
> >
> >spawnProcess("helloworld", EnvironmentArg(["x":"y"], true)); // use parent
> >environment, add x=y
> >spawnProcess("helloworld", EnvironmentArg(["x":"y"])); // replace environment with x=y
> >spawnProcess("helloworld", EnvironmentArg(false)); // use empty environment
> >spawnProcess("helloworld", EnvironmentArg(true)); // use parent environment exactly
> >
> >EnvironmentArg should probably have better name, and I would recommend some global functions that make common things, like:
> >
> >EnvironmentArg emptyEnvironment() { return EnvironmentArg(null, false);}
> >EnvironmentArg parentEnvironment() { return EnvironmentArg(null, true);}
> >
> >Like? Hate?
> 
> Hmm.. what if spawnProcess() takes a normal string[string] like it does now, but we add a flag to Config that determines whether it is merged with the parent's environment or not?
> 
> string[string] myEnv = [ "foo" : "bar" ];
> spawnProcess("helloworld", null);     // Parent's env
> spawnProcess("helloworld", myEnv);    // Parent's env + myEnv
> spawnProcess("helloworld", null, ..., Config.clearEnv);  // Empty env
> spawnProcess("helloworld", myEnv, ..., Config.clearEnv); // Only myEnv
[...]

+1. I like this idea. Makes code more self-documenting, which is a good thing.

Alternatively:

	struct useParentEnv { string[string] aa; }
	struct newEnv { string[string] aa; }

	spawnProcess(E)(string program, E env, ...) {
		static if (is(E == useParentEnv))
			// inherit values from current environment
		else
			// don't inherit from current environment
	}

	spawnProcess("helloworld", useParentEnv(["a": "b", ... ]), ...);
	spawnProcess("helloworld", newEnv(["a": "b", ... ]), ...);

Of course, rename useParentEnv and newEnv to something more suitable.


T

-- 
"You know, maybe we don't *need* enemies." "Yeah, best friends are about all I can take." -- Calvin & Hobbes

On Wednesday, 13 March 2013 at 20:44:00 UTC, Lars T. Kyllingstad
wrote:
> On Wednesday, 13 March 2013 at 20:26:44 UTC, Steven Schveighoffer wrote:
>> I hate to have feature creep at this point, but one kind of annoying thing
>> is, if you want to *add* to the current environment, it is a multi-step
>> process:
>>
>> auto curenv = environment.toAA;
>> curenv["x"] = "y";
>> spawnProcess("helloworld", curenv);
>>
>> But with something similar to Dennis' idea, we have a possible way to do
>> that without making a copy of the current environment into an AA and
>> adding:
>>
>> struct EnvironmentArg
>> {
>>     this(string[string] env, bool useParent=false) { this.env = env;
>> this.useParent = useParent;}
>>     this(bool useParent) {this.useParent = useParent;}
>>     string[string] env;
>>     bool useParent;
>> }
>>
>> spawnProcess("helloworld", EnvironmentArg(["x":"y"], true)); // use parent
>> environment, add x=y
>> spawnProcess("helloworld", EnvironmentArg(["x":"y"])); // replace
>> environment with x=y
>> spawnProcess("helloworld", EnvironmentArg(false)); // use empty environment
>> spawnProcess("helloworld", EnvironmentArg(true)); // use parent
>> environment exactly
>>
>> EnvironmentArg should probably have better name, and I would recommend
>> some global functions that make common things, like:
>>
>> EnvironmentArg emptyEnvironment() { return EnvironmentArg(null, false);}
>> EnvironmentArg parentEnvironment() { return EnvironmentArg(null, true);}
>>
>> Like? Hate?
>
> Hmm.. what if spawnProcess() takes a normal string[string] like it does now, but we add a flag to Config that determines whether it is merged with the parent's environment or not?
>
> string[string] myEnv = [ "foo" : "bar" ];
> spawnProcess("helloworld", null);     // Parent's env
> spawnProcess("helloworld", myEnv);    // Parent's env + myEnv
> spawnProcess("helloworld", null, ..., Config.clearEnv);  // Empty env
> spawnProcess("helloworld", myEnv, ..., Config.clearEnv); // Only myEnv

The more I think about this, the more it seems like a good idea:

1. A string[string] parameter for the environment, which defaults
to null, and for which there is no difference between null and
empty -- they are both empty.

2. A Config flag that determines whether the given AA should be
merged with the parent's environment or not, with the former
being the default.

3. Variables in the AA always override variables from the
parent's environment.

4. The two spawnProcess() overloads that do *not* take an
environment parameter will be removed.

5. Instead, we add two overloads without the redirection
parameters, since the Config parameter will probably be used more
often now:

   spawnProcess(string prog, string[string] env, Config conf);
   spawnProcess(string[] args, string[string] env, Config conf);

Looks good?

Lars

On Thu, 14 Mar 2013 16:20:24 -0400, Lars T. Kyllingstad <public@kyllingen.net> wrote:

> The more I think about this, the more it seems like a good idea:
>
> 1. A string[string] parameter for the environment, which defaults
> to null, and for which there is no difference between null and
> empty -- they are both empty.
>
> 2. A Config flag that determines whether the given AA should be
> merged with the parent's environment or not, with the former
> being the default.
>
> 3. Variables in the AA always override variables from the
> parent's environment.
>
> 4. The two spawnProcess() overloads that do *not* take an
> environment parameter will be removed.
>
> 5. Instead, we add two overloads without the redirection
> parameters, since the Config parameter will probably be used more
> often now:
>
>     spawnProcess(string prog, string[string] env, Config conf);
>     spawnProcess(string[] args, string[string] env, Config conf);
>
> Looks good?

Looks good.

Part of me thinks you shouldn't have to specify environment in order to specify redirects, but I don't know how that works with the overloads.  I know File is a struct, so it shouldn't bind to null, right?

By "AA should be merged with the parent's environment or not, with the former being the default", I'm assuming the "set" flag will mean "don't inherit".  What name do you have in mind?  Since we already have dontDoTheCloseThing, I think dontDoTheEnvironmentInheritThing would be good ;)

I know it's bikeshedding, but negative flags are awful, we should come up with positive ones.  ignoreParentEnv?

-Steve

On Thursday, 14 March 2013 at 20:34:11 UTC, Steven Schveighoffer wrote:
> On Thu, 14 Mar 2013 16:20:24 -0400, Lars T. Kyllingstad <public@kyllingen.net> wrote:
>
>> The more I think about this, the more it seems like a good idea:
>>
>> [...]
>>
>> Looks good?
>
> Looks good.
>
> Part of me thinks you shouldn't have to specify environment in order to specify redirects, but I don't know how that works with the overloads.  I know File is a struct, so it shouldn't bind to null, right?

No, there won't be any problem with adding overloads with and without environment, but then there'll be six spawnProcess() versions.  We have to weigh that against the user having to explicitly specify a null when they don't want to add to the environment, but still want redirects.  I don't know which is worse.  I don't think this is too bad, though:

  auto p = spawnProcess("myapp", null, File("input.txt"));


> By "AA should be merged with the parent's environment or not, with the former being the default", I'm assuming the "set" flag will mean "don't inherit".  What name do you have in mind?  Since we already have dontDoTheCloseThing, I think dontDoTheEnvironmentInheritThing would be good ;)
>
> I know it's bikeshedding, but negative flags are awful, we should come up with positive ones.  ignoreParentEnv?

Now that the big pieces are seemingly falling into place, it is probably time for bikeshedding.  I was thinking clearEnv or newEnv, but ignoreParentEnv is perhaps more explicit.

Speaking of negative flags, do you have better suggestions for the Config.noCloseStd... ones?

dontDoTheCloseThing became inheritFDs, btw. :)  Also open for suggestions on that one.

Lars

On Thursday, 14 March 2013 at 21:51:37 UTC, Lars T. Kyllingstad wrote:
> On Thursday, 14 March 2013 at 20:34:11 UTC, Steven Schveighoffer wrote:
>> Part of me thinks you shouldn't have to specify environment in order to specify redirects, but I don't know how that works with the overloads.  I know File is a struct, so it shouldn't bind to null, right?
>
> No, there won't be any problem with adding overloads with and without environment, but then there'll be six spawnProcess() versions.  We have to weigh that against the user having to explicitly specify a null when they don't want to add to the environment, but still want redirects.  [...]

We could switch them around, though, and put the environment after the redirects:

spawnProcess(args, stdin, stdout, stderr, env, config)
spawnProcess(args, env, config)
spawnProcess(prog, stdin, stdout, stderr, env, config)
spawnProcess(prog, env, config)

I didn't specify any default values there, but every parameter would have a default except the first one.

Lars

Am Thu, 14 Mar 2013 22:51:36 +0100
schrieb "Lars T. Kyllingstad" <public@kyllingen.net>:

> Now that the big pieces are seemingly falling into place, it is probably time for bikeshedding.  I was thinking clearEnv or newEnv, but ignoreParentEnv is perhaps more explicit.

I think clearEnv is pretty clear already. Someone should put up an online poll for that.

> dontDoTheCloseThing became inheritFDs, btw. :)  Also open for suggestions on that one.

Looks like a mix of Windows and Unix terminology. :) closeHandlesOnExec anyone? Am I right assuming that on Windows it will just not set the bInheritHandles flag?

-- 
Marco

On Friday, 15 March 2013 at 00:36:59 UTC, Marco Leise wrote:
> Am Thu, 14 Mar 2013 22:51:36 +0100
> schrieb "Lars T. Kyllingstad" <public@kyllingen.net>:
>
>> Now that the big pieces are seemingly falling into place, it is probably time for bikeshedding.  I was thinking clearEnv or newEnv, but ignoreParentEnv is perhaps more explicit.
>
> I think clearEnv is pretty clear already. Someone should put
> up an online poll for that.

Bikeshedding, yes, but I don't think we've quite reached the point where we need polls yet. :)


>> dontDoTheCloseThing became inheritFDs, btw. :)  Also open for suggestions on that one.
>
> Looks like a mix of Windows and Unix terminology. :)
> closeHandlesOnExec anyone? Am I right assuming that on Windows
> it will just not set the bInheritHandles flag?

You are not entirely right.  The flag is used only by POSIX code, and causes spawnProcess() to *not* close all open file descriptors.  On Windows, inheritFDs has no effect whatsoever.

closeHandlesOnExec looks even more like a mix to me. I can't recall ever seeing the word "handle" used in *NIX documentation.  "File descriptor" is used pretty consistently.  I also tried to avoid using the words "close on exec" in the name, because I didn't want users to think this has anything to do with the FD_CLOEXEC flag.

Lars

On Thu, 14 Mar 2013 17:51:36 -0400, Lars T. Kyllingstad <public@kyllingen.net> wrote:

> On Thursday, 14 March 2013 at 20:34:11 UTC, Steven Schveighoffer wrote:
>> On Thu, 14 Mar 2013 16:20:24 -0400, Lars T. Kyllingstad <public@kyllingen.net> wrote:
>>
>>> The more I think about this, the more it seems like a good idea:
>>>
>>> [...]
>>>
>>> Looks good?
>>
>> Looks good.
>>
>> Part of me thinks you shouldn't have to specify environment in order to specify redirects, but I don't know how that works with the overloads.  I know File is a struct, so it shouldn't bind to null, right?
>
> No, there won't be any problem with adding overloads with and without environment, but then there'll be six spawnProcess() versions.  We have to weigh that against the user having to explicitly specify a null when they don't want to add to the environment, but still want redirects.  I don't know which is worse.  I don't think this is too bad, though:
>
>    auto p = spawnProcess("myapp", null, File("input.txt"));

OK, that is fine with me.

>> By "AA should be merged with the parent's environment or not, with the former being the default", I'm assuming the "set" flag will mean "don't inherit".  What name do you have in mind?  Since we already have dontDoTheCloseThing, I think dontDoTheEnvironmentInheritThing would be good ;)
>>
>> I know it's bikeshedding, but negative flags are awful, we should come up with positive ones.  ignoreParentEnv?
>
> Now that the big pieces are seemingly falling into place, it is probably time for bikeshedding.  I was thinking clearEnv or newEnv, but ignoreParentEnv is perhaps more explicit.

clearEnv is somewhat incorrect, since you could specify a new environment via the env parameter, it won't be clear.
newEnv sounds ok, and probably is better than ignoreParentEnv to avoid a more verbose name, even if less descriptive.  At the very least, it will make people look it up ;)

>
> Speaking of negative flags, do you have better suggestions for the Config.noCloseStd... ones?

retainStdout

> dontDoTheCloseThing became inheritFDs, btw. :)  Also open for suggestions on that one.

That is fine with me.

-Steve

On Friday, 15 March 2013 at 13:57:10 UTC, Steven Schveighoffer wrote:
> On Thu, 14 Mar 2013 17:51:36 -0400, Lars T. Kyllingstad <public@kyllingen.net> wrote:
>
>> Speaking of negative flags, do you have better suggestions for the Config.noCloseStd... ones?
>
> retainStdout

Nice!

Sorry for the delay, but I've pushed a new version now.  There are still a few things I haven't done wrt. documentation* and unittests**, but the changes to the API and internals should be in place.

The biggest changes are that spawnProcess() now closes all non-std file descriptors on POSIX systems, and that it handles environment variables differently.  Specifically, they are now *merged* with the parent's environment by default, rather than replacing it.  In addition, there are some things that have been renamed, bugs that have been fixed, etc.

> Pull request:
> https://github.com/D-Programming-Language/phobos/pull/1151
>
> Code:
> https://github.com/kyllingstad/phobos/blob/std-process2/std/process2.d
>
> Documentation:
> http://www.kyllingen.net/code/std-process2/phobos-prerelease/std_process2.html


* I'm going to add a function table to the module introduction and flesh out the docs for some of the functions a bit.
** spawnShell() currently has no unittest, and I haven't enabled the "burn-in" test for Vladimir's escape*() functions yet.