Skip to content

[clang][test] Rewrote test using command substitution to work with lit internal shell syntax #105902

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

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

connieyzhu
Copy link
Contributor

This patch rewrites a test that uses command substitution $() and the stat command, which are not supported by lit's internal shell. Instead of using this syntax to perform the file size comparison done in this test, a Python script is used instead to perform the same operation.

Fixes #102384.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Aug 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2024

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Connie Zhu (connieyzhu)

Changes

This patch rewrites a test that uses command substitution $() and the stat command, which are not supported by lit's internal shell. Instead of using this syntax to perform the file size comparison done in this test, a Python script is used instead to perform the same operation.

Fixes #102384.


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

3 Files Affected:

  • (added) clang/test/Modules/compare-file-size.py (+22)
  • (modified) clang/test/Modules/reduced-bmi-size.cppm (+1-2)
  • (modified) clang/test/lit.cfg.py (+2)
diff --git a/clang/test/Modules/compare-file-size.py b/clang/test/Modules/compare-file-size.py
new file mode 100644
index 00000000000000..b5dc20642ea6ce
--- /dev/null
+++ b/clang/test/Modules/compare-file-size.py
@@ -0,0 +1,22 @@
+import argparse
+import os
+
+def get_file_size(file_path):
+    try:
+        return os.path.getsize(file_path)
+    except:
+        print(f"Unable to get file size of {file_path}")
+        return None
+
+def main():
+    parser = argparse.ArgumentParser()
+
+    parser.add_argument("file1", type=str)
+    parser.add_argument("file2", type=str)
+
+    args = parser.parse_args()
+
+    return get_file_size(args.file1) < get_file_size(args.file2)
+
+if __name__ ==  "__main__":
+    main()
diff --git a/clang/test/Modules/reduced-bmi-size.cppm b/clang/test/Modules/reduced-bmi-size.cppm
index 664f45f5c6a5a7..6d62573bc7aa39 100644
--- a/clang/test/Modules/reduced-bmi-size.cppm
+++ b/clang/test/Modules/reduced-bmi-size.cppm
@@ -10,7 +10,6 @@
 // RUN: %clang_cc1 -std=c++20 -emit-module-interface %s -o %t/a.pcm
 // RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %s -o %t/a.reduced.pcm
 //
-// %s implies the current source file. So we can't use it directly.
-// RUN: [ $(stat -c%\s "%t/a.pcm") -le $(stat -c%\s "%t/a.reduced.pcm") ]
+// RUN: %{python} %S/compare-file-size.py %t/a.pcm %t/a.reduced.pcm
 
 export module a;
diff --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 92a3361ce672e2..59330a6d51a611 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -74,6 +74,8 @@
 
 config.substitutions.append(("%PATH%", config.environment["PATH"]))
 
+config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))
+
 
 # For each occurrence of a clang tool name, replace it with the full path to
 # the build directory holding that tool.  We explicitly specify the directories

Copy link

github-actions bot commented Aug 23, 2024

✅ With the latest revision this PR passed the Python code formatter.

This patch rewrites a test that uses command substitution $() and the
stat command, which are not supported by lit's internal shell. Instead
of using this syntax to perform the file size comparison done in this
test, a Python script is used instead to perform the same operation.
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on my side. Thanks. But I didn't take a detailed look into https://discourse.llvm.org/t/rfc-enabling-the-lit-internal-shell-by-default/80179/29. So it might be good to wait for people there to approve this

to get file size

This patch removes the print statement that executes in the case of
exceptions, opting to let the system return its own error code when
failing to get the file size of a certain file. There are also some NFC
changes: adding description for compare-file-size.py and changing the
%{python} syntax to use the exisitng lit substitution %python.
@connieyzhu connieyzhu changed the title [clang][test] Rewrote test to work with lit internal shell syntax [clang][test] Rewrote test using $() to work with lit internal shell syntax Aug 26, 2024
@connieyzhu connieyzhu changed the title [clang][test] Rewrote test using $() to work with lit internal shell syntax [clang][test] Rewrote test using command substitution to work with lit internal shell syntax Aug 29, 2024
@connieyzhu connieyzhu merged commit 4caf019 into llvm:main Aug 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[llvm-lit] lit internal shell failing to parse and execute command substitution syntax
5 participants