[llvm][lit] fix writing results to --time-trace-output file

[da-viper]

This patch fixes an issue introduced with commit: 8507dba

[llvmbot]

@llvm/pr-subscribers-testing-tools

Author: Ebuka Ezike (da-viper)

Changes

Fix crash when time tracing.

---

Full diff: https://github.com/llvm/llvm-project/pull/130845.diff

1 Files Affected:

• (modified) llvm/utils/lit/lit/reports.py (+2-1)

diff --git a/llvm/utils/lit/lit/reports.py b/llvm/utils/lit/lit/reports.py
index 8312dcddc769a..cd32f0396677c 100755
--- a/llvm/utils/lit/lit/reports.py
+++ b/llvm/utils/lit/lit/reports.py
@@ -286,7 +286,8 @@ def _write_results_to_file(self, tests, elapsed, file):
 
         json_data = {"traceEvents": events}
 
-        json.dump(json_data, time_trace_file, indent=2, sort_keys=True)
+        with open(self.output_file, "w") as time_trace_file:
+            json.dump(json_data, time_trace_file, indent=2, sort_keys=True)
 
     def _get_test_event(self, test, first_start_time):
         test_name = test.getFullName()

[jh7370]

Just to clarify: is lit unconditionally failing with a python error here, because time_trace_file is an unknown identifier?

If so, LGTM, assuming that self.output_file is a file path and not a file object... (I haven't dug into all the code).

[da-viper]

Yes, this is the case. see

llvm-project/llvm/utils/lit/lit/reports.py

Lines 19 to 23 in 369c0a7

	class Report(object):
	def __init__(self, output_file):
	self.output_file = output_file
	# Set by the option parser later.

[jh7370]

Yes, this is the case. see

llvm-project/llvm/utils/lit/lit/reports.py

Lines 19 to 23 in 369c0a7

	class Report(object):
	def __init__(self, output_file):
	self.output_file = output_file
	# Set by the option parser later.

Okay, thanks. Do you have commit access or do you need me to merge this for you?

[da-viper]

Okay, thanks. Do you have commit access or do you need me to merge this for you?
No, not yet.

[jh7370]

Sorry, cancelling my LGTM. I'm not convinced about this change anymore.

I decided to dig into the background of this a bit more and it looks like it's a bug caused by @DavidSpickett's work in 8507dba. As far as I can see, this _write_results_to_file method shouldn't be called, except via the Report.write_results method. That method opens the file itself and then passes it into this function.

I think the correct fix is simply to use the passed-in file. @DavidSpickett should be able to confirm.

[jh7370]

LGTM again, but I'd like @DavidSpickett to just take a quick look to confirm.

[DavidSpickett]

Yes this looks correct. file will be the uniquely named file if it's been requested. Thanks for fixing this.

[jh7370]

@da-viper, sorry I didn't raise this before, but I just realised that you've got your email address marked as private in GitHub, which means we can't merge this per LLVM policy. See https://llvm.org/docs/DeveloperPolicy.html#email-addresses.

Also, I forgot to comment on your PR description/title, since those form the final commit message once this is merged. Since this PR is specific to a particular option, I suggest mentioning that in the title, e.g. "fix writing results to --time-trace file" (I can't remember the actual option name, please use the correct one!). In the description, you don't need to repeat what's in the title, but I'd suggest you add a reference to the commit that caused the issue (i.e. something like "This patch fixes an issue introduced with 8507dba.")

Let me know once you've addressed these issues and I'll click the merge button. (NB: if this doesn't happen by about 4pm GMT today, I'll not see your comment and am then off work for 2+ weeks, so you'll need to ask somebody else to merge the PR).

[da-viper]

@jh7370 Should be all done.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder arc-builder running on arc-worker while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/13059

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: 1200 seconds without output running [b'ninja', b'check-all'], attempting to kill
1.003 [59/18/1] Linking CXX executable tools/clang/unittests/Basic/BasicTests
1.728 [58/18/2] Linking CXX executable tools/clang/unittests/Format/FormatTests
6.267 [57/18/3] Linking CXX executable tools/clang/unittests/Lex/LexTests
6.388 [56/18/4] Linking CXX executable tools/clang/unittests/Rewrite/RewriteTests
6.606 [55/18/5] Linking CXX executable tools/clang/unittests/CrossTU/CrossTUTests
6.666 [54/18/6] Linking CXX executable tools/clang/unittests/libclang/libclangTests
7.149 [53/18/7] Linking CXX executable tools/clang/unittests/libclang/CrashTests/libclangCrashTests
command timed out: 1200 seconds without output running [b'ninja', b'check-all'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1362.735041