[mlir][polly][llvm-lit] Fixed logic for turning on external shell in lit

[connieyzhu]

For both mlir and polly, the lit internal shell is the default shell for running lit tests. However, if the user wanted to switch back to the external shell by setting LIT_USE_INTERNAL_SHELL=0, the not used in the body of the if conditional changes use_lit_shell to be True instead of the intended False. Removing not allows for this lit config to work as intended.

Fixes #106459.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Connie Zhu (connieyzhu)

Changes

For both mlir and polly, the lit internal shell is the default shell for running lit tests. However, if the user wanted to switch back to the external shell by setting LIT_USE_INTERNAL_SHELL=0, the not used in the body of the if conditional changes use_lit_shell to be True instead of the intended False. Removing not allows for this lit config to work as intended.

---

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

3 Files Affected:

• (modified) mlir/test/lit.cfg.py (+1-1)
• (modified) polly/test/UnitIsl/lit.cfg (+1-1)
• (modified) polly/test/lit.cfg (+1-1)

diff --git a/mlir/test/lit.cfg.py b/mlir/test/lit.cfg.py
index 98d0ddd9a2be11..930e4eb0aed68f 100644
--- a/mlir/test/lit.cfg.py
+++ b/mlir/test/lit.cfg.py
@@ -23,7 +23,7 @@
 use_lit_shell = True
 lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")
 if lit_shell_env:
-  use_lit_shell = not lit.util.pythonize_bool(lit_shell_env)
+    use_lit_shell = lit.util.pythonize_bool(lit_shell_env)
 
 config.test_format = lit.formats.ShTest(execute_external=not use_lit_shell)
 
diff --git a/polly/test/UnitIsl/lit.cfg b/polly/test/UnitIsl/lit.cfg
index 0944d543572d86..d91edfbaf4f944 100644
--- a/polly/test/UnitIsl/lit.cfg
+++ b/polly/test/UnitIsl/lit.cfg
@@ -22,7 +22,7 @@ config.name = 'Polly - isl unit tests'
 use_lit_shell = True
 lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")
 if lit_shell_env:
-  use_lit_shell = not lit.util.pythonize_bool(lit_shell_env)
+	use_lit_shell = lit.util.pythonize_bool(lit_shell_env)
 
 config.test_format = lit.formats.ShTest(execute_external=not use_lit_shell)
 
diff --git a/polly/test/lit.cfg b/polly/test/lit.cfg
index 156c1f97f5d3ae..c3f984b5c7f127 100644
--- a/polly/test/lit.cfg
+++ b/polly/test/lit.cfg
@@ -25,7 +25,7 @@ config.name = 'Polly'
 use_lit_shell = True
 lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")
 if lit_shell_env:
-  use_lit_shell = not lit.util.pythonize_bool(lit_shell_env)
+    use_lit_shell = lit.util.pythonize_bool(lit_shell_env)
 
 config.test_format = lit.formats.ShTest(execute_external=not use_lit_shell)
 

[boomanaiden154]

LGTM.

It seems like the two instances in polly were copied from the MLIR one and the MLIR version was inverted for whatever reason?

Given this seems to impact all the test suites where the internal shell is getting enabled, is it possible/a good idea to move the use of the environment variable to within lit (maybe as a follow up)?

[ilovepi]

The current RFC and plans are to incrementally adopt the internal shell as the default. The main lit config in llvm handles this correctly, but there is usually some special handling for Windows duplicated across the various subprojects. MLIR already had set it’s default for the internal shell, but this is rare for any of the subprojects to do outside of Windows. Polly had no tests with either non-portable syntax or REQUIRES: shell, so it wasn’t noticed that it didn’t work as intended when we adopted/copied the MLIR implementation.

I’m not sure there’s a good way to share the implementation settings between subprojects without using the same settings for all of them. Using the shared implementation may need to wait until we’re ready to switch them all.

[boomanaiden154]

I’m not sure there’s a good way to share the implementation settings between subprojects without using the same settings for all of them. Using the shared implementation may need to wait until we’re ready to switch them all.

Ack. Maybe some TODOs so it's more likely to get picked up in the future?

[ilovepi]

Yeah that’s likely the best idea. Or at least a tracking issue.

[connieyzhu]

Yeah that’s likely the best idea. Or at least a tracking issue.

@boomanaiden154 @ilovepi I added TODOs in my most recent commit and created an issue for this.

[ilovepi]

Thanks for filing the issue. LGTM.