Print build errors to stderr

[bew]

This addresses #4451

There are some cases which needs discussion about where to prints them:

• usages messages on missing CLI argument
• build errors when output format is JSON

[mverzilli]

LGTM! I think the two cases you mention should also print to stderr. Why would you send them to stdout?

[bew]

About the JSON formatted errors, I'm thinking about the use-case of using shell commands:

$ crystal build --format json file.cr | use_the_json_output

Once the JSON errors will be on stderr, they'll have a hard time getting the correct stream to go in the pipe to use_the_json_output program.

But maybe it's not our business, and it should stay on stderr, as most users will use their language's stdlib to spawn a process and granuraly get it's stdout/stderr programmatically.

[bew]

I fixed the specs, but now I don't know why the build doesn't work, did I broke something else? https://travis-ci.org/crystal-lang/crystal/jobs/240012478

[Sija]

@bew so far, so (green) good... :)

[RX14]

The travis build fails so often on osx, why don't we just disable it or ignore the results and use circleci only?

[bew]

Rebased, osx still failing, are we waiting for #4528 to be merged? Or can we merge this PR soon?

[Sija]

Looks like GTG, merge-time?

[akzhan]

I'm unsure to write errors to STDERR in JSON output case.

Better to choose output stream by output format. Errors to JSON to STDOUT is ok. Isn't it?

[Sija]

@akzhan I'd expect to see errors on STDERR on non-success process exit...

[bew]

@akzhan I believe that an error message should always go to stderr, whatever the output format is.

[akzhan]

@Sija @bew Ok.

[RX14]

Looks good to me, would be nice if we could spec this behaviour but i'm not really opposed to landing this without specs.

[mverzilli]

Restarted the build (I know, I know, we'll tackle that soon :)).

[RX14]

@mverzilli this PR is from may so I guess it doesn't have the .travis.yml merged so that OSX is an allowed failure.

[bew]

@RX14 You're right, I've rebased the branch on current master it should be good now.

[bew]

I've found another place where output should go on STDERR:

diff --git a/src/spec.cr b/src/spec.cr
index 2c3c303a5..aa49cdbcf 100644
--- a/src/spec.cr
+++ b/src/spec.cr
@@ -83,8 +83,8 @@ OptionParser.parse! do |opts|
     if location =~ /\A(.+?)\:(\d+)\Z/
       Spec.add_location $1, $2.to_i
     else
-      puts "location #{location} must be file:line"
-      exit
+      STDERR.puts "location #{location} must be file:line"
+      exit 1
     end
   end
   opts.on("--junit_output OUTPUT_DIR", "generate JUnit XML output") do |output_dir|
@@ -106,7 +106,7 @@ OptionParser.parse! do |opts|
 end

 unless ARGV.empty?
-  puts "Error: unknown argument '#{ARGV.first}'"
+  STDERR.puts "Error: unknown argument '#{ARGV.first}'"
   exit 1
 end

Should I add this change in this PR ? (it's not a build error, but it's still an error)

[Sija]

@bew IMHO it's an addition in line with this PR, so yes, I'd add it too.

[RX14]

@bew that's not actually build errors though is it? I'm fine with modifying the spec runner in this PR but the commit should be renamed to "Print build and spec errors to stderr".

[bew]

Indeed it's not a build error, that's why I asked first.

I'll make another PR later today (or someone else can if he wants to!) for the spec runners, as it was not in the scope of this PR to check every possible print/puts.

[RX14]

@bew so, is this ready to merge?

[bew]

If you all agree, yes I think so.

[mverzilli]

Great job @bew! Thank you ❤️