On Sunday, 24 February 2013 at 21:40:24 UTC, Andrej Mitrovic wrote:
> On 2/24/13, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>> a) they come with a very thorough unit test
>
> Unfortunately those unittests are never being run in the Phobos
> test-suite and as a result a regression was missed (which was fixed by
> now):
> http://d.puremagic.com/issues/show_bug.cgi?id=9309
>
> The report for the unittests:
> http://d.puremagic.com/issues/show_bug.cgi?id=9310

Hmm, that's unfortunate. Some of that function's callees are tested, but not the function itself.

The reason why unittest_burnin is not set in Phobos is that 1) it requires a helper program to be compiled beforehand, and 2) it runs indefinitely with random variations (hence its name).

I suppose a simpler additional unit test would be appropriate.
https://github.com/D-Programming-Language/phobos/pull/1161

On Sunday, 24 February 2013 at 22:13:35 UTC, Steven Schveighoffer wrote:
> On Sun, 24 Feb 2013 16:04:43 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>
>> On Sunday, 24 February 2013 at 17:41:44 UTC, Lars T. Kyllingstad wrote:
>>> Ok, a new version with non-blocking wait is up.
>>
>> 3. The documentation for the "gui" config item seems to be wrong: it prevents the creation of a console, instead of causing it.
>
> It means 'use gui mode' which means, don't create a console.  I don't consider a console window a gui.

Sorry, I think you misunderstood.

Currently, the documentation says:

"On Windows, this option causes the process to run in a console window."

However, when the flag is PRESENT, then the console window is SUPPRESSED (see line 522). The documentation's meaning is reversed.

> No, snn.lib included with the compilers for a few versions has been patched.  The exeception you would get would be different, that appears to be coming from std.process, even though the file name seems to be std.stdio.

OK... then I guess it doesn't work for me. Does it work for anyone else on Windows?

Full code of my ls test program:

import std.stdio;
import std.process2;

void main()
{
	string[] files;
	auto p = pipe();

	auto pid = spawnProcess("ls", stdin, p.writeEnd);
	scope(exit) wait(pid);

	foreach (f; p.readEnd.byLine())  files ~= f.idup;
}

On 2/25/13, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
> Full code of my ls test program:

Works for me on XP. Using 'ls' from GnuWin32.

On Sun, 24 Feb 2013 18:52:04 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Sunday, 24 February 2013 at 22:13:35 UTC, Steven Schveighoffer wrote:
>> On Sun, 24 Feb 2013 16:04:43 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>>
>>> On Sunday, 24 February 2013 at 17:41:44 UTC, Lars T. Kyllingstad wrote:
>>>> Ok, a new version with non-blocking wait is up.
>>>
>>> 3. The documentation for the "gui" config item seems to be wrong: it prevents the creation of a console, instead of causing it.
>>
>> It means 'use gui mode' which means, don't create a console.  I don't consider a console window a gui.
>
> Sorry, I think you misunderstood.
>
> Currently, the documentation says:
>
> "On Windows, this option causes the process to run in a console window."
>
> However, when the flag is PRESENT, then the console window is SUPPRESSED (see line 522). The documentation's meaning is reversed.

Yes, you are right.  It needs to be fixed.  Thanks.

>
>> No, snn.lib included with the compilers for a few versions has been patched.  The exeception you would get would be different, that appears to be coming from std.process, even though the file name seems to be std.stdio.
>
> OK... then I guess it doesn't work for me. Does it work for anyone else on Windows?
>
> Full code of my ls test program:
>
> import std.stdio;
> import std.process2;
>
> void main()
> {
> 	string[] files;
> 	auto p = pipe();
>
> 	auto pid = spawnProcess("ls", stdin, p.writeEnd);
> 	scope(exit) wait(pid);
>
> 	foreach (f; p.readEnd.byLine())  files ~= f.idup;
> }

Hm... that message is printed out if the code cannot set the inherit handle flag on the specific stdin.

Are you on windows 64 or 32?  It's a large difference since one uses MSVCRT and one uses DMCRT.  Also, I don't have windows 64, so I can't verify this if that's the case :)

-Steve

On Sunday, 24 February 2013 at 14:43:50 UTC, Lars T. Kyllingstad wrote:
> [snip]

Hi Lars,

First of all, about environment. I think the old behavior makes more sense.

I think you had a good point about making it behave like an associative array. I would expect using opIndex with an inexisting key to throw. Subtle deviations of behavior for types that generally behave like well-known types can introduce latent bugs.

The danger is even more potent in the case of environment variables, as those are often used for constructing command-lines and such. If attempting to get the value of an inexisting variable now returns null, which is used to build a command line, unexpected things can happen.

For example, let's say that you're writing a program for analyzing malware, which expects $BINDUMP to be set to the path of some analysis tool. So it runs environment["BINDUMP"] ~ args[1] - however, if BINDUMP is unset, the program runs the malware executable itself.

For another example, here's this classic catastrophic bug in shell scripts:

rm -rf $FOO/$BAR

What happens if $FOO and $BAR are unset?

One thing that I think is missing from the environment object is opIn_r. Implementing opIn_r would allow users to more safely explicitly check if a variable is set or not, and is more readable than environment.get("FOO", null).

And of course, there's the issue of people migrating code from the old module version to the new one: if they relied on the old behavior, the code can break in unexpected ways after the migration.

What are your specific reasons for changing environment's behavior?

Speaking of shells, I noticed you hardcode cmd.exe in std.process2. That's another bug, it should look at the COMSPEC variable.

Also, about the shell function, I noticed this recent bug report:
http://d.puremagic.com/issues/show_bug.cgi?id=9444

Maybe it somehow makes the transition to the new function easier? :)

If not, since you're adamant about not changing the name, can we overload the function (e.g. make the new one return some results in "out" parameters), and deprecate the original overload?

Finally, I'd just like to sum up that we seem to have two decisions on the scales: somehow solving the API incompatibilities, or introducing the new version as an entirely new module. The latter is a mess we really don't want to get into, so it'd need to be justified, and IMHO the incompatibilities don't seem to be as severe and unresolvable to warrant that mess.

On Monday, 25 February 2013 at 00:02:54 UTC, Steven Schveighoffer wrote:
> Hm... that message is printed out if the code cannot set the inherit handle flag on the specific stdin.
>
> Are you on windows 64 or 32?  It's a large difference since one uses MSVCRT and one uses DMCRT.  Also, I don't have windows 64, so I can't verify this if that's the case :)

I'm getting the same exception with DMD64 and DMD32.

On Sun, 24 Feb 2013 19:17:44 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Monday, 25 February 2013 at 00:02:54 UTC, Steven Schveighoffer wrote:
>> Hm... that message is printed out if the code cannot set the inherit handle flag on the specific stdin.
>>
>> Are you on windows 64 or 32?  It's a large difference since one uses MSVCRT and one uses DMCRT.  Also, I don't have windows 64, so I can't verify this if that's the case :)
>
> I'm getting the same exception with DMD64 and DMD32.

Are you running from a console?  If not, I think I see where the issue is.

-Steve

On Monday, 25 February 2013 at 00:27:10 UTC, Steven Schveighoffer wrote:
> On Sun, 24 Feb 2013 19:17:44 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>
>> On Monday, 25 February 2013 at 00:02:54 UTC, Steven Schveighoffer wrote:
>>> Hm... that message is printed out if the code cannot set the inherit handle flag on the specific stdin.
>>>
>>> Are you on windows 64 or 32?  It's a large difference since one uses MSVCRT and one uses DMCRT.  Also, I don't have windows 64, so I can't verify this if that's the case :)
>>
>> I'm getting the same exception with DMD64 and DMD32.
>
> Are you running from a console?  If not, I think I see where the issue is.

I am running from a console, and I don't think this would make any difference. Maybe you intended to ask if I'm linking with /SUBSYSTEM:WINDOWS? (I'm not)

On Sun, 24 Feb 2013 19:35:36 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Monday, 25 February 2013 at 00:27:10 UTC, Steven Schveighoffer wrote:
>> On Sun, 24 Feb 2013 19:17:44 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>>
>>> On Monday, 25 February 2013 at 00:02:54 UTC, Steven Schveighoffer wrote:
>>>> Hm... that message is printed out if the code cannot set the inherit handle flag on the specific stdin.
>>>>
>>>> Are you on windows 64 or 32?  It's a large difference since one uses MSVCRT and one uses DMCRT.  Also, I don't have windows 64, so I can't verify this if that's the case :)
>>>
>>> I'm getting the same exception with DMD64 and DMD32.
>>
>> Are you running from a console?  If not, I think I see where the issue is.
>
> I am running from a console, and I don't think this would make any difference. Maybe you intended to ask if I'm linking with /SUBSYSTEM:WINDOWS? (I'm not)

So here is the code that is throwing that exception:

 static void prepareStream(ref File file, DWORD stdHandle, string which,
                              out int fileDescriptor, out HANDLE handle)
    {
        fileDescriptor = _fileno(file.getFP());
        if (fileDescriptor < 0) handle = GetStdHandle(stdHandle);
        else
        {
            version (DMC_RUNTIME) handle = _fdToHandle(fileDescriptor);
            else /* MSVCRT */ handle = _get_osfhandle(fileDescriptor);
        }
        if (!SetHandleInformation(handle, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT))
        {
            throw new StdioException(
                "Failed to pass "~which~" stream to child process", 0);
        }
    }

Called like this:

    prepareStream(stdin_, STD_INPUT_HANDLE, "stdin" , stdinFD, startinfo.hStdInput );

Since "stdin" is what is in the exception.

It looks like you are passing stdin as the handle for stdin.  From the above, the ways this exception could fail are:

1. The file descriptor from stdin failed to come out, and windows gives back a valid handle from GetStdHandle
2. The file descriptor is valid (0 or above), but _fdToHandle/_get_osfhandle fails to get a valid handle
3. We have a valid handle, but for some reason SetHandleInformation fails.

I'm guessing that since you are running with a normal subsystem, with a console, you have a valid handle.  So my guess would be that SetHandleInformation is failing.

Can you catch the exception and print out GetLastError()?

-Steve

On Monday, 25 February 2013 at 00:44:43 UTC, Steven Schveighoffer wrote:
> Can you catch the exception and print out GetLastError()?

87 (ERROR_INVALID_PARAMETER): The parameter is incorrect.

Consider using FileException, which calls sysErrorString.