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 😁

[mahadzaryab1]

@bitfield do you need someone to work on this? I would be happy to contribute

[bitfield]

Sure, go right ahead!

[mahadzaryab1]

@bitfield PR for this issue is ready for review at #211

[bitfield]

@mahadzaryab1 the PR is great, but I wonder if we can really justify having two more or less identical methods, one that returns the error and one that doesn't. To play devil's advocate for a moment, if we already had WaitError, what would be the point of adding Wait?

After all, if Wait returned error, you'd be free to ignore it, as presumably existing programs do. But if you want the error value, you could receive it. Would it make more sense to have Wait return error instead?

[mahadzaryab1]

@bitfield Sounds good to me! I'll make the change.

[mahadzaryab1]

@bitfield updated #211 to have Wait return error instead.

[bitfield]

@philippgille and @bcho, will this solution work for you?

[philippgille]

That's great, thank you! 👍