This is an archive of the discontinued LLVM Phabricator instance.

[lit] Drop "Script:", make -v and -a imply -vv
ClosedPublic

Authored by jdenny on Jul 11 2023, 9:52 AM.

Details

Summary

This patch and D156954 were discussed in https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839.

Motivation: -a shows output from all tests, and -v shows output from just failed tests. Without this patch, that output from each test includes a section called "Script:", which includes all shell commands that lit has computed from RUN directives and will attempt to run for that test. The effect of -vv (which also implies -v if neither -a or -v is specified) is to extend that output with shell commands as they are executing so you can easily see which one failed.

For example, when using lit's internal shell and -vv:

Script:                                                                                                                 
--                                                                                                                      
: 'RUN: at line 1'; echo hello world                                                                                    
: 'RUN: at line 2'; 3c40 hello world                                                                                    
: 'RUN: at line 3'; echo hello world                                                                                    
--                                                                                                                      
Exit Code: 127                                                                                                          
                                                                                                                        
Command Output (stdout):                                                                                                
--                                                                                                                      
$ ":" "RUN: at line 1"                                                                                                  
$ "echo" "hello" "world"                                                                                                
# command output:                                                                                                       
hello world                                                                                                             
                                                                                                                        
$ ":" "RUN: at line 2"                                                                                                  
$ "3c40" "hello" "world"                                                                                                
# command stderr:                                                                                                       
'3c40': command not found                                                                                               
error: command failed with exit status: 127                                                                             
                                                                                                                        
--

Notice that all shell commands that actually execute appear in the output twice, once for "Script:" and once for -vv. Especially for tests with many RUN directives, the result is noisy. When searching through the output for a particular shell command, it is easy to get lost and mistake shell commands under "Script:" for shell commands that actually executed.

Change: With this patch, a test's output changes in two ways. First, the "Script:" section is never shown. Second, omitting -vv no longer disables printing of shell commands as they execute. That is, -a and -v imply -vv, and so -vv is deprecated as it is just an alias for -v.

Secondary motivation: We are also working to introduce a PYTHON directive, which can appear between RUN directives. How should PYTHON directives be represented in the "Script:" section, which has previously been just a shell script? We could probably think of something, but adding info about PYTHON directive execution in the -vv trace seems more straight-forward and more useful.

(This patch also removes a confusing point in the -vv documentation: at least when using bash as an external shell, -vv echoes commands to the shell's stderr not stdout.)

Diff Detail

Event Timeline

jdenny created this revision.Jul 11 2023, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 9:52 AM
Herald added a subscriber: delcypher. · View Herald Transcript
jdenny requested review of this revision.Jul 11 2023, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2023, 9:52 AM

This is a very nice clean-up, thanks!

I've only skimmed through so far and just wanted to post a quick question about this:

Change: After this patch, there is no "Script:" section

IMO, it would be nice to be able to simply print the RUN lines that LIT has collected. Especially for complex tests for which RUN lines depend on various CMake flags (to help developing such tests). So, I would keep the "Script" section, but:

  • only print it when explicitly requested (with a dedicated flag, e.g. -print-script),
  • make it default to "collect the RUN lines, print them and exit" (so the tests wouldn't run).

Would this be difficult and would it make sense to you? Or perhaps that's already supported? :)

awarzynski added inline comments.Jul 11 2023, 2:39 PM
llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
9–12

IIUC, full RUN line would be printed here, right?

jdenny added a comment.EditedJul 11 2023, 2:50 PM

This is a very nice clean-up, thanks!

I've only skimmed through so far and just wanted to post a quick question about this:

Thanks for taking a look.

Change: After this patch, there is no "Script:" section

IMO, it would be nice to be able to simply print the RUN lines that LIT has collected. Especially for complex tests for which RUN lines depend on various CMake flags (to help developing such tests).

In case it wasn't clear, with this patch, you do see the fully expanded RUN lines that actually execute, as in the trace of any shell language (or any that I've used).

If a RUN line fails, you don't see the remaining RUN lines that didn't execute. Are you saying you would like to see those too? In my usage, I have never wanted to because I always wanted to address the current failure and then retry before looking any farther in case there's an interaction. Can you help me to understand your use case a bit more?

So, I would keep the "Script" section, but:

  • only print it when explicitly requested (with a dedicated flag, e.g. -print-script),
  • make it default to "collect the RUN lines, print them and exit" (so the tests wouldn't run).

Would this be difficult and would it make sense to you? Or perhaps that's already supported? :)

This patch does not currently support that. It would not be difficult now but will require more thought if something like the PYTHON directive is accepted, especially if it grows the ability to affect substitutions and thus subsequent RUN lines.

jdenny added inline comments.Jul 11 2023, 3:04 PM
llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
9–12

Yes. Previously, this test was looking in the "Script:" section to verify correct expansion of %if. Now that "Script:" is gone, I changed it to just look for correct stdout from the executed command. Perhaps I should have changed it to look at the printed RUN line (from -vv)? What do you think? @asavonic might also want to comment, as he wrote the original test.

LGTM, but I'd wait for more approvals

LGTM, but I'd wait for more approvals

Thanks for reviewing. I agree about waiting in case there are use cases for "Script:" I hadn't imagined.

In case it wasn't clear, with this patch, you do see the fully expanded RUN lines that actually execute, as in the trace of any shell language (or any that I've used).

Yes, but together with all the other test output, which can be very noisy :(

If a RUN line fails, you don't see the remaining RUN lines that didn't execute. Are you saying you would like to see those too?

I meant something slightly different. Some RUN get very complicated - especially when adding {RE}DEFINE to the mix and e.g. emulators to run tests on. I would like to be able to simply check that LIT correctly interprets my RUN lines without running the tests (which can take some time to run and generate a lot of output). I think that that would be very useful. And looks like it's going to get harder to achieve :/

Btw, this patch changes the semantics of LIT. I think that it would be good post a short RFC on Discourse and to advertise this. Some people might rely on the current behavior. Or perhaps this has already been discussed somewhere?

llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
9–12

Hm, so this patch gets rid of "Script", but everything else remains intact, right. The changes in tests suggest that it;s a bit more intrusive, hence my original question.

If a RUN line fails, you don't see the remaining RUN lines that didn't execute. Are you saying you would like to see those too?

I meant something slightly different. Some RUN get very complicated - especially when adding {RE}DEFINE to the mix and e.g. emulators to run tests on. I would like to be able to simply check that LIT correctly interprets my RUN lines without running the tests (which can take some time to run and generate a lot of output). I think that that would be very useful.

So I'm hearing there are two issues with using -vv-like traces for your use case:

  1. They are noisy. I wonder if the real problem is that we just need a more readable format for the trace. For example, I wonder if indenting all stdout/stderr lines with "> " or something would help distinguish them from command lines. There's also the issue that we can easily control lit's internal shell, but external shells format the trace differently.
  2. They take a long time to run. I don't know how to address that in the -vv trace. Indeed it seems you would need something like the "Script:" section.

And looks like it's going to get harder to achieve :/

Right, PYTHON directives make shell commands more dynamic, so lit cannot always fully expand the shell commands until it's time to execute them.

However, one simple possibility is for your proposed -print-script to print all RUN lines before the first PYTHON directive. If you don't use PYTHON directives (e.g., if the community rejects PYTHON directives), you'll get exactly what you want. If you do use PYTHON directives, you'll get only any initial RUN commands. Of course, we could also discuss printing "Script:" sections between PYTHON blocks as the test executes, but that runs into the execution time problem above.

Btw, this patch changes the semantics of LIT. I think that it would be good post a short RFC on Discourse and to advertise this. Some people might rely on the current behavior.

After the points you raise, I agree we should have an RFC before landing this patch. However, do you mind if we have more discussion here first? I think that would produce a better RFC.

Or perhaps this has already been discussed somewhere?

I don't recall anything.

jdenny added inline comments.Jul 12 2023, 9:16 AM
llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
9–12

I tried to update tests while maintaining their original focus (as well as I understood) and not clutter them with checking of the trace. I think the changes to shtest-run-at-line.py show the effect of this patch the best. However, look only at the internal shell checks. The external shell checks intentionally avoid showing most of the trace given that we have no control over the way they format traces.

Thanks for this feedback. I'm going to work on adding an example to the patch summary as well.

jdenny added inline comments.Jul 12 2023, 9:19 AM
llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt
9–12

Hm, so this patch gets rid of "Script", but everything else remains intact, right.

Yes, that's right. It also includes the -vv trace any time -v or -a shows output for a test.

jdenny edited the summary of this revision. (Show Details)Jul 12 2023, 9:51 AM

LGTM, but I'd wait for more approvals

Thanks for reviewing. I agree about waiting in case there are use cases for "Script:" I hadn't imagined.

This is non-blocking, but I do use Script: a lot to reproduce libc++ test failures. Here's an example:

FAIL: llvm-libc++-shared.cfg.in :: std/input.output/filesystems/fs.op.funcs/fs.op.proximate/proximate.pass.cpp (1 of 1)
******************** TEST 'llvm-libc++-shared.cfg.in :: std/input.output/filesystems/fs.op.funcs/fs.op.proximate/proximate.pass.cpp' FAILED ********************
Script:
--
: 'COMPILED WITH';  <TOOLCHAIN>/usr/bin/clang++ <LLVM>/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.proximate/proximate.pass.cpp -pthread -isysroot <SDK> --target=arm64-apple-darwin21.4.0 -nostdinc++ -I <LLVM>/build/default/include/c++/v1 -I <LLVM>/build/default/include/c++/v1 -I <LLVM>/libcxx/test/support -std=c++2b LOTS-OF-FLAGS -nostdlib++ -L <LLVM>/build/default/lib -Wl,-rpath,<LLVM>/build/default/lib -lc++ -o <LLVM>/build/default/test/std/Output/proximate.pass.cpp.dir/t.tmp.exe
: 'EXECUTED AS';  "python3.11" <LLVM>/libcxx/test/../utils/run.py --execdir <LLVM>/build/default/test/std/Output/proximate.pass.cpp.dir --  <LLVM>/build/default/test/std/Output/proximate.pass.cpp.dir/t.tmp.exe
--
Exit Code: 250

Command Output (stdout):
--
$ ":" "COMPILED WITH"
$ "<TOOLCHAIN>/usr/bin/clang++" "<LLVM>/libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.proximate/proximate.pass.cpp" "-pthread" "-isysroot" "<SDK>" "--target=arm64-apple-darwin21.4.0" "-nostdinc++" "-I" "<LLVM>/build/default/include/c++/v1" "-I" "<LLVM>/build/default/include/c++/v1" "-I" "<LLVM>/libcxx/test/support" "-std=c++2b" LOTS-OF-FLAGS "-nostdlib++" "-L" "<LLVM>/build/default/lib" "-Wl,-rpath,<LLVM>/build/default/lib" "-lc++" "-o" "<LLVM>/build/default/test/std/Output/proximate.pass.cpp.dir/t.tmp.exe"
# command stderr:
ld: warning: object file (<LLVM>/build/default/lib/libc++experimental.a(memory_resource.cpp.o)) was built for newer macOS version (13.3) than being linked (13.0)
ld: warning: dylib (<LLVM>/build/default/lib/libc++.dylib) was built for newer macOS version (13.3) than being linked (13.0)

$ ":" "EXECUTED AS"
$ "python3.11" "<LLVM>/libcxx/test/../utils/run.py" "--execdir" "<LLVM>/build/default/test/std/Output/proximate.pass.cpp.dir" "--" "<LLVM>/build/default/test/std/Output/proximate.pass.cpp.dir/t.tmp.exe"
# command stderr:
Assertion failed: (!PathEq(output, expect)), function basic_test, file proximate.pass.cpp, line 151.

error: command failed with exit status: 250

There are two properties of Script that I like here:

  1. It is not quoted. It is easier to copy-paste, modify and run into a terminal to reproduce the issue and iterate outside of Lit.
  2. It shows all commands at once instead of showing only the ones that were executed, with output in between. If I want to know how to execute the program that was compiled above, I can easily use Script to figure that out. If I only have the Command Output, I have to search for EXECUTED AS through potentially a lot of output from the first command. And if the first command fails, then I will never know how this would get executed if this were to compile successfully.

I wouldn't say either of these issues should block this patch, however as-is this will cause a small quality-of-life regression for libc++ development. I don't have anything concrete to suggest here, so I'm fine with this, but if you can think of a way to retain either of these properties, it might be worth exploring.

  1. They take a long time to run. I don't know how to address that in the -vv trace. Indeed it seems you would need something like the "Script:" section.

It just occurred to me that lit doesn't show the "Script:" section until the execution has terminated (lit prints it at the same time as the script's exit code). So this execution time issue is more of a feature request. It is not a regression that would be caused by this patch.

This is non-blocking, but I do use Script: a lot to reproduce libc++ test failures. Here's an example:

Thanks for describing your use case.

There are two properties of Script that I like here:

  1. It is not quoted. It is easier to copy-paste, modify and run into a terminal to reproduce the issue and iterate outside of Lit.

Are you saying you have to do some sort of fixup in order to copy-paste and run it in a terminal? Or are you saying the quoting is just annoying if you want to modify the command line? Either way, for lit's internal shell (which your example appears to be using), I'm sure we can find a way to reduce unnecessary quoting.

  1. It shows all commands at once instead of showing only the ones that were executed, with output in between. If I want to know how to execute the program that was compiled above, I can easily use Script to figure that out. If I only have the Command Output, I have to search for EXECUTED AS through potentially a lot of output from the first command.

Would it be difficult in your workflow to send it through grep? Is there any way to format the -vv trace to make that easier?

And if the first command fails, then I will never know how this would get executed if this were to compile successfully.

I don't think we can address this issue generally with PYTHON directives, but it's still possible to print all leading RUN directives.

I wouldn't say either of these issues should block this patch, however as-is this will cause a small quality-of-life regression for libc++ development.

Well, my main motivation was to improve quality of life. For me, "Script:" has been noise for years, but obviously that's not true for everyone.

Would you be ok with @awarzynski's request that "Script:" is printed only with -print-script, which doesn't execute the test?

MaskRay added a comment.EditedJul 14 2023, 1:37 PM

Thanks for the clean up. I think the new option (-a -v) semantics are more orthogonal. I always use -a or -vv and avoid -v.

My understanding of the behavior of $build/bin/llvm-lit -a test: for a succeeded test, all commands will be printed. For a failed test, all commands til the failed one will be printed.

I wonder whether we can change the Command Output (stderr): (pipe a|b is printed as + a\n+ b; for copy pasting, we need to remove the plus sign) to be like Script: (a|b is printed as is).
Perhaps it will fulfill @ldionne's request "This is non-blocking, but I do use Script: a lot to reproduce libc++ test failures. Here's an example:"


If changing Command Output (stderr): may lose functionality that someone may use, we can add a new option for the current + a\n+ b behavior (IMHO not useful). For dumped commands from -a and -v, I wish that they are copyable without needing to strip +.

After the points you raise, I agree we should have an RFC before landing this patch. However, do you mind if we have more discussion here first? I think that would produce a better RFC.

Sure! Just to clarify my view:

  • strong +1 to how the relation between -v/-a/-vv are cleaned-up in this patch,
  • preserving "Script" would be helpful.

I rely on "Script" when debugging test set-ups. Specifically, MLIR integration tests (e.g. sparse_conversion.mlir) that can:

  • run natively or via an emulator,
  • use host or x-compiled lli (or mlir-cpu-runner),
  • run 3-4 tools per RUN line (i.e. tool-1 | tool-2 | tool-3 | tool-4).

The resulting RUN lines tend to be quite complex. Having them summarised in one place helps.

For reference, this is what I get with "Script" (ToT LLVM):

: 'RUN: at line 10';  <WORKDIR>build/release/bin/mlir-opt <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir --sparse-compiler=enable-runtime-library=true | <WORKDIR>/build/release/bin/mlir-cpu-runner -e entry -entry-point-result=void -shared-libs=<WORKDIR>/build/release/lib/libmlir_c_runner_utils.so | <WORKDIR>/build/release/bin/FileCheck <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir

This can be c&p into a shell and will "just work". That's very useful.

This is what I get with -a with your patch:

+ <WORKDIR>/build/release/bin/mlir-opt <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir --sparse-compiler=enable-runtime-library=true
+ <WORKDIR>/build/release/bin/mlir-cpu-runner -e entry -entry-point-result=void -shared-libs=<WORKDIR>/build/release/lib/libmlir_c_runner_utils.so
+ <WORKDIR>/build/release/bin/FileCheck <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir

Sadly, this won't work in a shell without a bit of formatting. Could this be updated to match "Script"? If yes then that's not a big issue. I would still like for "Script" to be preserved, but that's not a blocker for me (I would still suggest checking on Discourse). Btw, IMHO it would be helpful to include the ideas proposed in https://reviews.llvm.org/D154987 in any potential RFC.

Last, but not least, thank you for caring about LIT and improving it for us - that's greatly appreciated!

There are two properties of Script that I like here:

  1. It is not quoted. It is easier to copy-paste, modify and run into a terminal to reproduce the issue and iterate outside of Lit.

Are you saying you have to do some sort of fixup in order to copy-paste and run it in a terminal? Or are you saying the quoting is just annoying if you want to modify the command line? Either way, for lit's internal shell (which your example appears to be using), I'm sure we can find a way to reduce unnecessary quoting.

It's just that the quoting is annoying. I'd be more than happy if we manage to reduce unnecessary quoting, or perhaps I just want to learn to live with it. After all, quoted commands are more copy-paste safe than unquoted ones.

  1. It shows all commands at once instead of showing only the ones that were executed, with output in between. If I want to know how to execute the program that was compiled above, I can easily use Script to figure that out. If I only have the Command Output, I have to search for EXECUTED AS through potentially a lot of output from the first command.

Would it be difficult in your workflow to send it through grep? Is there any way to format the -vv trace to make that easier?

I'm usually just eyeballing at the Lit results, so yeah I don't really want to use grep for that. At the same time, our SCRIPTs are usually not too large, so I don't think this is a huge issue.

Would you be ok with @awarzynski's request that "Script:" is printed only with -print-script, which doesn't execute the test?

TBH, I'd love to avoid introducing another flag to control the output of Lit. This just seems like adding complexity to me. I think your proposed changes are better and more general, and they unblock useful feature work. I would go for the pure approach and then we can all learn to live with the new output results and improve them (without adding new flags) as we gain experience with that new output.

Again, just my .02 but I'd be fine if you didn't try to satisfy everyone's current usage. We'll learn to live with it and we'll improve the baseline output of the tool as we go, but let's not add complexity to the tool just for the sake of getting consensus here.

@MaskRay and @awarzynski: Thanks for your comments. You both have pointed out that the -vv-style command trace breaks apart commands joined by | (and other command separators) so it's hard to copy-and-paste command lines to a terminal.

Which of the following would you prefer?

  1. Keep "Script:". To address the redundancy problem that motivated this patch, disable "Script:" by default when just given -a or -v, and add some new command-line option to enable it (or maybe recycle -vv). To address the PYTHON line problem that motivated this patch, "Script:" would not include RUN lines beyond the first PYTHON line.
  1. Extend lit's internal shell to print full shell command lines before executing them. Reconfigure tests suites that need this functionality to use lit's internal shell. (The examples you showed appear to be using external shells.) Perhaps improve the formatting of stdout/stderr to better distinguish it so shell commands are easier to find.

1 would be the easiest to implement now, but its solutions to the original motivations for this patch feel like a bad design with an inconsistent user interface.

2 requires more work, including adjusting some test suites that want to use it. However, if people can live with it, I feel like it's the cleanest path forward.

I started to propose a third alternative to handle external shells. However, I feel like we should encourage use of lit's internal shell for portability anyway, and we should avoid hacking around external shells to try to make them do what we want.

Last, but not least, thank you for caring about LIT and improving it for us - that's greatly appreciated!

You're welcome!

TBH, I'd love to avoid introducing another flag to control the output of Lit. This just seems like adding complexity to me. I think your proposed changes are better and more general, and they unblock useful feature work. I would go for the pure approach and then we can all learn to live with the new output results and improve them (without adding new flags) as we gain experience with that new output.

Again, just my .02 but I'd be fine if you didn't try to satisfy everyone's current usage. We'll learn to live with it and we'll improve the baseline output of the tool as we go, but let's not add complexity to the tool just for the sake of getting consensus here.

Thanks for this opinion and your other answers. It sounds to me like your thinking is compatible with option 2 above... but with the added note that we don't have to perfect the trace format immediately.

  1. Keep "Script:". To address the redundancy problem that motivated this patch, disable "Script:" by default when just given -a or -v, and add some new command-line option to enable it (or maybe recycle -vv).

I can easily imagine people being confused that what was -vv is now -v, but -vv lives on with new semantics.

To address the PYTHON line problem that motivated this patch, "Script:" would not include RUN lines beyond the first PYTHON line.

This doesn't feel intuitive for users as well. More like an implementation detail leaking to user interface, which users will have to learn about.

Because of that, I'd discourage going with option 1 even as stopgap until landing another option.

MaskRay added a comment.EditedJul 17 2023, 11:25 PM
  1. Keep "Script:". To address the redundancy problem that motivated this patch, disable "Script:" by default when just given -a or -v, and add some new command-line option to enable it (or maybe recycle -vv).

I can easily imagine people being confused that what was -vv is now -v, but -vv lives on with new semantics.

To address the redundancy problem, disabling "Script:" in -a and -v modes looks good to me. Keeping "Script:" functionality with -vv looks good if users need this feature.

I have had the habit to always use -vv instead of -v to know the failed command number. Some users may be like me.
Removing Script: from -v and reporting the failed commands will make the -v mode usable.

To address the PYTHON line problem that motivated this patch, "Script:" would not include RUN lines beyond the first PYTHON line.

This doesn't feel intuitive for users as well. More like an implementation detail leaking to user interface, which users will have to learn about.

Because of that, I'd discourage going with option 1 even as stopgap until landing another option.

  1. Extend lit's internal shell to print full shell command lines before executing them. Reconfigure tests suites that need this functionality to use lit's internal shell.

Does this mean to print a pipe as one single command instead of two? Current output is:

Script:
--
: 'RUN: at line 1';   echo xxx | cat
: 'RUN: at line 2';   echo yyy | cat >&2
--
Exit Code: 0

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "echo" "xxx"
# command output:
xxx

$ "cat"
$ ":" "RUN: at line 2"
$ "echo" "yyy"
# command output:
yyy

$ "cat"

I can see that preserving the pipe and redirection ($ "echo" "xxx" | "cat" 2>&1) will make the command copyable and possibly more useful.

I don't know how people feel about printing # command output: immediately after printing a command with the internal shell (LIT_USE_INTERNAL_SHELL=1).
It seems to get in the way if we want to replace Script:.

Regarding the external shell, it collects all stdout and stderr. I think just removing the + sign at the beginning of a command will make the output a replacement for Script:.

  1. Extend lit's internal shell to print full shell command lines before executing them. Reconfigure tests suites that need this functionality to use lit's internal shell.

Does this mean to print a pipe as one single command instead of two?

Yes, it would print command lines the way "Script:" did except it would print them right before executing them.

I don't know how people feel about printing # command output: immediately after printing a command with the internal shell (LIT_USE_INTERNAL_SHELL=1).
It seems to get in the way if we want to replace Script:.

I'm not sure I understand your meaning. How does it get in the way?

Regarding the external shell, it collects all stdout and stderr. I think just removing the + sign at the beginning of a command will make the output a replacement for Script:.

Do you know how to tell the external shells to change the way they format their trace output? Is there a reason test suites still need to use external shells instead of lit's more portable internal shell?

  1. Extend lit's internal shell to print full shell command lines before executing them. Reconfigure tests suites that need this functionality to use lit's internal shell.

Does this mean to print a pipe as one single command instead of two?

Yes, it would print command lines the way "Script:" did except it would print them right before executing them.

I don't know how people feel about printing # command output: immediately after printing a command with the internal shell (LIT_USE_INTERNAL_SHELL=1).
It seems to get in the way if we want to replace Script:.

I'm not sure I understand your meaning. How does it get in the way?

It gets in the way because if people want to copy all commands, they'd copy the command output as well.
And to differentiate command output from RUN lines , we have to print prompts like $ .

If the internal shell can collect Command Output (stdout): and stderr and print them after all RUN commands are dumped, we can remove the prompts.

Regarding the external shell, it collects all stdout and stderr. I think just removing the + sign at the beginning of a command will make the output a replacement for Script:.

Do you know how to tell the external shells to change the way they format their trace output? Is there a reason test suites still need to use external shells instead of lit's more portable internal shell?

I think many more developers work on non-Windows platforms and get external shell by default, and very few people know LIT_USE_INTERNAL_SHELL=1.

Command Output (stdout)
$ ...

If we can make the internal shell work well enough and make it the default for non-Windows platforms, that's fine with me.

For now, I mostly care about the behavior of the external behavior:)

If the internal shell can collect Command Output (stdout): and stderr and print them after all RUN commands are dumped, we can remove the prompts.

I specifically want to see which command printed which part of the output. Separating them as you suggest would lose that information.

What if we put the output in shell comments so it has no effect when pasted into a terminal?

I think many more developers work on non-Windows platforms and get external shell by default, and very few people know LIT_USE_INTERNAL_SHELL=1.

I don't normally use that environment variable. Instead, I configure the test suite permanently with the following in its lit.cfg:

config.test_format = lit.formats.ShTest(execute_external=False)

If we can make the internal shell work well enough and make it the default for non-Windows platforms, that's fine with me.

I'm not sure we can change the lit default in general. Some test suites might still require external shell features that lit's internal shell doesn't support. Instead, we can change it per test suite, as shown above.

@MaskRay and @awarzynski: Thanks for your comments. You both have pointed out that the -vv-style command trace breaks apart commands joined by | (and other command separators) so it's hard to copy-and-paste command lines to a terminal.

Which of the following would you prefer?

  1. Keep "Script:". To address the redundancy problem that motivated this patch, disable "Script:" by default when just given -a or -v, and add some new command-line option to enable it (or maybe recycle -vv). To address the PYTHON line problem that motivated this patch, "Script:" would not include RUN lines beyond the first PYTHON line.
  1. Extend lit's internal shell to print full shell command lines before executing them. Reconfigure tests suites that need this functionality to use lit's internal shell. (The examples you showed appear to be using external shells.) Perhaps improve the formatting of stdout/stderr to better distinguish it so shell commands are easier to find.

We should probably aim for Option 2 as the cleaner and more future-proof approach. Personally, I mostly care about being able to copy & paste the command. "Script" (in the output) is nice as it makes finding RUN lines easy, but it's definitely not a show-stopper.

I don't quite follow the "internal vs external shell" argument - could you explain? Why can't we make this work with both internal _and_ external shell?

What if we put the output in shell comments so it has no effect when pasted into a terminal?

Here's an example of what I mean, but we can certainly tweak more:

echo hello world
# command stdout
# hello world
echo goodbye world
# command stdout
# goodbye world

I found I can copy and paste that entire text into a terminal, and just the actual command lines execute, as desired.

Personally, I mostly care about being able to copy & paste the command. "Script" (in the output) is nice as it makes finding RUN lines easy, but it's definitely not a show-stopper.

Would the above work for you?

I don't quite follow the "internal vs external shell" argument - could you explain? Why can't we make this work with both internal _and_ external shell?

To execute RUN lines, lit can use its own internal shell, which is written in python. As lit developers, we have complete control over how it formats execution traces.

Alternatively, lit can use external shells, such as cmd for windows or bash/sh for unix. In those cases, lit uses the external shell's debug features, such as @echo on or set -x. I just checked the sh standard, and I now see that the PS4 variable controls the prefix (defaults to +) printed before a command. If the format we settle on requires no changes beyond that, then we're probably fine. However, I don't yet know how to prevent it from breaking apart pipelines into separate commands. I don't yet know how to prevent it from mixing the commands and their stderr (where it prints the commands). I don't yet know how to control windows.

Here's an example of what I mean, but we can certainly tweak more:

Thanks for the example.

Would the above work for you?

Yes, that should work.

Alternatively, lit can use external shells, such as cmd for windows or bash/sh for unix.

Thanks for the explanation - now I understand what you were referring to.

I don't yet know how to prevent it from mixing the commands and their stderr (where it prints the commands).

I think that I have an example for that. For this "broken" version of RUN line from concatenate_dim_1.mlir:

<WORKDIR>build/release/bin/mlir-opt <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir --sparse-compiler=enableruntime-library=true | <WORKDIR>/build/release/bin/mlir-cpu-runner -e entry -entry-point-result=void -shared-libs=<WORKDIR>/build/release/lib/libmlir_c_runner_utils.so | <WORKDIR>/build/release/bin/FileCheck <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir

I get:

+ <WORKDIR>/build/release/bin/mlir-opt <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir --sparse-compiler=enableruntime-library=true
+ <WORKDIR>/build/release/bin/mlir-cpu-runner -e entry -entry-point-result=void -shared-libs=<WORKDIR>/build/release/lib/libmlir_c_runner_utils.so
+ <WORKDIR>/build/release/bin/FileCheck <WORKDIR>/mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_index.mlir
<Pass-Options-Parser>: no such option enableruntime-library
Error: entry point not found
FileCheck error: '<stdin>' is empty.

This output is quite noisy - it's the 1st command that fails, but the error doesn't get printed until after the 3rd command from the RUN line. I am not aware of any way to improve this. One could use BASH_XTRACEFD to redirect the debug output, but I'm not convinced that that would be an improvement.

Yes, that should work.

Thanks for confirming. If others here like that proposal and are ok with not addressing external shells, then I'll prototype it and start thinking about an RFC.

One could use BASH_XTRACEFD to redirect the debug output, but I'm not convinced that that would be an improvement.

It also seems limited to bash. For example, there is a path in lit where it uses /bin/sh, which on my linux box is dash, which is "POSIX-compliant", "small as possible", and not bash. And then there's also windows.

jdenny updated this revision to Diff 546980.Aug 3 2023, 1:07 PM

Rebased.

The RFC has been up for a week with no significant activity for several days. The response was very positive. Thanks for all the feedback so far in this review, which led to D156954 and thus helped to make the RFC a success.

I saw no feedback in the RFC that would impact this patch. I think this patch is ready for a formal review/accept.

MaskRay accepted this revision.Aug 25 2023, 11:03 AM
This revision is now accepted and ready to land.Aug 25 2023, 11:03 AM

I've gone ahead and accepted the patch. I agree with https://reviews.llvm.org/D156954#4617746 , especially this sentence

However, people seemed fine to wait to do that in a separate patch if an actual user who cares about that use case speaks up.

It seems much easier to have this patch in place so that people can check the changes and possibly suggest future improvement.
Making -vv usable is a significant improvement to both external and internal lit shell workflows.

Good idea. I'll make sure that's in their commit logs.

Thanks for reviewing. I'll plan to land these early next week when I can keep a closer eye on the bots.

awarzynski accepted this revision.Aug 27 2023, 7:52 AM

https://reviews.llvm.org/D156954 addresses all my concerns raised here and from the Discourse RFC we know that the community is in favor of this change as well. Thanks for all the great work, LGTM!

@awarzynski Thanks again for reviewing.

jdenny edited the summary of this revision. (Show Details)Aug 29 2023, 8:50 AM

Hi,

I'm not sure if this has been discussed already but I just noticed a change with this patch that at least makes it a bit annoying.
So before running

build-all//bin/llvm-lit -a test/Transforms/ArgumentPromotion/opaque-ptr.ll

yielded

: 'RUN: at line 2';   /repo/uabelho/dev-main/llvm/build-all/bin/opt -S -passes=argpromotion < /repo/uabelho/dev-main/llvm/test/Transforms/ArgumentPromotion/opaque-ptr.ll | /repo/uabelho/dev-main/llvm/build-all/bin/FileCheck /repo/uabelho/dev-main/llvm/test/Transforms/ArgumentPromotion/opaque-ptr.ll

which I could directly copy and paste into my terminal to reproduce the test.
Now I instead get

+ : 'RUN: at line 2'
+ /repo/uabelho/dev-main/llvm/build-all/bin/opt -S -passes=argpromotion
+ /repo/uabelho/dev-main/llvm/build-all/bin/FileCheck /repo/uabelho/dev-main/llvm/test/Transforms/ArgumentPromotion/opaque-ptr.ll

where the "opt" line doesn't even include the test file fed to opt via "< %s". Sure you see the file in the FileCheck command but before reproducing could be done with one single copy/paste, now it takes three.

This will be annoying every time a lit test fails and you want to reproduce it...

this has also broke my workflow, I use lit -a and copy/paste the command to shell to see the output or add different flags, it seems like a fundamental feature and I can't understand why this has been removed?

I'm not sure if this has been discussed already

Yes, it has. That was my original feedback for this change and in reply Joel has done 2 very helpful things:

Please be mindful that this change (as well as other LIT patches that are being worked on in tandem) bring various improvements to the overall LIT infrastructure. These things often require compromises. In the case of -a flag (and the "Script" block being removed), it means having to use LIT's internal shell. That was mentioned:

Tl;Dr Please try and see whether that is sufficient for you:

LIT_USE_INTERNAL_SHELL=1 path/to/llvm-lit ...

If not then I suggest taking this discussion back to Discourse. For better visibility.

jdenny added a comment.EditedAug 30 2023, 7:36 AM

I'm not sure if this has been discussed already

Yes, it has.

Thanks for explaining the situation.

@uabelho, @c-rhodes The goal is to make lit more usable. I apologize if it has broken your workflow despite all the efforts to avoid doing so. There are multiple ways we might think to improve your situation, so please do continue the discourse discussion as @awarzynski suggested (unless his LIT_USE_INTERNAL_SHELL=1 suggestion is sufficient). Thanks.

I'm not sure if this has been discussed already

Yes, it has. That was my original feedback for this change and in reply Joel has done 2 very helpful things:

Please be mindful that this change (as well as other LIT patches that are being worked on in tandem) bring various improvements to the overall LIT infrastructure. These things often require compromises. In the case of -a flag (and the "Script" block being removed), it means having to use LIT's internal shell. That was mentioned:

Tl;Dr Please try and see whether that is sufficient for you:

LIT_USE_INTERNAL_SHELL=1 path/to/llvm-lit ...

If not then I suggest taking this discussion back to Discourse. For better visibility.

Thanks! I had no idea about this lit internal shell thing.
So, using

LIT_USE_INTERNAL_SHELL=1 path/to/llvm-lit ...

makes it look nice. Thanks for pointing this out!

Next question then would be why isn't LIT_USE_INTERNAL_SHELL=1 the default. But yeah I saw from the RFC thread not all tests work with that.

jdenny updated this revision to Diff 556792.EditedSep 14 2023, 9:37 AM

Rebased onto today's main. This brings in PR #65242, which included some code to accommodate the Script: section. The current patch eliminates the Script: section, so this update cleans up that code. Otherwise, the current patch hasn't changed much.

I don't think this patch requires another review before I land it, but of course reviewers are welcome to comment. I will wait to re-land it along with PR #65267 (edit: that's now PR #66408), which still requires more work and review.