Remove command package and command.Runner in favor of simpler and stateless execext package

[bufdev]

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).

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Nov 1, 2024, 5:44 PM

[doriable]

Went through this, seems reasonable to me! Took a super quick look in core, the impact seems minimal, but will see if others have thoughts on this.

[pkwarren]

Such a nice cleanup to not have to pass around a Runner everywhere.

[pkwarren]

Any reason to prefer this over setting cmd.Stdin = nil (which will have it read from os.DevNull)?

[bufdev]

No there's not - good catch and we could change this. I am virtually certain there was a reason, given what I did with stdout and stderr, perhaps os/exec changed the behavior of the default stdin at some point in the last couple of years? I don't know, but I'll confirm that this is true back to 1.22 and then change.

[bufdev]

This was the case since at least 2014 it turns out.

I'm actually just going to keep this as-is, mostly because I'm terrified of changing it. I agree that there should be no difference to cmd.Stdin = nil, but I am somewhat sure I did this for a reason, I can't remember it, and changing it really worries me.

I'm adding a code comment to this effect so that we don't touch this in the future.

[bufdev]

Comment added.

[bufdev]

Comment for posterity:

// Note: This *should* be the same as just having cmd.Stdin = nil, given that
// exec.Cmd documents that Stdin has the same behavior as Stdout/Stderr, namely
// that os.DevNull is used. This has been the case for Stdin since at least 2014.
// However, way back when this package was first introduced, we set up the discardReader
// for some reason, and we can't remember why. We're terrified to change it, as there
// *may* have been some reason to do so. os.DevNull is actually just a string such
// as "/dev/null" on Unix systems, so how Golang actually handles this is somewhat
// unknown. Honestly, I might just want to change Stdout/Stderr to use a discardWriter.