fix(src/build): output child process stderr to console.error #1998
Conversation
| return new Transform({ | ||
| // transform will be called whenever the upstream source pushes data to us | ||
| transform(chunk, encoding, done) { | ||
| const lines = chunk.toString().split('\n'); |
There was a problem hiding this comment.
I think you can pass the encoding to toString?
There was a problem hiding this comment.
Ah, it seems encoding will always be "buffer", and toString() doesn't like it. And the official doc tells us to ignore "buffer".
transform._transform(chunk, encoding, callback)
chunk <Buffer> | <string> | <any> The Buffer to be transformed, converted from the string passed to stream.write().
encoding <string> If the chunk is a string, then this is the encoding type. If chunk is a buffer, then this is the special value 'buffer'. Ignore it in that case.
callback <Function> A callback function (optionally with an error argument and data) to be called after the supplied chunk has been processed.
But I added a comment there so people will know why we are ignoring the encoding here.
|
|
||
| childProcess.stderr.on('data', error => forEachMessageLine(error, line => stderr.push(line))); | ||
| const stdErrPipe = newChildProcessOutputPipeTransform(line => { |
There was a problem hiding this comment.
One of the problems I was trying to solve here was to draw a visual distinction between what came from stderr and what didn't. This still will be the case (we're logging the entire contents of stderr below) but maybe we can prefix each line with [STDERR] or something like that as we log it?
There was a problem hiding this comment.
The problem is stderr doesn't always mean error. For example, in the case of the taskfile, it outputs normal content to stderr (see the image I attached above). If we prepend [STDERR] to each line, it might actually cause confusion.
There was a problem hiding this comment.
I'm not so sure, esp. if we also prepend the stdout lines as well. We're clarifying which stream a given log is coming from, in that case - not that it's an error.
There was a problem hiding this comment.
Yeah, so I think the question is why do we want to distinguish from these two streams? If you run a command in the terminal, it won't tell any difference. The command will use colors or log types like ERROR: xxx to tell the user whether there are any errors.
There was a problem hiding this comment.
If you run a command in the terminal, it won't tell any difference
I feel like that's a flaw, and if you're debugging something, ideally the issue would be coming from stderr, so it's good UX to focus on that content.
There was a problem hiding this comment.
Let's take an example of this output:
The first three lines are from stdout, and all the following are from stderr. Do you prefer we output something like the following?
[stdout] Running [node '--trace-uncaught' ...
[stdout] Running [go 'run' ...
[stdout] task: [/home/runner/work/...]
[stderr] task: "client:tun2socks:linux" started
[stderr] task: "client:tun2socks:electron" started
...
[stderr]
[stderr] task "client:tun2socks:electron" finished
[stderr] task "client:tun2socks:linux" finished
There was a problem hiding this comment.
I do, actually. I know that in your example nothing sent from stderr is actually an error, but shouldn't it be stdout? Is there a reason the lifecycle logs are stderr?
There was a problem hiding this comment.
You need to differentiate info output from the Task app from the actual task output. That's why the difference.
There was a problem hiding this comment.
BTW, as @jyyi1 pointed out, stderr output doesn't have to mean error. Logging uses that in general.
|
It just feels wrong to me, but okay! Disregard
…On Fri, May 3, 2024, 12:01 PM Vinicius Fortuna ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/build/spawn_stream.mjs
<#1998 (comment)>
:
>
- childProcess.stderr.on('data', error => forEachMessageLine(error, line => stderr.push(line)));
+ const stdErrPipe = newChildProcessOutputPipeTransform(line => {
BTW, as @jyyi1 <https://github.com/jyyi1> pointed out, stderr output
doesn't have to mean error. Logging uses that in general.
—
Reply to this email directly, view it on GitHub
<#1998 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4V5VADQ57SOUBGEETOZO3ZAOYFJAVCNFSM6AAAAABGWENO5SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZYGQ4DCMZUGY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
This PR redirects
stderrof a child process toconsole.error. This allows us to view the build steps ofclient/src/tun2socks/build, becausego run taskwill output tostderr.Additionally, unnecessary line breaks have been removed to improve the readability of the output.
Before
After
