tests: Wrapped all %s that refer to paths with "

[watermelonwolverine]

See issue #2598

Replaced all %s with "%s" inside .mlir, .toy and some .py.
Wrapped some paths with " in lit.cfg

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.60%. Comparing base (552231f) to head (bc7c4dd).
Report is 22 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2628   +/-   ##
=======================================
  Coverage   89.60%   89.60%           
=======================================
  Files         358      358           
  Lines       45714    45714           
  Branches     6900     6900           
=======================================
  Hits        40962    40962           
  Misses       3697     3697           
  Partials     1055     1055           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superlopuh]

Suggested change

	config.substitutions.append(('XDSL_ROUNDTRIP', "xdsl-opt \"%s\" --print-op-generic --split-input-file | xdsl-opt --split-input-file | filecheck \"%s\""))
	config.substitutions.append(('XDSL_ROUNDTRIP', 'xdsl-opt "%s" --print-op-generic --split-input-file | xdsl-opt --split-input-file | filecheck "%s"'))

[superlopuh]

Suggested change

	config.substitutions.append(("XDSL_GENERIC_ROUNDTRIP", "xdsl-opt \"%s\" --print-op-generic --split-input-file | filecheck \"%s\" --check-prefix=CHECK-GENERIC"))
	config.substitutions.append(("XDSL_GENERIC_ROUNDTRIP", 'xdsl-opt "%s" --print-op-generic --split-input-file | filecheck "%s" --check-prefix=CHECK-GENERIC'))

[superlopuh]

Looks good! Small suggestion to avoid escaping, LGTM otherwise

[math-fehr]

Looking at the lit.cfg file, it seems that we can actually do that automatically without having to change it for every file!

Adding this line: config.substitutions.append(('%s', '"%s"')) in tests/filecheck/lit.cfg seems to add the quotes to all %s, so we won't have to 1) change all of them and 2) keep reminding ourselves that it is necessary (I fear we will often forget them when copy-pasting a test from LLVM)

Would that make more sense?
For non-lit tests, we still have to add the quotes though

[georgebisbas]

Looking at the lit.cfg file, it seems that we can actually do that automatically without having to change it for every file!

Adding this line: config.substitutions.append(('%s', '"%s"')) in tests/filecheck/lit.cfg seems to add the quotes to all %s, so we won't have to 1) change all of them and 2) keep reminding ourselves that it is necessary (I fear we will often forget them when copy-pasting a test from LLVM)

Would that make more sense? For non-lit tests, we still have to add the quotes though

This looks like a better solution, I agree.

[georgebisbas]

What was the motivation behind that change?

[watermelonwolverine]

Looking at the lit.cfg file, it seems that we can actually do that automatically without having to change it for every file!

Adding this line: config.substitutions.append(('%s', '"%s"')) in tests/filecheck/lit.cfg seems to add the quotes to all %s, so we won't have to 1) change all of them and 2) keep reminding ourselves that it is necessary (I fear we will often forget them when copy-pasting a test from LLVM)

Would that make more sense? For non-lit tests, we still have to add the quotes though

Personally for me this gets a bit too close to magic. Magic in the sense that a shell command that should not work is working or it is working in one situation but not in another situation (e.g. for non-lit tests). This confusion can be quickly resolved, but I think it would be better if it couldn't happen in the first place.

[watermelonwolverine]

What was the motivation behind that change?

See issue: #2598: If the absolute path of the project directory contains a space make filecheck fails.

[math-fehr]

Personally for me this gets a bit too close to magic. Magic in the sense that a shell command that should not work is working or it is working in one situation but not in another situation (e.g. for non-lit tests). This confusion can be quickly resolved, but I think it would be better if it couldn't happen in the first place.

I am not sure I understand what you mean here.
Here, wrapping %s in quotes is just to fix the lack of escaping done by lit. To me, the long term solution is to fix lit directly. I checked what happens in LLVM, and lit also fails if there is a space in the path leading to the repository.

What I'm worried about is that people will copy-paste their lit tests from something like MLIR, and forget to escape the %s. Fixing this at the configuration level will ensure that this is not forgotten, and also has the advantage of not changing
all the tests, which I think is important as well.

[watermelonwolverine]

This is a bit philosophical and this is also not a hill I'm willing to die on so feel free to solve this problem the way you suggested /gen.

I think code should never lie. That's the reason why singletons are an anti-pattern, because they make code lie. The same is true for configurations that change the way code is interpreted, they also make the code lie. It might be a small thing in this case, especially for someone who knows about the configuration. But small things add up and eventually become an entry barrier or a configuration/dependency hell, something this project is trying to take away from MLIR if I understood correctly.

A little less philosophical and more practical: This correct code will be incorrect after the substitution:

// RUN: xdsl-opt "%s" --print-op-generic | filecheck "%s"

as it gets replaced with:

// RUN: xdsl-opt ""%s"" --print-op-generic | filecheck ""%s""

Again, a bit more philosophical:

Quoting variables is universally a good practice in shell scripts to prevent word splitting and globbing: https://www.shellcheck.net/wiki/SC2086
Adding a substitution via the lit.cfg file would make this good practice into a bad practice. An outside programmer coming into this project would first need to learn about this fact, coming back to the entry barrier thing I mentioned.

Back to a little more practical reasons:

You would need to add this substitution for every substitution there is if you want to cover all future cases: https://llvm.org/docs/CommandGuide/lit.html#substitutions

All that said, I totally acknowledge that there is always a trade-off between practicality and "cleanliness" (if there even is such a thing). I see that in this case the practical solution might be the better one.

[math-fehr]

I wrote an implementation that changes the configuration file directly, so we do the substitution with escaping of spaces ourselves.
Tell me if you think this is reasonnable: #2671

This is fixing lit issue of spaces, and while I think this should be done in lit itself, I do not currently have the time to fix it there, especially if this will happen to be a controversial change.

[superlopuh]

Hi @watermelonwolverine, I think the best course of action would be to fix lit instead of xDSL, since it's an external dependency. I poked around, and it looks like it's doable, and someone's already done it, but I'm not sure how much work it would be to port the changes and get them approved on the LLVM side.

Here are some resources about this:
https://discourse.llvm.org/t/spaces-in-path-with-lit-tests/79283
https://reviews.llvm.org/D29185

I'll close this for now, since I don't think we want to make this change in the tests for the time being.