Repository navigation
expose windows DETACHED_PROCESS flag #32
Description
Activity
- added a commit that references this issue
on May 27, 2015 Joey, I don't have my windows machine set up for building; could you take it from here?
I don't think it makes sense to use setsid on Unix; I wouldn't think of detach_from_console as necessarily implying that. If we want to have something like that, a separate field would make sense.
Like #13, this will be a breaking change. This is making me itch towards having a release that hides the
CreateProcessconstructor, but the breakage from a change like that is likely too high to tolerate. So instead, let's just plan on having a 1.3 major version bump for both this and #13 (pinging @proger).It's not a perfect match, but it's much the same as a controlling terminal. A process with a console or a controlling terminal can be interactive, a process without one cannot. It's possible that there's a better way to implement it than using setsid though.
A non-API breaking alternative would be to set DETACHED_PROCESS when output, input, and error are all redirected, as is currently done with CREATE_NO_WINDOW. Thinking being that, if the process is not being allowed to talk to the console, it could as well be fully detached. However, I don't know enough about Windows to be sure that would always be safe.
I'm also on the fence about setsid. It's broadly the same idea. It may not match exactly, but that's probably true of a lot of things this library abstracts over between windows and unix. There's already a way to setsid the current process in haskell, but there is value in being able to spawn a command and setsid it. On balance, I'm in favor of making the unix side call setsid. If the user doesn't want that, they can avoid using detach_from_console on unix after all.
On the other hand, if we have a separate setsid flag, then the user has trivial control over what will happen without needing any kind of conditional compilation.
I'll review the PR, and most likely ping the -cafe about breaking changes before merging.
Michael Snoyman wrote:
On the other hand, if we have a separate setsid flag, then the user has trivial
control over what will happen without needing any kind of conditional
compilation.I can only provide the data point that in my use case, setsid and
DETACHED_PROCESS behave similarly enough that I want them both on their
respective platforms.Also, I did some testing of setsid vs DETACHED_PROCESS behavior when
various FDs are left connected to the console, etc, and they seemed to
behave quite similarly.see shy jo
- added a commit that references this issue
on Aug 18, 2015 I went ahead and split it into two flags; I don't quite see the need, but it's no big deal either way.
Thanks @db48x. I've merged this onto the new-flags branch, and have added some sanity checks. However, when I try to run this on Windows, I get:
Running test: detach_console test.exe: echo: createProcess: invalid argument (Invalid argument)Can you test if this occurs for you too?
I see that too; create_new_console also fails if the detach_console test is commented out. new_session succeeds, however.
- added 2 commits that reference this issue
on Aug 24, 2015 Well yes. That's the commit that adds the relevant code to the test suite.
Reverting that just hides the problem.On Mon, Aug 24, 2015, 3:17 AM Joey Hess [email protected] wrote:
The problem commit seems to be 431379b
431379bI reverted that commit, and the test suite passes (once it's fixed to not
test create_new_console).—
Reply to this email directly or view it on GitHub
#32 (comment).He must have pasted the wrong commit id, since he edited the message to include a different one.
db48x's fix still doesn't fix it. 0xF = 15, we want 16, so 0x10.
I've confirmed that 0x10 fixes it
I was just coming to that... I'm an idiot. Thanks.
Merged to master.
Yea, I noticed that about 2 seconds after I committed it, hence the amended commit.
To be fair, I made the mistake in the first place
If this were just C I'd make the constants be 1<<0, 1<<1, 1<<2, etc, as this eliminates this type of mistake. Is there something similar we can do here?
On Windows the DETACHED_PROCESS flag lets the process detached from the terminal. I need this to run ssh with SSH_ASKPASS, which only works when it has no controlling console. So, could this be added as a new field to CreateProcess, perhaps called detach_from_console?
I needed to set both DETACHED_PROCESS and CREATE_NO_WINDOW to get the desired result. (Otherwise, windows pops up a new console window.)
It might make sense to have the unix implementation of detach_from_console call setsid.
If this plan seems to make sense, I'll cook up a patch, since I need this functionality.