Open sybrenstuvel opened 1 year ago
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.
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 runsgo 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
?
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:
This means that for every command to execute there are typically three lines of code necessary. My proposal is to introduce something like this:
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.