On Monday, 25 February 2013 at 00:44:43 UTC, Steven Schveighoffer wrote:
> 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

fileDescriptor is 0.
The handle is 3. GetStdHandle(STD_INPUT_HANDLE) is also 3.

> 3. We have a valid handle, but for some reason SetHandleInformation fails.

Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?

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

> On Monday, 25 February 2013 at 00:44:43 UTC, Steven Schveighoffer wrote:
>> 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
>
> fileDescriptor is 0.
> The handle is 3. GetStdHandle(STD_INPUT_HANDLE) is also 3.
>
>> 3. We have a valid handle, but for some reason SetHandleInformation fails.
>
> Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?

I suppose that is possible.  By default the normal stdin is used, so maybe the OS makes the decision on inheritance based on whether it is used or not.  Clearly it works for some people...

Did you try SetHandleInformation directly on that handle?  It's still slightly possible that some other call caused the exception while this one was being thrown.

-Steve

On Monday, 25 February 2013 at 00:57:42 UTC, Vladimir Panteleev wrote:
> Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?

GetHandleInformation reveals that HANDLE_FLAG_INHERIT is already set for the stdin handle for me.

On Monday, 25 February 2013 at 01:09:32 UTC, Steven Schveighoffer
wrote:
> Did you try SetHandleInformation directly on that handle?  It's still slightly possible that some other call caused the exception while this one was being thrown.

I put a writeln in the if, so I'm quite sure that it was
SetHandleInformation that failed.

On Monday, 25 February 2013 at 01:10:08 UTC, Vladimir Panteleev wrote:
> On Monday, 25 February 2013 at 00:57:42 UTC, Vladimir Panteleev wrote:
>> Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?
>
> GetHandleInformation reveals that HANDLE_FLAG_INHERIT is already set for the stdin handle for me.

This fixes it for me:

http://dump.thecybershadow.net/2418951ca5eea369fb9f84d0514aa5e3/aoeu.patch

On Sun, 24 Feb 2013 20:15:02 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:

> On Monday, 25 February 2013 at 01:10:08 UTC, Vladimir Panteleev wrote:
>> On Monday, 25 February 2013 at 00:57:42 UTC, Vladimir Panteleev wrote:
>>> Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?
>>
>> GetHandleInformation reveals that HANDLE_FLAG_INHERIT is already set for the stdin handle for me.
>
> This fixes it for me:
>
> http://dump.thecybershadow.net/2418951ca5eea369fb9f84d0514aa5e3/aoeu.patch

OK, we'll make this change.

-Steve

On Sunday, 24 February 2013 at 21:04:45 UTC, Vladimir Panteleev 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.
>
> 1. Can the Firefox example be replaced with something else? Spawning a specific browser to open a webpage is bad practice, and I've noticed in several programs. It would be nice not to perpetuate it any further. There exists a proper way to open an URL in the default browser, already implemented as browse(url) in the current std.process.

Sure, I can think of another example.  But I wouldn't read too much into this one; it was never meant as a demonstration of the "correct" way to open a web page.   It was just a simple example of spawnProcess() usage that uses a cross-platform application everyone's heard of.

After all, you *could* argue this way about almost any kind of application which wasn't just invented for the sake of the example.  (In the last one, shouldn't we open the user's preferred word processor, etc?)


> 2. (Nitpick) The grep example uses a POSIX quoting syntax (single quotes). Would be better to use double quotes, or pass as array to avoid one more needless OS-specific element.

Actually, the quotes can just be removed altogether.  I believe this is an old example, BTW, from the module's infancy, when it was POSIX-only.  If anyone has a good idea for sample code which will be familiar to users of all platforms, please speak up.


> 3. The documentation for the "gui" config item seems to be wrong: it prevents the creation of a console, instead of causing it.

I've fixed it now.


> 4. I see that my command escaping functions have not made it through. I believe the matter has been discussed before, and I thought the consensus was to use them, although it's been a while. The function escapeShellCommand and its callees from the current std.process have the advantages that a) they come with a very thorough unit test, whereas std.process2's Windows escaping code does not have tests at all, and b) they are usable independently, which allows constructing scripts and batch files in D programs.

They were indeed supposed to be used in the new std.process, it just slipped my mind.  I don't have time to fix it right now, but I'll do it at some point.

Personally, I don't think they should be part of the public API.  They are inherently platform-specific, and we've tried to keep the module as platform-agnostic as possible.  Besides, they are not really usable with any of the other functions, and I am afraid it will be interpreted that way if we make them public.  How about we put them somewhere in the std.windows package?  (std.windows.util, for example?)


> 5. The spawnProcess versions which take a single string command simply call std.string.split on the command. I believe this approach to be fundamentally wrong, as passing an argument in quotes will not work as expected. Furthermore, on Windows (where process arguments are passed as a single string), splitting the string in spawnProcess and then putting it back together in spawnProcessWindows will result in a final command line that is different from the one supplied by the user.

The whole point was to avoid any kind of arcane platform-specific quoting and escaping rules.  If you have spaces inside your command line arguments, use the functions that take an array, and spawnProcess() will properly quote them.  If you have any other funky characters in there, don't worry about it, spawnProcess() will properly escape them.

The way it is now, the rules (if you can call it that) are exceedingly simple, and they are the same on all platforms.  This has the added benefit of discouraging platform-dependent client code.

Lars

On Monday, 25 February 2013 at 01:20:53 UTC, Steven Schveighoffer wrote:
> On Sun, 24 Feb 2013 20:15:02 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>
>> On Monday, 25 February 2013 at 01:10:08 UTC, Vladimir Panteleev wrote:
>>> On Monday, 25 February 2013 at 00:57:42 UTC, Vladimir Panteleev wrote:
>>>> Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?
>>>
>>> GetHandleInformation reveals that HANDLE_FLAG_INHERIT is already set for the stdin handle for me.
>>
>> This fixes it for me:
>>
>> http://dump.thecybershadow.net/2418951ca5eea369fb9f84d0514aa5e3/aoeu.patch
>
> OK, we'll make this change.

Done.  Thanks for tracking this down, guys!  Vladimir, could you just verify that it works with the code I pushed just now?

Lars

On Monday, 25 February 2013 at 00:15:21 UTC, Vladimir Panteleev wrote:
> 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.

Sorry, I have to get to work now, and I don't have time to answer your post properly.  I will say this, though:  You make a strong case about environment.opIndex(). :)

I'll think some more about it and write a proper reply later.

Lars

On Monday, 25 February 2013 at 06:46:32 UTC, Lars T. Kyllingstad wrote:
> On Monday, 25 February 2013 at 01:20:53 UTC, Steven Schveighoffer wrote:
>> On Sun, 24 Feb 2013 20:15:02 -0500, Vladimir Panteleev <vladimir@thecybershadow.net> wrote:
>>
>>> On Monday, 25 February 2013 at 01:10:08 UTC, Vladimir Panteleev wrote:
>>>> On Monday, 25 February 2013 at 00:57:42 UTC, Vladimir Panteleev wrote:
>>>>> Looks like it. Maybe you can't SetHandleInformation on standard handles in Windows 7?
>>>>
>>>> GetHandleInformation reveals that HANDLE_FLAG_INHERIT is already set for the stdin handle for me.
>>>
>>> This fixes it for me:
>>>
>>> http://dump.thecybershadow.net/2418951ca5eea369fb9f84d0514aa5e3/aoeu.patch
>>
>> OK, we'll make this change.
>
> Done.  Thanks for tracking this down, guys!  Vladimir, could you just verify that it works with the code I pushed just now?

Yep, works now.