Enhancement: Simpler way to execute multiple commands

[sybrenstuvel]

Describe the feature
This is a proposal to simplify the execution of multiple commands, where an error should stop this chain. Basically the behaviour of make when executing multiple commands for a single target.

What problem does this feature address?
AFAIK this is the way to execute multiple commands in a "build" target:

func Build() error {
	if err := sh.Run("go", "mod", "download"); err != nil {
		return err
	}
	if err := sh.Run("go", "build", "./cmd/gcoderamp"); err != nil {
		return err
	}
	return sh.Run("go", "build", "./cmd/pcb2gcodewrap")
}

This means that for every command to execute there are typically three lines of code necessary. My proposal is to introduce something like this:

func Build(ctx context.Context) error {
	r := NewRunner(ctx)
	r.Run("go", "mod", "download")
	r.Run("go", "build", "./cmd/gcoderamp")
	r.Run("go", "build", "./cmd/pcb2gcodewrap")
	return r.Wait()
}

I think it's a good idea to make running shell commands as direct as possible.

Additional context
This is the way I implemented it locally. It does add the dependency on golang.org/x/sync/errgroup, so the code cannot be used as-is. I think it's relatively simple to restructure the code to avoid this dependency. Before doing that, I'd rather discuss the overall idea first, though.

//go:build mage

package main

import (
	"context"

	"github.com/magefile/mage/sh"
	"golang.org/x/sync/errgroup"
)

// Runner allows running a group of commands sequentially, stopping at the first
// failure.
type Runner struct {
	group *errgroup.Group
	ctx   context.Context
}

// NewRunner constructs a new runner that's bound to the given context. If the
// context is done, no new command will be executed. It does NOT abort an
// already-running command.
func NewRunner(ctx context.Context) *Runner {
	group, groupctx := errgroup.WithContext(ctx)
	group.SetLimit(1)

	return &Runner{
		group: group,
		ctx:   groupctx,
	}
}

// Run the given command.
// This only runs a command if no previous command has failed yet.
func (r *Runner) Run(cmd string, args ...string) {
	r.group.Go(func() error {
		if err := r.ctx.Err(); err != nil {
			return err
		}
		return sh.Run(cmd, args...)
	})
}

// Wait for the commands to finish running, and return any error.
func (r *Runner) Wait() error {
	return r.group.Wait()
}

[natefinch]

So, I am sympathetic to the view that writing multiple command line commands in mage is a little wordy. However, I don't think this is the right fix. One problem is that you can't know what has succeeded and what has failed. So you won't know what needs to be cleaned up afterward. Second is that the implementation uses errgroup.Group.Go, which runs the code in a goroutine, which is probably not what most people would expect. Most people would expect the code to be run sequentially. For example, in your code, I think most people would expect go mod download to finish before mage runs go build.

My suggestion would be to use a mechanism like mg.Deps uses, where instead of returning an error, it panics with a special value, and the rest of the mage infrastructure just knows how to deal with that panic value, and converts it into an error message for the user.

so, for example:

func Build(ctx context.Context) {
    sh2.Run("go", "mod", "download")
    sh2.Run("go", "build", "./cmd/gcoderamp")

    // if we get here, we know the go build step completed,
    // so add a cleanup step once we're done.
    defer os.Remove("./cmd/gcoderamp")

    sh2.Run("go", "build", "./cmd/pcb2gcodewrap")

    // no need for a separate return call here, since 
    // the above would panic if they errored.
}

I'll have to think some more about it. It's doable, but it needs some thought to make sure we cover all the bases.

[sybrenstuvel]

So, I am sympathetic to the view that writing multiple command line commands in mage is a little wordy. However, I don't think this is the right fix. One problem is that you can't know what has succeeded and what has failed. So you won't know what needs to be cleaned up afterward.

You can, this is an example of a failed build:

$ mage build
# gitlab.com/dr.sybren/gcodetool
.\gcodetool.go:10:1: syntax error: non-declaration statement outside function body
Error: running "go build ./cmd/gcoderamp" failed with exit code 2

As you can see, it shows exactly what's going wrong.

Second is that the implementation uses errgroup.Group.Go, which runs the code in a goroutine, which is probably not what most people would expect. Most people would expect the code to be run sequentially. For example, in your code, I think most people would expect go mod download to finish before mage runs go build.

Thanks to the group.SetLimit(1) call, this does run sequentially.

My suggestion would be to use a mechanism like mg.Deps uses, where instead of returning an error, it panics with a special value, and the rest of the mage infrastructure just knows how to deal with that panic value, and converts it into an error message for the user.

This sounds rather elaborate to me, and goes against my personal 'don't use panic for actual program logic' approach. However, I do like your example, as it's really clean & simple. Which was the whole point of this exercise.

I'll have to think some more about it. It's doable, but it needs some thought to make sure we cover all the bases.

Yeah, that's why I wanted to discuss before spending more time on the actual code itself ;-)

---

Going slightly off-topic, this is some code I also added to my own magefiles/clean.go:

func rm(path ...string) error {
	for _, p := range path {
		if err := sh.Rm(p); err != nil {
			return err
		}
	}
	return nil
}

Maybe sh.Rm() could get extended in the same way, and become func Rm(path ...string) error?