Errors regarding `Cmd` interpolation with environment blocks

[staticfloat]

When interpolating in a Cmd literal, if you try to interpolate in a Cmd object with a non-default environment block, only the first interpolant is allowed to be inserted. Example:

julia> Cmd(`ffmpeg`; env=Dict("PATH" => "path1"))
setenv(`ffmpeg`,["PATH=path1"])

julia> ffmpeg = Cmd(`ffmpeg`; env=Dict("PATH" => "path1"))
setenv(`ffmpeg`,["PATH=path1"])

julia> shell = `/bin/bash`
`/bin/bash`

julia> run(`$(shell) -c $(ffmpeg)`)
ERROR: ArgumentError: Non-default environment behavior is only permitted for the first interpolant.
Stacktrace:
 [1] arg_gen(cmd::Cmd)
   @ Base ./cmd.jl:357
 [2] cmd_gen(parsed::Tuple{Tuple{Cmd}, Tuple{SubString{String}}, Tuple{Cmd}})
   @ Base ./cmd.jl:391
 [3] top-level scope
   @ REPL[11]:1

This is, I assume, because it is not entirely obvious what to do with the environment blocks of these Cmd objects. Do we merge them? If so, what precedence do we take? Personally, I think the most natural thing to do would be to merge them in left-to-right order and leave it up to the user to ensure there are no conflicts, but the code is complicated enough that I am willing to accept that this may not be easy.

The real kicker is that despite the error message, the true requirement is that only the first word of a Cmd may be a Cmd interpolant with an env block:

julia> shell = Cmd(`/bin/bash`; env=Dict("FOO" => "foofoo"))
       run(`sudo $(shell) -c echo \$FOO`)
ERROR: ArgumentError: Non-default environment behavior is only permitted for the first interpolant.
Stacktrace:
 [1] arg_gen(cmd::Cmd)
   @ Base ./cmd.jl:357
 [2] cmd_gen(parsed::Tuple{Tuple{SubString{String}}, Tuple{Cmd}, Tuple{SubString{String}}, Tuple{SubString{String}}, Tuple{SubString{String}}})
   @ Base ./cmd.jl:396
 [3] top-level scope
   @ REPL[6]:2

This is clearly a bug, and presents a usability issue for things like the new executable wrappers that BB provides. We want to move away from a withenv()-based system (due to thread-unsafety) and towards an addenv()-based system, where you get passed a Cmd block with the necessary PATH and LD_LIBRARY_PATH and other variables set already within it. However, if you needed to, say, call sudo before invoking your executable, you would have to use this shameful workaround:

julia> using UserNSSandbox_jll
       sandbox_cmd = sandbox()
       run(Cmd(`sudo $(sandbox_cmd.exec) --verbose --rootfs /tmp/rootfs /bin/bash`; env=sandbox_cmd.env))

Not quite as nice as simply:

julia> using UserNSSandbox_jll
       run(`sudo $(sandbox()) --verbose --rootfs /tmp/rootfs /bin/bash`)

In the short term, fixing the error would be good, but long-term I think it would be best if we remove this limitation altogether.

[staticfloat]

Assigning Stefan as I know he has had a hand in this area before, but feel free to re-assign.

[StefanKarpinski]

@staticfloat, do you still want to do this? If so, we should nail down the design and make a PR.

[staticfloat]

yeah, I do still want to do this. The design decision is to figure out how to merge the environments together. The usecase I have in mind is when we have multiple Cmd objects, each representing a command, and they get combined together due to something like sudo or lldb. E.g.

run(`$(sudo_cmd) $(lldb_cmd) -- $(target_exe)`)

In this case, each command may have had environment variables set within it, to alter its behavior. I'm totally okay with conflicts causing issues, and the user has to sort things out themselves to run properly. But I would rather have it have some kind of simple behavior that can work, rather than just throw an error immediately.

Ideally, there will be no overlap of environment variables and things should merge cleanly. But in the event that there are conflicts, I think the first command should take precedence, e.g. sudo_cmd's environment overrides keys with the same name within lldb_cmd, which itself overrides keys with the same name within target_cmd. This can be done by concatenating the environment blocks (such that sudo_cmd's environment block comes last), or by converting them to dictionaries and using merge() appropriately.

[vtjnash]

something like sudo

That particular example is not generally correct. The normal security policy for sudoers strips out all environment variables from all commands. Most however may be passed on the command line in the form of VAR=value. So just for this example, the correct form would be:

run(`sudo $(sandbox_cmd.env) $(sandbox_cmd.exec) --verbose --rootfs /tmp/rootfs /bin/bash`)

The same could be said to be true of other wrapper programs, such as ssh. So for even more cases, the program env - NAME=VALUE COMMAND may be needed.

[staticfloat]

Sure, but I consider these implementation details of that particular wrapper program. If I were designing an interface for running a wrapper program, I would expect that the environment variables get merged according to "simple" semantics, then require the user to work around complexities of sudo, ssh, etc... in general.

[vtjnash]

Yeah, just pointing out that the examples so far would do something very wrong, which is why this is an error in the first place. There are cases where this proposal should work (for example env and setsid), but that seems to be the exception (and both of those examples have trivial replacement functions in Julia)