icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
101 stars 13 forks source link

Slice.Tools JSON error #4043

Closed bernardnormier closed 1 week ago

bernardnormier commented 1 week ago

I am getting this error whenever I get a redefinition error in a Slice file:

    /Users/bernard/builds/icerpc-csharp/tools/IceRpc.Slice.Tools/IceRpc.Slice.Tools.targets(60,5): error : Error parsing JSON: 'F' is an invalid start of a value. LineNumber: 0 | BytePositionInLine: 0.. JSON content: Failed: Compilation failed with 3 error(s)
pepone commented 1 week ago

This is due to the compiler emitting non JSON diagnostics:

{"message":"redefinition of 'Locator'","severity":"error","span":{"start":{"row":20,"col":11},"end":{"row":20,"col":18},"file":"slice\\Ice\\Locator.slice"},"notes":[{"message":"'Locator' was previously defined here","span":{"start":{"row":14,"col":11},"end":{"row":14,"col":18},"file":"slice\\Ice\\Locator.slice"}}],"error_code":"E012"}
Failed: Compilation failed with 1 error(s)
externl commented 1 week ago

It used to be (I think) that JSON output was emitted to stdout and the filed summary was sent to stderr. IIRC rust does something similar. I thought that we used to only read from stdout, or at least not emit errors if JSON unmarshaling failed.

pepone commented 1 week ago

No quite correct.

We used to only emit JSON to STDERR for error messages. But now with --telemetry we also emit telemetry info as JSON formatted data to STDOUT.

I guess the simple is to ignore this non JSON messages from STDOUT as we did before, and just collect the STDOUT JSON messages that has the expected telemetry data.

InsertCreativityHere commented 1 week ago

that JSON output was emitted to stdout and the filed summary was sent to stderr

It's the other way around with slicec, the Compilation failed with ... message is sent to stdout, and all the errors are sent to stderr (either as JSON or text).

InsertCreativityHere commented 1 week ago

Another, maybe easier fix is to change slicec-cs to emit it's telemetry to stderr instead. That way stderr is purely JSON, and stdout is purely text.

And this would be a one-line change in slicec-cs to implement. Just change this: https://github.com/icerpc/icerpc-csharp/blob/b1be5453abe350afaff5338fc3e2b7eaf151bab4/tools/slicec-cs/src/main.rs#L74-L76 To eprintln instead.

pepone commented 1 week ago

Another, maybe easier fix is to change slicec-cs to emit it's telemetry to stderr instead.

Seems wrong to emit non error data to stderr

externl commented 1 week ago

Yeah, I'd rather just ignore non-JSON output in the tools.

InsertCreativityHere commented 1 week ago

Fair enough, I agree it'd be weirder, was just pitching ideas!