Bitrise logging optimisation v2 #295
Conversation
| Stderr: loggingIO.ToolStderr, | ||
| }) | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
What was the reason for this? Using defer() is safer usually to clean up things as it is called from all exit points
There was a problem hiding this comment.
The comment left there is the key, we have to close the logging before we use the xcbuild raw output. We could leave xcpretty in defer, but at this point we could just explicitly close it with the others.
There was a problem hiding this comment.
The way how defer works is it will be executed just before exiting the method, so it will first execute the commands below if that's what you mean.
What was the reason you wanted to change this?
There was a problem hiding this comment.
Yes, with defer the loggingIO.XcbuildRawout.Bytes(), wasn't getting the right data (actually it wasn't getting any data). It was caught by the e2e tests of steps. The problem is exactly what you write, it will execute the below stuff first and the loggingIO.Close() is where we wait for the buffers to flush.
There was a problem hiding this comment.
Ah I see, so we need loggingIO.XcbuildRawout.Bytes() to execute after the closure, gotcha. Can you drop this in a comment on that line (91) please?
So // Wait for the filtering to finish before calling loggingIO.XcbuildRawout.Bytes()
| // Always close xcpretty outputs | ||
| defer func() { | ||
| if err := loggingIO.Close(); err != nil { | ||
| fmt.Printf("logging IO failure, error: %s", err) |
There was a problem hiding this comment.
Don't we want to close logging in these cases?
There was a problem hiding this comment.
The comment left there is the key, we have to close the logging before we use the xcbuild raw output. We could leave xcpretty in defer, but at this point we could just explicitly close it with the others.
There was a problem hiding this comment.
In this case we're not explicitly calling these in lines 82, 86, 91. But if we opened the pipes we should close them in those cases too, no?
This is why defer is preferable as you don't need to maintain explicit closing, it will just execute after the method exits
There was a problem hiding this comment.
Right, I forgot about the other exit points. I get how defer works.
There was a problem hiding this comment.
👍🏻 Ok, I see what we want to achieve here now.
In this case I suggest to create an anon method to avoid a lot of duplication
No description provided.