[llvm-branch-commits] [llvm] [CI] Make premerge_advisor_explain write comments (PR #166605)

[Aiden Grossman via llvm-branch-commits]

================
@@ -45,13 +83,31 @@ def main(commit_sha: str, build_log_files: list[str]):
     )
     if advisor_response.status_code == 200:
         print(advisor_response.json())
+        comments = [
+            get_comment(
+                github_token,
+                pr_number,
+                generate_test_report_lib.generate_report(
+                    generate_test_report_lib.compute_platform_title(),
+                    return_code,
+                    junit_objects,
+                    ninja_logs,
+                    failure_explanations_list=advisor_response.json(),
----------------
boomanaiden154 wrote:

> I think these comments could get quite large, but then again we are only leaving at most 1. You might have to add some more size limits for the extreme cases but not worth doing until we see it happen in practice.

Yeah. We already have the size limit that will get applied here, but those can still be large. I agree it's probably not worth doing until we see it in practice. I don't think I've seen any PR with more than ~10 failures so far.

> I wonder if the premerge advisor content should also go to the build summary, but perhaps A: it already does or B: the advisor runs at a point where we've already submitted the build summary.

> Then again, having the build summary be very much "this is exactly what happened" and the comments on the PR be more "human" and maybe speculative makes some sense to me.

It doesn't currently go to the build summary. I think I'd like to keep the build summary as exactly what happened for now. We can revisit that decision based on user feedback. And yeah, a big part of the reason to surface the advisor findings in a comment is because a lot of people do not realize the summary view exists.

https://github.com/llvm/llvm-project/pull/166605