Add option to pipe to file and another tool #329
Conversation
node/toolrunner.ts
Outdated
| this.pipeOutputToTool._getSpawnArgs(options), | ||
| this.pipeOutputToTool._getSpawnOptions(options)); | ||
|
|
||
| let fileStream: fs.WriteStream = this.pipeOutputToFile ? fs.createWriteStream(this.pipeOutputToFile) : null; |
There was a problem hiding this comment.
will this work if the file already exists? It should append in that case I guess?
node/toolrunner.ts
Outdated
|
|
||
| let fileStream: fs.WriteStream = this.pipeOutputToFile ? fs.createWriteStream(this.pipeOutputToFile) : null; | ||
| if (fileStream) { | ||
| fileStream.write(this._getCommandString(options) + os.EOL); |
There was a problem hiding this comment.
is it weird to write the command string? I would expect the file to only contain the output of the first tool.
There was a problem hiding this comment.
The command string helps set context for what this output was for. Otherwise user has to use the step number to match the output especially if there are multiple tasks of the same type that are producing these files. I initially did not write it but after I tried e2e it made more sense to write it.
There was a problem hiding this comment.
we cant do this. it may contain secrets and does not get run through the log scrubber
node/toolrunner.ts
Outdated
| cpFirst.stdout.on('data', (data: Buffer) => { | ||
| try { | ||
| if (fileStream) { | ||
| fileStream.write(data.toString()); |
There was a problem hiding this comment.
that's right, it should not be needed. I will pull it out and verify.
node/toolrunner.ts
Outdated
| }); | ||
| cpFirst.stderr.on('data', (data: Buffer) => { | ||
| if (fileStream) { | ||
| fileStream.write(data.toString()); |
There was a problem hiding this comment.
same question about .toString()
node/toolrunner.ts
Outdated
| }); | ||
| cpFirst.on('error', (err) => { | ||
| if (fileStream) { | ||
| fileStream.on('finish', () => { |
There was a problem hiding this comment.
copy paste error? shouldn't this write stderr to the file too?
node/toolrunner.ts
Outdated
| defer.reject(new Error(toolPathFirst + ' failed. ' + err.message)); | ||
| }); | ||
| cpFirst.on('close', (code, signal) => { | ||
| if (fileStream) { |
There was a problem hiding this comment.
I wonder if this is reliable. Do you know whether close means no further stdout/stderr data events are pending to be drained? I believe the node docs have good details about this, but I don't remember the details offhand.
There was a problem hiding this comment.
It should be as per https://nodejs.org/api/child_process.html#child_process_event_close - The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams.
There was a problem hiding this comment.
I wrote tests and found that there were some timing issues. I fixed the issues but the code looks ugly. Will sync up before doing any refactoring.
madhurig
force-pushed
the
users/madhurig/pipeToFileAndTool
branch
from
68e56e7 to
4c75ff7
Compare
|
|
||
| var inQuotes = false; | ||
| var escaped =false; | ||
| var escaped = false; |
There was a problem hiding this comment.
@ericsciple: I ran format command in VSCode that brought in other changes. If you don't want these committed, I can back them out .
…FileAndTool Add option to pipe to file and another tool
We want to be able to write the output of the first tool to a file in addition to piping it to another tool to simulate something like this: xcodebuild | tee xcodebuild.log | xcpretty. This works but looking for feedback on the approach before adding unit tests.