ExecForEach().Error() doesn't execute

[philippgille]

Hello, I'm currently trying to migrate from some build.sh and build.ps1 files to a magefile using script.

Maybe I misunderstood the purpose of Error(), but when I don't need the output of a command (as String() would return), I thought Error() would basically ignore the output, but still return the error. But it seems that when chained after an ExecForEach, the commands in that are not executed.

For example, let's say we have files coverage.txt and foo/coverage.txt and I want to remove them. Then the following works:

	_, err := script.FindFiles(".").Match("coverage.txt").ExecForEach("rm ./{{.}}").String()
	return err

But the following doesn't:

	return script.FindFiles(".").Match("coverage.txt").ExecForEach("rm ./{{.}}").Error()

Is that expected?

[bitfield]

Hey @philippgille, thanks for the report!

Don't forget that all filters run concurrently. So calling ExecForEach(...) starts that process happening in the background (the equivalent of running it with & in the shell).

If you immediately call Error, then you'll get the current error status of the pipe, which is probably nil, and you won't then have any way of getting the results of the ExecForEach, if you want them. Indeed, if the program exits, that process probably won't have a chance to run at all.

You could write instead something like this:

p := script.FindFiles(".").Match("coverage.txt").ExecForEach("rm ./{{.}}")
p.Wait()
return p.Error()

[philippgille]

Thanks for the super quick reply!

Ah I see, Error() is not a sink as the ones listed here, which is why it doesn't wait for the concurrent operations to finish.

I'd like to propose two things:

1. Maybe the Error() Godoc can be extended a bit to make this more clear, like to specifically say that it's not a sink that waits for the pipe to be finished
2. Add another sink that combines the Wait() and Error() behavior
   • I can imagine the use case to wait but not dismiss an error is fairly common, but changing Wait() to this behavior is probably out of the question? Or maybe it's acceptable because code using it so far assumes no return value, and introducing a return value won't break that code.
   • Or Wait() could be kept as is, WaitError() could be added, and potentially for consistency WaitIgnore() could also be added.

The great thing about script is its conciseness, and being able to wait and get any errors in one line would be great!

If you agree, I could work on a PR.

[bcho]

Yep, it would be great to have a Wait method with returning error. This can allow us to write in one line:

if err := script.Exec("sleep 10").WaitError(); err != nil {
  return err
}

[bitfield]

I like this idea 😁