More details on IJ timings in the log #5325
Conversation
dkashyn-sfdc
requested review from
mai93,
jastice and
tpasternak
as code owners
github-actions
bot
added
product: CLion
| try { | ||
| Interner<String> stringInterner = | ||
| Optional.ofNullable(context.getScope(SharedStringPoolScope.class)) | ||
| .map(SharedStringPoolScope::getStringInterner) | ||
| .orElse(null); | ||
| return BlazeBuildOutputs.fromParsedBepOutput( | ||
| buildResult, buildResultHelper.getBuildOutput(stringInterner)); | ||
| context.output(PrintOutput.log("%s Parsing BEP outputs...", SummaryOutput.Prefix.TIMESTAMP)); |
There was a problem hiding this comment.
I don't know what are the consequences of it, but we mix PrintOutput and SummaryOutput here. How about doing it this way?
context.output(SummaryOutput.output(SummaryOutput.Prefix.TIMESTAMP, "Parsing BEP outputs..."));
additionally we could avoid overriding TIMESTAMP.toString(), which also doesn't look good to me, as in this case we would call IO inside toString method.
There was a problem hiding this comment.
Otherwise it just prints TIMESTAMP :(
There was a problem hiding this comment.
@tpasternak what would you say regarding output like this?
12:03:28 Sync started
12:06:13 Triggering build with flags '[--build_event_binary_file=/var/folders/k3/n5f1skhj65g10072006pgtvr0000gq/T/intellij-bep-31d9e34c-7f8f-4376-b9be-1b944e47f3d9, --nobuild_event_binary_file_path_conversion]' ...
12:07:40 Build command finished. Retrieving BEP outputs ...
12:07:40 BEP file '/var/folders/k3/n5f1skhj65g10072006pgtvr0000gq/T/intellij-bep-31d9e34c-7f8f-4376-b9be-1b944e47f3d9' file size 294227682 bytes
12:07:40 Parsing BEP outputs...
12:07:42 Handling parsed BEP outputs...
12:07:57 BEP outputs has been processed.
There was a problem hiding this comment.
So the line "Triggering build with flags is literally a duplicate of that, so i'm not sure if we need it that much
intellij/base/src/com/google/idea/blaze/base/async/process/ExternalTask.java
Lines 245 to 252 in 0435a2a
About the bep file path - I'm not sure if it's that useful, but yes, we can merge it if you wish
| context.output(SummaryOutput.output(SummaryOutput.Prefix.TIMESTAMP, "Build command finished. Retrieving BEP outputs ...")); | ||
| if (buildResultHelper instanceof BuildResultHelperBep) { | ||
| File outputFile = ((BuildResultHelperBep) buildResultHelper).getOutputFile(); | ||
| context.output(SummaryOutput.output(SummaryOutput.Prefix.TIMESTAMP, String.format("BEP file '%s' file size %d bytes", outputFile.getAbsolutePath(), outputFile.length()))); |
There was a problem hiding this comment.
I'm not sure if we really want to print the file path. But it's not a problem, we can merge it if you wish. Please only remove the duplicate file word in BEP file ... file size
There was a problem hiding this comment.
I don't know whether shards are expected to be executed sequentially forever so wanted to have some "correlation id" just for the case.
|
@tpasternak could you please re-review and merge if you are ok with the current state? |
|
Yeah, please only remove the "Triggering build with flags" line and let's merge it |
|
Removed @tpasternak |
github-actions
bot
removed
the
awaiting-review
Checklist
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.
Discussion thread for this change
Issue number:
<please reference the issue number or url here>Description of this change