[Debugify] Add 'acceptance-test' mode for the debugify report script #147574

SLTozer · 2025-07-08T17:17:10Z

For the purposes of setting up CI that makes use of debugify, this patch adds an alternative mode for the llvm-original-di-preservation.py script, which produces terminal-friendly(-ish) YAML output instead of an HTML report, and sets the return code to 1 if the input file contains errors, or 0 if the input file contains no errors or does not exist, making it simple to use it in CI.

This introduces a small change in existing usage, in that the path for the HTML report file is now passed with --report-file <path> rather than as a positional argument; I could make the argparse logic work without this change, but I believe that is simpler to understand this way, and to my knowledge debugify isn't currently being used in automated environments where changing this might cause issues. As a small change while passing by, I also changed -compress to --compress, for consistency.

As a note for reviewers, the reason that we treat a non-existent input file as a pass is that this is actually the expected state: we use clang to compile numerous files, passing a filepath for debugify errors. Any errors found by debugify will be written to this file; if none are found, the file is untouched. This is also mentioned in a code comment, but I think it useful to state upfront.

Finally, the justification for adding a new mode to this script instead of adding a separate script for the separate functionality is that this script understands debugify's output, and performs some deduplication that is useful for clarifying the resulting output. Writing a new script would require duplicating logic unnecessarily, and risks the scripts falling out-of-sync if changes are made to debugify's output.

For the purposes of setting up CI that makes use of debugify, this patch adds an alternative flag for the llvm-original-di-preservation.py script, which produces terminal-friendly output instead of an HTML report, and sets the return code to 1 if the input file contains errors, or 0 if the input file contains no errors or does not exist, making it simple to use it in CI. This introduces a small change in existing usage, in that the path for the HTML report file is now passed with `--report-file <path>` rather than as a positional argument; I could make the argparse logic work without this change, but I believe that is simpler to understand this way, and to my knowledge debugify isn't currently being used in automated environments where changing this might cause issues. The reason that we treat a non-existent input file as a pass is that this is actually the expected state: we use clang to compile numerous files, passing a filepath for debugify errors. Any errors found by debugify will be written to this file; if none are found, the file is untouched. The reason that we use this script at all, rather than using a separate script for what is largely a separate purpose, is that this script understands debugify's output, and performs some deduplication that is useful for clarifying the resulting output. Writing a new script would require duplicating logic unnecessarily.

github-actions · 2025-07-09T16:00:09Z

✅ With the latest revision this PR passed the Python code formatter.

jmorse

Looks good in general; I'm generally unfamiliar with this script though, so I'd much prefer if someone else could review it for approval.

jmorse · 2025-07-14T10:20:31Z

llvm/utils/llvm-original-di-preservation.py

+        if self.origin:
+            result["origin"] = self.origin
+        return result


No test coverage for this property ("origin")?

Added in latest commit.

jmorse · 2025-07-14T10:23:46Z

llvm/utils/llvm-original-di-preservation.py

+            "action": self.action,
+        }
+
+


If this becomes load-bearing, at some point we're going to have to switch over to using the "proper" yaml dumping and pretty-printing facilities. Does it make sense to do that now instead? (I don't think we need to make an all-singing-all-dancing implementation for all scripts, we can just focus on their narrow purpose).

I originally started using "proper" YAML, but this does require an extra package. Currently this (and most other utility scripts) can be run with just python, with no install commands necessary, and I consider preserving this to be a benefit worth some cost. My expectation is, this script has very straightforward and limited functionality, so there isn't too much to gain from using a proper YAML package - only very basic printing logic is required, and the YAML dumping segment is purely for user benefit (it has no expectation of being programmatically parsed).

jmorse · 2025-07-14T10:24:53Z

llvm/utils/llvm-original-di-preservation.py

-    parser.add_argument("html_file", type=str, help="html file to output data")
-    parser.add_argument(
-        "-compress", action="store_true", help="create reduced html report"
+    parser.add_argument("--compress", action="store_true", help="create reduced report")


I feel "short" or "reduced" or "summary" would be better for the option name, as "compress" makes me (and others?) think about file compression. YMMV, but if it's not too hard a change IMO it'd be better to not use "compress".

I don't wholly disagree, but this is a pre-existing flag so I'd prefer not to change it too much in this PR; the only reason for the change here is consistency with the other flags using -- instead of -.

Now that I think again, probably "reduce" is better match here.

jmorse · 2025-07-14T10:27:10Z

llvm/utils/llvm-original-di-preservation.py

+    if opts.error_test:
+        if os.path.isdir(opts.file_name):
+            print(f"error: Directory passed as input file: '{opts.file_name}'")
+            sys.exit(1)
+        if not os.path.exists(opts.file_name):
+            # We treat an empty input file as a success, as debugify will generate an output file iff any errors are
+            # found, meaning we expect 0 errors to mean that the expected file does not exist.
+            print(f"No errors detected for: {opts.file_name}")
+            sys.exit(0)


Why is this needed -- a switch enables code-paths to be tested that otherwise are never run?

The --error-test switch means that we're checking to see whether the input file contains any errors - this script will normally simply fail with 1 if we don't have a valid JSON file as input, but in this specific case we want to pass if given the path to a non-existent file because that's what we expect in the case where there are no errors (as Clang will never open the results file to write to it).

This does make sense, but I am having problems with the option name. I would rather go with something more intuitive, e.g. --allow-missing-results?

I agree it could be more clear - it does do more than just allow missing results though, as this is the toggle for a different "mode" of the script. I'll come up with something slightly more descriptive shortly!

I've gone for "acceptance test", since that more accurately describes the purpose imo.

SLTozer · 2025-07-16T09:27:33Z

I suspect if anyone is well-equipped to review this it would be @djtodoro - otherwise, I think this is a relatively "low-stakes" change.

djtodoro

@SLTozer thanks for this, it does make sense!

djtodoro · 2025-07-16T15:11:10Z

llvm/utils/llvm-original-di-preservation.py

-    parser.add_argument("html_file", type=str, help="html file to output data")
-    parser.add_argument(
-        "-compress", action="store_true", help="create reduced html report"
+    parser.add_argument("--compress", action="store_true", help="create reduced report")


Now that I think again, probably "reduce" is better match here.

djtodoro · 2025-07-16T15:11:59Z

llvm/utils/llvm-original-di-preservation.py

+
+    report_type_group = parser.add_mutually_exclusive_group(required=True)
+    report_type_group.add_argument(
+        "--report-file", type=str, help="output HTML file for the generated report"


Can we use --report-html-file instead?

djtodoro · 2025-07-16T15:12:49Z

llvm/utils/llvm-original-di-preservation.py

+    report_type_group.add_argument(
+        "--error-test",
+        action="store_true",
+        help="if set, produce terminal-friendly output and return 0 iff the input file is empty or does not exist",


Okay, it does make sense.

djtodoro · 2025-07-16T15:17:59Z

llvm/utils/llvm-original-di-preservation.py

+    if opts.error_test:
+        if os.path.isdir(opts.file_name):
+            print(f"error: Directory passed as input file: '{opts.file_name}'")
+            sys.exit(1)
+        if not os.path.exists(opts.file_name):
+            # We treat an empty input file as a success, as debugify will generate an output file iff any errors are
+            # found, meaning we expect 0 errors to mean that the expected file does not exist.
+            print(f"No errors detected for: {opts.file_name}")
+            sys.exit(0)


This does make sense, but I am having problems with the option name. I would rather go with something more intuitive, e.g. --allow-missing-results?

djtodoro

LGTM

SLTozer requested review from CarlosAlbertoEnciso, OCHyams, djtodoro, jmorse and jryans July 8, 2025 17:17

SLTozer self-assigned this Jul 8, 2025

SLTozer added 2 commits July 8, 2025 18:39

Missed the compress change in the test

8a9e435

Fix call to print fn, add lit test

2f5b32f

jmorse reviewed Jul 14, 2025

View reviewed changes

Add origin YAML test, darker format

733a3f2

djtodoro reviewed Jul 16, 2025

View reviewed changes

Address review comments

e5b6c36

djtodoro approved these changes Jul 16, 2025

View reviewed changes

Update tests and variable uses for new names, format

4fddfaf

SLTozer changed the title ~~[Debugify] Add 'error-test' mode for the debugify report script, for CI~~ [Debugify] Add 'acceptance-test' mode for the debugify report script Jul 17, 2025

SLTozer merged commit b7c14b6 into llvm:main Jul 17, 2025
10 checks passed

This was referenced Jul 23, 2025

test abhinavgaba/llvm-project#2

Closed

Add dataFence plugin interface abhinavgaba/llvm-project#3

Closed

[Debugify] Add 'acceptance-test' mode for the debugify report script #147574

[Debugify] Add 'acceptance-test' mode for the debugify report script #147574

Uh oh!

Conversation

SLTozer commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmorse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SLTozer commented Jul 16, 2025

Uh oh!

djtodoro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djtodoro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SLTozer commented Jul 8, 2025 •

edited

Loading

github-actions bot commented Jul 9, 2025 •

edited

Loading