Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose libuv windows verbatim and hidden process spawning #13780

Merged
merged 1 commit into from
Nov 24, 2015

Conversation

stevengj
Copy link
Member

This fixes #13776 by exporting a windows_verbatim option to Cmd(cmd; keywords...) to enable verbatim arguments when spawning windows commands, for processes that don't parse embedded quotes.

I also noticed another potentially useful libuv process flag on windows, which I exported as a windows_hide flag in Cmd. This allows you to spawn Windows processes without popping up a console window, which is obviously useful for GUI programs etc.

`cmd` will normally create a new console window to display its output.
The `windows_hide` function suppresses that console window.
"""
windows_hide(cmd::Cmd) = Cmd(cmd; flags = cmd.flags | UV_PROCESS_WINDOWS_HIDE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than exposing this as a new function, I was considering making this an official API for Cmd:

Cmd(cmd; detach = false, windows_hide = true)

thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. Deprecate the old detach API, or keep it around since detaching is pretty common?

@stevengj
Copy link
Member Author

Okay, updated so that you now do Cmd(cmd; windows_verbatim=true, windows_hide=false) etcetera.

Do we want to deprecate detach, ignorestatus, and setenv in favor of Cmd(cmd; detach=..., ignorestatus=..., env=...)?

@@ -1316,6 +1316,8 @@ export
setenv,
spawn,
success,
windows_hide,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer a function so shouldn't be exported?

@stevengj stevengj force-pushed the windows_cmd_flags branch 3 times, most recently from 43f24c3 to 1806690 Compare October 28, 2015 18:21
immutable Cmd <: AbstractCmd
exec::Vector{ByteString}
ignorestatus::Bool
detach::Bool
flags::UInt8 # libuv process flags
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why UInt8, when it's int in jl_uv.c?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there are < 8 bits of flags, why store more?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

today. will there always be < 8 bits of flags? not very future proof to pick the smallest possible number

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change it to support additional flags at some distant future time when libuv uses more than 8 bits of flags, we can always change the flags variable type.

Unless there is some reason that we don't want sizeof(Cmd) to ever change?

This is not like a C library where there is an ABI stability issue, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is potentially subtly breaking to change sizes of internal fields. People generally shouldn't be messing with internal fields, but I still lean towards being a bit more future proof than worrying about conserving 3 bytes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I changed it. I think it's a bit pointless to worry about ABI stability in this context, but 3 bytes are not worth arguing about.

@stevengj
Copy link
Member Author

stevengj commented Nov 4, 2015

Okay to merge?

new console window is displayed when the `Cmd` is executed. This has
no effect if a console is already open or on non-Windows systems.
* `env`: Set environment variables to use when running the `Cmd`. `env`
is either a dictionary mapping strings to strings, an array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing's a little odd here

looks ok otherwise

vtjnash added a commit that referenced this pull request Nov 24, 2015
expose libuv windows verbatim and hidden process spawning
@vtjnash vtjnash merged commit 003f415 into JuliaLang:master Nov 24, 2015
@vtjnash
Copy link
Member

vtjnash commented Nov 24, 2015

it should be good to have this exposed. we can develop it further on master if anyone runs into problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

need Julia API for UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS
3 participants