[PATCH] D82791: [lit] Improve lit's output with default settings and --verbose.

[Varun Gandhi via Phabricator via llvm-commits]

varungandhi-apple added a comment.

Some of the changes probably don't make perfect sense in this revision because I initially started out with the whole change being one revision and then split things into commits, while trying to minimize the churn between revisions. Please also take a look at the follow-up revision D82811 <https://reviews.llvm.org/D82811> and let me know if that's acceptable (less churn, less self-contained, things make sense when you look at the two together) or if you'd prefer that I make this revision more self-contained (that would create more churn in D82811 <https://reviews.llvm.org/D82811>).



================
Comment at: llvm/utils/lit/lit/OutputSettings.py:34
+ONLY_FAILING_COMMAND = CommandOutputStyle("OnlyFailing")
+UP_TO_AND_INCLUDING_FAILING_COMMAND = CommandOutputStyle("UpToAndIncluding")
+
----------------
yln wrote:
> I really like the "communicating intent" part of this infrastructure.  However, this is a lot of code and `if`s considering we really only have to distinguish two cases.  From your commit message:
> 
> - default (no flags): no script, show only failing line in command output
> - `-v`: full script, adds 'set +x', shows command output
> 
> Am I missing something or could everything be keyed off a single verbose (reading from the code below `showAllOutput` implies verbose) flag.  Do we anticipate other combinations being useful? (In general I would argue for striving for the simplest implementation that gives us what we currently want and not try to anticipate future extensions.)
> 
> Have you considered (or started out with) just using a single verbose flag to base your decisions in the implementation functions?  
Not sure what you mean by two cases, I think there are three cases:

1. Quiet -> Corresponds on `NO_COMMAND`
2. Default (no quiet, no verbose) -> Corresponds to `ONLY_FAILING`.
3. Verbose -> Corresponds to `UP_TO_AND_INCLUDING_FAILING`.

Since there is a 1-1 correspondence, we _could_ compare quiet vs (!quiet && !verbose) vs verbose in `make_command_output`. I chose not to do that because I figured this makes the intent clearer by distinguishing between user-specified options and derived options.

Does that make sense? Would you still prefer that I get rid of this?


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1503-1508
+    Returns a pair of:
+    - The index in ``output_str`` pointing to immediately after the preceding
+      newline, i.e. the start of the RUN line, before any shell-specific
+      prefix.
+    - The matched substring itself, including the number at the end,
+      starting with 'RUN', skipping the shell-specific prefix.
----------------
yln wrote:
> Why use a complicated parser-like return value? Our only caller below could just receive the potentially found RUN line.
Sorry, it's not super clear because in this revision, there is only one caller, which could do with just using the index. However, the highlighting patch (which comes right after this logically) ends up making use of the substring. The line of reasoning is:

1. A substring by itself is not good enough (and hence `make_command_output` in this patch makes use of the first index return value), because we want to distinguish the case `line_start == 0` from the case `line_start > 0` by printing `Command Output (stderr)` instead of `Command Output (stderr, truncated)` when `line_start == 0`.
2. An index by itself could be "good enough", but we try to be smarter in the highlighting patch (https://reviews.llvm.org/D82811) by not highlighting the shell-specific prefix, and starting highlighting from the "RUN", which `make_script_output` makes use of.

Does that make sense? I decided against splitting changes to this function into two revisions and have the full functionality in the first iteration because I felt like that would create more churn with the tests and the implementation, but I see how it can be confusing as to why two related-but-slightly-different-values are being returned simultaneously when only one is really being used in this patch.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1593
+    assert(lit_config.script_output_style == OutputSettings.FULL_SCRIPT)
+    return default_output()
+
----------------
yln wrote:
> Why are we using two local functions here?
> 
> The whole thing could be (already assuming just one verbose flag):
> 
> ```
> def make_script_output(lit_config, script_lines, exit_code):
>     if not lit_config.verbose:
>         return ""
>     return ...
> ```
It mimics the structure of the `make_command_output` function right below. Right now, this function is very simple, so it probably seems overkill. However, `make_script_output` becomes much more complicated in the highlighting patch after this: https://reviews.llvm.org/D82811, so I figured it would be helpful to have the two functions `make_script_output` and `make_command_output` follow a similar pattern.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1614-1617
+    line_start, run_line_str = locate_last_run_line(cmd_output)
+    # maybe there was an error, or maybe we are not truncating anything
+    if run_line_str is None or line_start == 0:
+        return default_output()
----------------
yln wrote:
> This block should be pushed into the `lit_config.command_output_style == OutputSettings.ONLY_FAILING_COMMAND` case, otherwise we are always searching for the last line, even if we don't really use the result of the computation.
> 
> Also: if we fail to find the RUN line, we might print the note to use `--verbose` even if the user already specified it.
> This block should be pushed into the lit_config.command_output_style == OutputSettings.ONLY_FAILING_COMMAND case, otherwise we are always searching for the last line, even if we don't really use the result of the computation.

This part is actually used by the highlighting patch (https://reviews.llvm.org/D82811), where the last line of this function becomes:

```
    failing_lines = highlight_failure_lines(cmd_output[line_start:])
    return make_output(cmd_output[:line_start] + failing_lines,
                       is_truncated=False)
```

That's why I didn't push it into the conditional branch.

> Also: if we fail to find the RUN line, we might print the note to use --verbose even if the user already specified it.

Good catch, I've fixed `make_output` to check for `is_truncated` properly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82791/new/

https://reviews.llvm.org/D82791