This PR does a longstanding cleanup that has been on my local computer for a while and just needed finishing. This removes the command package in favor of a much-simpler execext package that wraps os/exec with the safety checks we care about.
This gets rid of the stateful command.Runner type, which kept track of the number of running processes it created, and set a hard limit on these, usually based on thread. This was overkill; in places where we perform parallelism, we already use thread.Parallelize, and adding an additional semaphore to block within command had no practical use. The downside was that we had to pass the command.Runner type everywhere, which really cluttered our code, including in some important packages (such as diff) where the fact that we had to pass command.Runner exposed an ugly implementation detail.
This also:
Changes execext.WithEnv (formerly command.RunWithEnv) to take a []string instead of a map[string]string, as []string is what the stdlib uses for environment variables. The practical effect for us (using app.EnvContainers) is to use app.Environ(container) instead of app.EnvironMap(container) when calling execext.WithEnv.
Removes RunStdout, a convenience function that used a app.EnvStdioContainer to return the value of stdout as a byte slice. This depended on the app package, which is not in the stdlib, and on balance, it's nicer to have execext just be stdlib-only as opposed to it depending on something custom. We only used RunStdout in a couple of places.
The impact on core should be relatively minimal, as mostly the command package was just used to call command.NewRunner for testing. However, the port is usually just this:
Remove any command.NewRunner calls.
Remove any command.Runner parameters.
Replace command.Runner.Run calls with execext.Run.
Replace command.RunWith.* with execext.With.
Use []string instead of map[string]string for execext.WithEnv, usually by replacing app.EnvironMap(container) with app.Environ(container).
This PR does a longstanding cleanup that has been on my local computer for a while and just needed finishing. This removes the
command
package in favor of a much-simplerexecext
package that wrapsos/exec
with the safety checks we care about.This gets rid of the stateful
command.Runner
type, which kept track of the number of running processes it created, and set a hard limit on these, usually based onthread
. This was overkill; in places where we perform parallelism, we already usethread.Parallelize
, and adding an additional semaphore to block withincommand
had no practical use. The downside was that we had to pass thecommand.Runner
type everywhere, which really cluttered our code, including in some important packages (such asdiff
) where the fact that we had to passcommand.Runner
exposed an ugly implementation detail.This also:
execext.WithEnv
(formerlycommand.RunWithEnv
) to take a[]string
instead of amap[string]string
, as[]string
is what the stdlib uses for environment variables. The practical effect for us (usingapp.EnvContainers
) is to useapp.Environ(container)
instead ofapp.EnvironMap(container)
when callingexecext.WithEnv
.RunStdout
, a convenience function that used aapp.EnvStdioContainer
to return the value of stdout as a byte slice. This depended on theapp
package, which is not in the stdlib, and on balance, it's nicer to haveexecext
just be stdlib-only as opposed to it depending on something custom. We only usedRunStdout
in a couple of places.The impact on core should be relatively minimal, as mostly the
command
package was just used to callcommand.NewRunner
for testing. However, the port is usually just this:command.NewRunner
calls.command.Runner
parameters.command.Runner.Run
calls withexecext.Run
.command.RunWith.*
withexecext.With
.[]string
instead ofmap[string]string
forexecext.WithEnv
, usually by replacingapp.EnvironMap(container)
withapp.Environ(container)
.