Skip to content

[MLIR] Fix inverted logic of LIT_USE_INTERNAL_SHELL #105483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Aug 21, 2024

The value of LIT_USE_INTERNAL_SHELL is inverted although it is not supposed to. The inversion was introduced in #65415.

Assume the environment variable LIT_USE_INTERNAL_SHELL=0 (i.e. use /bin/sh) is set:

use_lit_shell = True                                          # Use internal shell
lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")      # lit_shell_env = "0", i.e. use /bin/sh
if lit_shell_env:
  use_lit_shell = not lit.util.pythonize_bool(lit_shell_env)  # use_lit_shell = not bool("0") = True, i.e. use internal shell

config.test_format = lit.formats.ShTest(execute_external=not use_lit_shell) # execute_external = not True = False, i.e. use internal shell even though we explicitly switched off using `LIT_USE_INTERNAL_SHELL=0`.

@Meinersbur Meinersbur requested a review from joker-eph August 21, 2024 08:36
@llvmbot llvmbot added the mlir label Aug 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 21, 2024

@llvm/pr-subscribers-mlir

Author: Michael Kruse (Meinersbur)

Changes

The value of LIT_USE_INTERNAL_SHELL is inverted although it is not supposed to. The inversion was introduced in #65415.

Assume the environment variable LIT_USE_INTERNAL_SHELL=0 (i.e. use /bin/sh) is set:

use_lit_shell = True                                          # Use internal shell
lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")      # lit_shell_env = "0", i.e. use /bin/sh
if lit_shell_env:
  use_lit_shell = not lit.util.pythonize_bool(lit_shell_env)  # use_lit_shell = not bool("0") = True, i.e. use internal shell

config.test_format = lit.formats.ShTest(execute_external=not use_lit_shell) # execute_external = not True = False, i.e. use internal shell even though we explicitly switched off `LIT_USE_INTERNAL_SHELL`.

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

1 Files Affected:

  • (modified) mlir/test/lit.cfg.py (+1-1)
diff --git a/mlir/test/lit.cfg.py b/mlir/test/lit.cfg.py
index 98d0ddd9a2be11..81a668e73d4b24 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)
 

Copy link

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r 50f4168e40790bd91123824ee338643ac18ccc0b...d2de4b855c0f56c9700e2841abdb99cc98daa7bb mlir/test/lit.cfg.py
View the diff from darker here.
--- lit.cfg.py	2024-08-21 08:22:52.000000 +0000
+++ lit.cfg.py	2024-08-21 08:39:59.313207 +0000
@@ -21,11 +21,11 @@
 # We prefer the lit internal shell which provides a better user experience on failures
 # unless the user explicitly disables it with LIT_USE_INTERNAL_SHELL=0 env var.
 use_lit_shell = True
 lit_shell_env = os.environ.get("LIT_USE_INTERNAL_SHELL")
 if lit_shell_env:
-  use_lit_shell = 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)
 
 # suffixes: A list of file extensions to treat as test files.
 config.suffixes = [

@Meinersbur
Copy link
Member Author

Deprecated by 18e5505

@Meinersbur Meinersbur closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants