Feat: output STDERR from the building process

[beta-ziliani]

Solves #535 by outputting the STDERR of the crystal build to the STDERR of the shards process only if the build is successful. This allows to notice deprecation warnings and the like.

In order to test it, I had to concatenate the STDOUT and STDERR, which isn't as clean as possible but works.

[luislavena]

Hello @beta-ziliani, was wondering why not simply inherit error too? That way, you only need to check for success of process status and raise the single error (shown at the bottom) after all the error has been automatically output to you, no need to check if there were errors collected (IO::Memory) and then send that to STDERR later.

I think would be simpler code, but maybe I'm missing what you're trying to achieve as DX.

Cheers.

[straight-shoota]

Currently, in case of failure the captured STDERR is part of the error message.
If we just pass STDERR through, the output from crystal build would be printed independent of the error and not be part of the error message.
I think it's better to keep it as part of the error message for consistency. That requires capturing and reprinting, but it's not a big deal.

[luislavena]

So in the case of warning:

❯ shards build
Dependencies are satisfied
Building: d
In d.cr:9:1

 9 | a
     ^
Warning: Deprecated top-level a.

A total of 1 warnings were found.

In the case of errors:

❯ shards build
Dependencies are satisfied
Building: d
Error target d failed to compile:
Showing last frame. Use --error-trace for full trace.

In d.cr:3:3

 3 | foo
     ^--
Error: undefined local variable or method 'foo' for top-level

Thought since error is being displayed multiple times, the message about the failed target got lost in the middle, instead of something like:

❯ shards build
Dependencies are satisfied
Building: d
Showing last frame. Use --error-trace for full trace.

In d.cr:3:3

 3 | foo
     ^--
Error: undefined local variable or method 'foo' for top-level
Target d failed to compile

But what you propose makes sense, to make a section in the output with the error. 👍🏽

Cheers.

[straight-shoota]

We could certainly change the order in the error message! That's not the point.

I'm concerned about maintaining that the message of the exception contains all the error information.

[luislavena]

I'm concerned about maintaining that the message of the exception contains all the error information.

Not sure I follow your concern on that, from what I understand is that Crystal outputs the error message entirely to STDERR, so the error is captured with the current approach.

$ crystal build d.cr --error-trace 
In d.cr:9:1

 9 | a
     ^
Warning: Deprecated top-level a.

A total of 1 warnings were found.

$ crystal build d.cr --error-trace 2>/dev/null

$ echo $?
0

Same for errors:

$ crystal build d.cr --error-trace 
In d.cr:10:1

 10 | foo
      ^--
Error: undefined local variable or method 'foo' for top-level

$ crystal build d.cr --error-trace 2>/dev/null

$ echo $?
1

Thank you.

[straight-shoota]

@luislavena I meant that the error output from crystal build is contained in the Error exception.