Skip to content

[clang][python][test] Move python binding tests to lit framework #142948

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rorth
Copy link
Collaborator

@rorth rorth commented Jun 5, 2025

As discussed in PR #142353, the current testsuite of the clang Python bindings has several issues:

  • It libclang.so cannot be loaded into python to run the testsuite, the whole ninja check-all aborts.
  • The result of running the testsuite isn't report like the lit-based tests, rendering them almost invisible.
  • The testsuite is disabled in a non-obvious way (RUN_PYTHON_TESTS) in tests/CMakeLists.txt, which again doesn't show up in the test results.

All these issues can be avoided by integrating the Python bindings tests with lit, which is what this patch does:

  • The actual test lives in clang/test/bindings/python/bindings.sh and is run by lit.
  • The current clang/bindings/python/tests directory (minus the now-subperfluous CMakeLists.txt) is moved into the same directory.
  • The check if libclang is loadable (originally from PR [clang][python][test] Check if libclang.so is loadable #142353) is now handled via a new lit feature, libclang-loadable.
  • The various ways to disable the tests have been turned into XFAILs as appropriate. This isn't complete and not completely tested yet.

Tested on sparc-sun-solaris2.11, sparcv9-sun-solaris2.11, i386-pc-solaris2.11, amd64-pc-solaris2.11, i686-pc-linux-gnu, and x86_64-pc-linux-gnu.

As discussed in PR llvm#142353, the current testsuite of the `clang` Python
bindings has several issues:

- It `libclang.so` cannot be loaded into `python` to run the testsuite, the
  whole `ninja check-all` aborts.
- The result of running the testsuite isn't report like the `lit`-based
  tests, rendering them almost invisible.
- The testsuite is disabled in a non-obvious way (`RUN_PYTHON_TESTS`) in
  `tests/CMakeLists.txt`, which again doesn't show up in the test results.

All these issues can be avoided by integrating the Python bindings tests
with `lit`, which is what this patch does:

- The actual test lives in `clang/test/bindings/python/bindings.sh` and is
  run by `lit`.
- The current `clang/bindings/python/tests` directory (minus the
  now-subperfluous `CMakeLists.txt`) is moved into the same directory.
- The check if `libclang` is loadable (originally from PR llvm#142353) is now
  handled via a new `lit` feature, `libclang-loadable`.
- The various ways to disable the tests have been turned into `XFAIL`s as
  appropriate.  This isn't complete and not completely tested yet.

Tested on `sparc-sun-solaris2.11`, `sparcv9-sun-solaris2.11`,
`i386-pc-solaris2.11`, `amd64-pc-solaris2.11`, `i686-pc-linux-gnu`, and
`x86_64-pc-linux-gnu`.
@rorth rorth requested a review from Endilll June 5, 2025 11:43
@rorth rorth requested a review from DeinAlptraum as a code owner June 5, 2025 11:43
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:as-a-library libclang and C++ API labels Jun 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 5, 2025

@llvm/pr-subscribers-github-workflow

@llvm/pr-subscribers-clang

Author: Rainer Orth (rorth)

Changes

As discussed in PR #142353, the current testsuite of the clang Python bindings has several issues:

  • It libclang.so cannot be loaded into python to run the testsuite, the whole ninja check-all aborts.
  • The result of running the testsuite isn't report like the lit-based tests, rendering them almost invisible.
  • The testsuite is disabled in a non-obvious way (RUN_PYTHON_TESTS) in tests/CMakeLists.txt, which again doesn't show up in the test results.

All these issues can be avoided by integrating the Python bindings tests with lit, which is what this patch does:

  • The actual test lives in clang/test/bindings/python/bindings.sh and is run by lit.
  • The current clang/bindings/python/tests directory (minus the now-subperfluous CMakeLists.txt) is moved into the same directory.
  • The check if libclang is loadable (originally from PR #142353) is now handled via a new lit feature, libclang-loadable.
  • The various ways to disable the tests have been turned into XFAILs as appropriate. This isn't complete and not completely tested yet.

Tested on sparc-sun-solaris2.11, sparcv9-sun-solaris2.11, i386-pc-solaris2.11, amd64-pc-solaris2.11, i686-pc-linux-gnu, and x86_64-pc-linux-gnu.


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

38 Files Affected:

  • (modified) clang/CMakeLists.txt (-1)
  • (removed) clang/bindings/python/tests/CMakeLists.txt (-66)
  • (added) clang/test/bindings/python/bindings.sh (+48)
  • (added) clang/test/bindings/python/lit.local.cfg (+22)
  • (renamed) clang/test/bindings/python/tests/init.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/a.inc ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/b.inc ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/compile_commands.json ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/header1.h ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/header2.h ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/header3.h ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/hello.cpp ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/include.cpp ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/parse_arguments.c ()
  • (renamed) clang/test/bindings/python/tests/cindex/INPUTS/testfile.c ()
  • (renamed) clang/test/bindings/python/tests/cindex/init.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_access_specifiers.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_cdb.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_code_completion.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_comment.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_cursor.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_cursor_kind.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_diagnostics.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_enums.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_exception_specification_kind.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_file.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_index.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_lib.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_linkage.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_location.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_rewrite.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_source_range.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_tls_kind.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_token_kind.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_tokens.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_translation_unit.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/test_type.py ()
  • (renamed) clang/test/bindings/python/tests/cindex/util.py ()
diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index ab2ac9bc6b9ad..5111953397d04 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -533,7 +533,6 @@ if( CLANG_INCLUDE_TESTS )
     clang_unit_site_config=${CMAKE_CURRENT_BINARY_DIR}/test/Unit/lit.site.cfg
   )
   add_subdirectory(test)
-  add_subdirectory(bindings/python/tests)
 
   if(CLANG_BUILT_STANDALONE)
     umbrella_lit_testsuite_end(check-all)
diff --git a/clang/bindings/python/tests/CMakeLists.txt b/clang/bindings/python/tests/CMakeLists.txt
deleted file mode 100644
index a0ddabc21bb41..0000000000000
--- a/clang/bindings/python/tests/CMakeLists.txt
+++ /dev/null
@@ -1,66 +0,0 @@
-# Test target to run Python test suite from main build.
-
-# Avoid configurations including '-include' from interfering with
-# our tests by setting CLANG_NO_DEFAULT_CONFIG.
-add_custom_target(check-clang-python
-    COMMAND ${CMAKE_COMMAND} -E env
-            CLANG_NO_DEFAULT_CONFIG=1
-            CLANG_LIBRARY_PATH=$<TARGET_FILE_DIR:libclang>
-            "${Python3_EXECUTABLE}" -m unittest discover
-    DEPENDS libclang
-    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/..)
-
-set(RUN_PYTHON_TESTS TRUE)
-set_target_properties(check-clang-python PROPERTIES FOLDER "Clang/Tests")
-
-# Tests require libclang.so which is only built with LLVM_ENABLE_PIC=ON
-if(NOT LLVM_ENABLE_PIC)
-  set(RUN_PYTHON_TESTS FALSE)
-endif()
-
-# Do not try to run if libclang was built with sanitizers because
-# the sanitizer library will likely be loaded too late to perform
-# interception and will then fail.
-# We could use LD_PRELOAD/DYLD_INSERT_LIBRARIES but this isn't
-# portable so its easier just to not run the tests when building
-# with ASan.
-if(NOT LLVM_USE_SANITIZER STREQUAL "")
-  set(RUN_PYTHON_TESTS FALSE)
-endif()
-
-# Tests fail on Windows, and need someone knowledgeable to fix.
-# It's not clear whether it's a test or a valid binding problem.
-if(WIN32)
-  set(RUN_PYTHON_TESTS FALSE)
-endif()
-
-# The Python FFI interface is broken on AIX: https://bugs.python.org/issue38628.
-if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
-  set(RUN_PYTHON_TESTS FALSE)
-endif()
-
-# AArch64, Hexagon, and Sparc have known test failures that need to be
-# addressed.
-# SystemZ has broken Python/FFI interface:
-# https://reviews.llvm.org/D52840#1265716
-if(${LLVM_NATIVE_ARCH} MATCHES "^(AArch64|Hexagon|Sparc|SystemZ)$")
-  set(RUN_PYTHON_TESTS FALSE)
-endif()
-
-# Tests will fail if cross-compiling for a different target, as tests will try
-# to use the host Python3_EXECUTABLE and make FFI calls to functions in target
-# libraries.
-if(CMAKE_CROSSCOMPILING)
-  # FIXME: Consider a solution that allows better control over these tests in
-  # a crosscompiling scenario. e.g. registering them with lit to allow them to
-  # be explicitly skipped via appropriate LIT_ARGS, or adding a mechanism to
-  # allow specifying a python interpreter compiled for the target that could
-  # be executed using qemu-user.
-  message(WARNING "check-clang-python not added to check-all as these tests fail in a cross-build setup")
-  set(RUN_PYTHON_TESTS FALSE)
-endif()
-
-if(RUN_PYTHON_TESTS)
-    set_property(GLOBAL APPEND PROPERTY
-                 LLVM_ALL_ADDITIONAL_TEST_TARGETS check-clang-python)
-endif()
diff --git a/clang/test/bindings/python/bindings.sh b/clang/test/bindings/python/bindings.sh
new file mode 100755
index 0000000000000..ec4ca55e41822
--- /dev/null
+++ b/clang/test/bindings/python/bindings.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+# UNSUPPORTED: !libclang-loadable
+
+# Tests require libclang.so which is only built with LLVM_ENABLE_PIC=ON
+#
+# Covered by libclang-loadable, may need to augment test for lack of
+# libclang.so.
+
+# Do not try to run if libclang was built with sanitizers because
+# the sanitizer library will likely be loaded too late to perform
+# interception and will then fail.
+# We could use LD_PRELOAD/DYLD_INSERT_LIBRARIES but this isn't
+# portable so its easier just to not run the tests when building
+# with ASan.
+#
+# FIXME: Handle !LLVM_USE_SANITIZER = "".
+# lit.site.cfg.py has config.llvm_use_sanitizer = ""
+
+# Tests fail on Windows, and need someone knowledgeable to fix.
+# It's not clear whether it's a test or a valid binding problem.
+# XFAIL: target={{.*windows.*}}
+
+# The Python FFI interface is broken on AIX: https://bugs.python.org/issue38628.
+# XFAIL: target={{.*-aix.*}}
+
+# AArch64, Hexagon, and Sparc have known test failures that need to be
+# addressed.
+# SystemZ has broken Python/FFI interface:
+# https://reviews.llvm.org/D52840#1265716
+# XFAIL: target={{(aarch64|hexagon|sparc*|s390x)-.*}}
+
+# Tests will fail if cross-compiling for a different target, as tests will try
+# to use the host Python3_EXECUTABLE and make FFI calls to functions in target
+# libraries.
+#
+# FIXME: Consider a solution that allows better control over these tests in
+# a crosscompiling scenario. e.g. registering them with lit to allow them to
+# be explicitly skipped via appropriate LIT_ARGS, or adding a mechanism to
+# allow specifying a python interpreter compiled for the target that could
+# be executed using qemu-user.
+#
+# FIXME: Handle CMAKE_CROSSCOMPILING.
+# Again, might already be handled by libclang-loadable.
+
+# RUN: env PYTHONPATH=%S/../../../bindings/python \
+# RUN:   CLANG_LIBRARY_PATH=`llvm-config --libdir` \
+# RUN:   %python -m unittest discover -s %S/tests
diff --git a/clang/test/bindings/python/lit.local.cfg b/clang/test/bindings/python/lit.local.cfg
new file mode 100644
index 0000000000000..d3608565f5aef
--- /dev/null
+++ b/clang/test/bindings/python/lit.local.cfg
@@ -0,0 +1,22 @@
+def is_libclang_loadable():
+    try:
+        sys.path.append(os.path.join(config.clang_src_dir, "bindings/python"))
+        from clang.cindex import Config
+        conf = Config()
+        Config.set_library_path(config.clang_lib_dir)
+        conf.lib
+        return True
+    except Exception as e:
+        # Benign error modes.
+        if "wrong ELF class: ELFCLASS32" in str(e):
+            return False
+        elif "No such file or directory" in str(e):
+            return False
+        # Unknown error modes.
+        else:
+            return True
+
+if is_libclang_loadable():
+    config.available_features.add("libclang-loadable")
+
+config.suffixes = ['.sh']
diff --git a/clang/bindings/python/tests/__init__.py b/clang/test/bindings/python/tests/__init__.py
similarity index 100%
rename from clang/bindings/python/tests/__init__.py
rename to clang/test/bindings/python/tests/__init__.py
diff --git a/clang/bindings/python/tests/cindex/INPUTS/a.inc b/clang/test/bindings/python/tests/cindex/INPUTS/a.inc
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/a.inc
rename to clang/test/bindings/python/tests/cindex/INPUTS/a.inc
diff --git a/clang/bindings/python/tests/cindex/INPUTS/b.inc b/clang/test/bindings/python/tests/cindex/INPUTS/b.inc
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/b.inc
rename to clang/test/bindings/python/tests/cindex/INPUTS/b.inc
diff --git a/clang/bindings/python/tests/cindex/INPUTS/compile_commands.json b/clang/test/bindings/python/tests/cindex/INPUTS/compile_commands.json
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/compile_commands.json
rename to clang/test/bindings/python/tests/cindex/INPUTS/compile_commands.json
diff --git a/clang/bindings/python/tests/cindex/INPUTS/header1.h b/clang/test/bindings/python/tests/cindex/INPUTS/header1.h
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/header1.h
rename to clang/test/bindings/python/tests/cindex/INPUTS/header1.h
diff --git a/clang/bindings/python/tests/cindex/INPUTS/header2.h b/clang/test/bindings/python/tests/cindex/INPUTS/header2.h
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/header2.h
rename to clang/test/bindings/python/tests/cindex/INPUTS/header2.h
diff --git a/clang/bindings/python/tests/cindex/INPUTS/header3.h b/clang/test/bindings/python/tests/cindex/INPUTS/header3.h
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/header3.h
rename to clang/test/bindings/python/tests/cindex/INPUTS/header3.h
diff --git a/clang/bindings/python/tests/cindex/INPUTS/hello.cpp b/clang/test/bindings/python/tests/cindex/INPUTS/hello.cpp
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/hello.cpp
rename to clang/test/bindings/python/tests/cindex/INPUTS/hello.cpp
diff --git a/clang/bindings/python/tests/cindex/INPUTS/include.cpp b/clang/test/bindings/python/tests/cindex/INPUTS/include.cpp
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/include.cpp
rename to clang/test/bindings/python/tests/cindex/INPUTS/include.cpp
diff --git a/clang/bindings/python/tests/cindex/INPUTS/parse_arguments.c b/clang/test/bindings/python/tests/cindex/INPUTS/parse_arguments.c
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/parse_arguments.c
rename to clang/test/bindings/python/tests/cindex/INPUTS/parse_arguments.c
diff --git a/clang/bindings/python/tests/cindex/INPUTS/testfile.c b/clang/test/bindings/python/tests/cindex/INPUTS/testfile.c
similarity index 100%
rename from clang/bindings/python/tests/cindex/INPUTS/testfile.c
rename to clang/test/bindings/python/tests/cindex/INPUTS/testfile.c
diff --git a/clang/bindings/python/tests/cindex/__init__.py b/clang/test/bindings/python/tests/cindex/__init__.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/__init__.py
rename to clang/test/bindings/python/tests/cindex/__init__.py
diff --git a/clang/bindings/python/tests/cindex/test_access_specifiers.py b/clang/test/bindings/python/tests/cindex/test_access_specifiers.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_access_specifiers.py
rename to clang/test/bindings/python/tests/cindex/test_access_specifiers.py
diff --git a/clang/bindings/python/tests/cindex/test_cdb.py b/clang/test/bindings/python/tests/cindex/test_cdb.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_cdb.py
rename to clang/test/bindings/python/tests/cindex/test_cdb.py
diff --git a/clang/bindings/python/tests/cindex/test_code_completion.py b/clang/test/bindings/python/tests/cindex/test_code_completion.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_code_completion.py
rename to clang/test/bindings/python/tests/cindex/test_code_completion.py
diff --git a/clang/bindings/python/tests/cindex/test_comment.py b/clang/test/bindings/python/tests/cindex/test_comment.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_comment.py
rename to clang/test/bindings/python/tests/cindex/test_comment.py
diff --git a/clang/bindings/python/tests/cindex/test_cursor.py b/clang/test/bindings/python/tests/cindex/test_cursor.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_cursor.py
rename to clang/test/bindings/python/tests/cindex/test_cursor.py
diff --git a/clang/bindings/python/tests/cindex/test_cursor_kind.py b/clang/test/bindings/python/tests/cindex/test_cursor_kind.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_cursor_kind.py
rename to clang/test/bindings/python/tests/cindex/test_cursor_kind.py
diff --git a/clang/bindings/python/tests/cindex/test_diagnostics.py b/clang/test/bindings/python/tests/cindex/test_diagnostics.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_diagnostics.py
rename to clang/test/bindings/python/tests/cindex/test_diagnostics.py
diff --git a/clang/bindings/python/tests/cindex/test_enums.py b/clang/test/bindings/python/tests/cindex/test_enums.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_enums.py
rename to clang/test/bindings/python/tests/cindex/test_enums.py
diff --git a/clang/bindings/python/tests/cindex/test_exception_specification_kind.py b/clang/test/bindings/python/tests/cindex/test_exception_specification_kind.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_exception_specification_kind.py
rename to clang/test/bindings/python/tests/cindex/test_exception_specification_kind.py
diff --git a/clang/bindings/python/tests/cindex/test_file.py b/clang/test/bindings/python/tests/cindex/test_file.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_file.py
rename to clang/test/bindings/python/tests/cindex/test_file.py
diff --git a/clang/bindings/python/tests/cindex/test_index.py b/clang/test/bindings/python/tests/cindex/test_index.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_index.py
rename to clang/test/bindings/python/tests/cindex/test_index.py
diff --git a/clang/bindings/python/tests/cindex/test_lib.py b/clang/test/bindings/python/tests/cindex/test_lib.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_lib.py
rename to clang/test/bindings/python/tests/cindex/test_lib.py
diff --git a/clang/bindings/python/tests/cindex/test_linkage.py b/clang/test/bindings/python/tests/cindex/test_linkage.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_linkage.py
rename to clang/test/bindings/python/tests/cindex/test_linkage.py
diff --git a/clang/bindings/python/tests/cindex/test_location.py b/clang/test/bindings/python/tests/cindex/test_location.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_location.py
rename to clang/test/bindings/python/tests/cindex/test_location.py
diff --git a/clang/bindings/python/tests/cindex/test_rewrite.py b/clang/test/bindings/python/tests/cindex/test_rewrite.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_rewrite.py
rename to clang/test/bindings/python/tests/cindex/test_rewrite.py
diff --git a/clang/bindings/python/tests/cindex/test_source_range.py b/clang/test/bindings/python/tests/cindex/test_source_range.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_source_range.py
rename to clang/test/bindings/python/tests/cindex/test_source_range.py
diff --git a/clang/bindings/python/tests/cindex/test_tls_kind.py b/clang/test/bindings/python/tests/cindex/test_tls_kind.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_tls_kind.py
rename to clang/test/bindings/python/tests/cindex/test_tls_kind.py
diff --git a/clang/bindings/python/tests/cindex/test_token_kind.py b/clang/test/bindings/python/tests/cindex/test_token_kind.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_token_kind.py
rename to clang/test/bindings/python/tests/cindex/test_token_kind.py
diff --git a/clang/bindings/python/tests/cindex/test_tokens.py b/clang/test/bindings/python/tests/cindex/test_tokens.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_tokens.py
rename to clang/test/bindings/python/tests/cindex/test_tokens.py
diff --git a/clang/bindings/python/tests/cindex/test_translation_unit.py b/clang/test/bindings/python/tests/cindex/test_translation_unit.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_translation_unit.py
rename to clang/test/bindings/python/tests/cindex/test_translation_unit.py
diff --git a/clang/bindings/python/tests/cindex/test_type.py b/clang/test/bindings/python/tests/cindex/test_type.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/test_type.py
rename to clang/test/bindings/python/tests/cindex/test_type.py
diff --git a/clang/bindings/python/tests/cindex/util.py b/clang/test/bindings/python/tests/cindex/util.py
similarity index 100%
rename from clang/bindings/python/tests/cindex/util.py
rename to clang/test/bindings/python/tests/cindex/util.py

@AaronBallman
Copy link
Collaborator

CC @llvm/infrastructure-area-team -- I'm not certain who is the best person to review lit work; that's sort of the confluence between infraustructure and the rest of the project. Do you have any good suggestions? (We don't have a maintainer listed for it that I could find.)

Copy link
Contributor

@DeinAlptraum DeinAlptraum left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, imo this is a much better approach than the previous one.

As I mentioned before, I'm not familiar with the testing infrastructure, but insofar as it works, this looks good to me.

Do note that the libclang CI should also be adapted: .github/workflows/libclang-python-tests.yml
The target it uses doesn't exist anymore, and the paths that trigger the workflow should also be changed to remove the old path and include clang/tests/bindings/python. You can now also remove the trigger on clang/CMakeLists.txt, which should make this run far less often on unrelated PRs.

Comment on lines 10 to 17
# Benign error modes.
if "wrong ELF class: ELFCLASS32" in str(e):
return False
elif "No such file or directory" in str(e):
return False
# Unknown error modes.
else:
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems backwards - (either the implementation or the comment) if the error modes are benign, wouldn't that mean it's OK/libclang /is/ loadable?

(& also in general, I'd have thought we'd look for specific error modes we can recover from, and anything else means "no recovery" rather than assuming anything other than these bad ones are good?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intent (and it worked for me) is as follows:

  • If libclang.so cannot be loaded, this can be for two reasons so far:
    • In a 32-bit build on a 64-bit system, the resulting 32-bit libclang.so cannot be loaded into the system's 64-bit python. This is as expected, nothing to be done about that, thus the lib is truely considered unloadable.
    • If libclang.so is missing completely, this must be because building it has been disabled somehow (probably with -DLLVM_ENABLE_PIC=OFF. Again, this is intentional and nothing to be done.
  • However, in other cases there may well be a bug lurking here. Consider (just for the sake of argument) a libclang.so matching python's ELF class, but built with missing dependencies so it's not self-contained and cannot be loaded. This is simply a bug that could/should be fixed. Therefore, I return True for unknown/unhandled errors, which lets the test to FAIL, forcing investigation. The failure may be benign yet not currently handled, or there may be a real bug that needs fixing.

If I always return False on a failure to load libclang.so, such bugs would go unnoticed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK. Some more comments might be helpful - like "if the failure is unexpected, return true, allowing the test to run and fail so developers can see the failure" or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the update addresses your concern. I also decided to log unexpected errors from lit.local.cfg to avoid every developer encountering this having to add their own debug code.

@DeinAlptraum
Copy link
Contributor

@rorth have you tested that your is_libclang_loadable works as intended w.r.t the "Benign error modes"?
Do correct me if I made a mistake here, but I just tried testing this by manually adding raise Exception("wrong ELF class: ELFCLASS32") at the start of get_cindex_library and then running the tests via build/bin/llvm-lit clang/test/bindings/python, and this just gives me a regular FAIL. Adding some debugging statements into is_libclang_loadable shows that the except block is never entered for some reason...

@rorth
Copy link
Collaborator Author

rorth commented Jun 6, 2025

Thank you for the PR, imo this is a much better approach than the previous one.

Thanks. The previous approach was just a minimal fix (hack?) to avoid breaking the build in a specific situation. This one (if completed) should handle all known issues with Python bindings testing.

As I mentioned before, I'm not familiar with the testing infrastructure, but insofar as it works, this looks good to me.

It will certainly need more testing in situations currently covered by RUN_PYTHON_TESTS that I known nothing about and probably cannot test myself.

Do note that the libclang CI should also be adapted: .github/workflows/libclang-python-tests.yml The target it uses doesn't exist anymore, and the paths that trigger the workflow should also be changed to remove the old path and include clang/tests/bindings/python. You can now also remove the trigger on clang/CMakeLists.txt, which should make this run far less often on unrelated PRs.

Ah, I see. I've made the obvious adjustments locally, but am at a bit of a loss how to handle build_target now. Doing this manually would be an invocation of bin/llvm-lit clang/test --filter=bindings/python or some such. Maybe a new implementation of the check-clang-python target needs to be introduced? Don't know if/how I can test this myself.

@rorth
Copy link
Collaborator Author

rorth commented Jun 6, 2025

@rorth have you tested that your is_libclang_loadable works as intended w.r.t the "Benign error modes"? Do correct me if I made a mistake here, but I just tried testing this by manually adding raise Exception("wrong ELF class: ELFCLASS32") at the start of get_cindex_library and then running the tests via build/bin/llvm-lit clang/test/bindings/python, and this just gives me a regular FAIL. Adding some debugging statements into is_libclang_loadable shows that the except block is never entered for some reason...

As mentioned in the description, I've tested the patch in a variety of configurations:

  • Solaris/amd64 (where libclang.so matches python, causing the test to PASS)
  • Solaris/i386 (where the 32-bit libclang.so cannot be loaded into the system python, which causes the test to become UNSUPPORTED.
  • Similarly Solaris/sparcv9, Solaris/sparc, Linux/x86_64, Linux/i686 (and also, not mentioned above) Linux/sparc64 and Linux/sparc.

Besides, I've manually moved libclang.so aside to test the libclang missing case.

I've got no explanation why your explicit raise didn't come through (but then I known very little Python).

@DeinAlptraum
Copy link
Contributor

I've got no explanation why your explicit raise didn't come through (but then I known very little Python).

Strange, I'll see if I can find out why, but as long as it works also for the expected failure case for you, that's good enough for me.

am at a bit of a loss how to handle build_target now [...] Don't know if/how I can test this myself.

All the check-clang-python did is depend on the libclang target and then call unittest, so I think it should be enough to change the build_target to libclang, and then add another step to the workflow that calls lit as you described.
Feel free to abuse the CI to test this until it works.

- Handle `LLVM_USE_SANITIZER` in `lit.local.cfg`.
- Fix SPARC handling, restrict to Linux/sparc*.
- Explain failure modes, log unhandled ones.
@rorth
Copy link
Collaborator Author

rorth commented Jun 11, 2025

am at a bit of a loss how to handle build_target now [...] Don't know if/how I can test this myself.

All the check-clang-python did is depend on the libclang target and then call unittest, so I think it should be enough to change the build_target to libclang, and then add another step to the workflow that calls lit as you described. Feel free to abuse the CI to test this until it works.

I've made some adjustments. The weird thing is that build_target isn't even documented in Workflow syntax for GitHub Actions. I've added an llvm-lit invocation that should run the test, but don't know anything about the layout of the build tree. Attempts to test this may have to wait until I return from vacation in a week and a half.

@rorth
Copy link
Collaborator Author

rorth commented Jun 11, 2025

Two issues are worth mentioning about the updated PR:

  • Although I'd originally disabled the Clang Python tests on SPARC in [python, tests] Disable Clang Python tests on SPARC, they now PASS on Solaris/sparcv9, while on Linux/sparc64 python SEGVs when loading libclang.so. In a Debug build, however, I get a SIGBUS which seems to point to an FFI issue again, so I'm restricting the XFAIL to Linux/sparc64.
  • I'm now logging unhandled failures from lit.local.cfg so there's a clear indication what's wrong and where to look.

@rorth rorth requested review from lanza, bcardosolopes, nikic and a team as code owners June 11, 2025 12:21
@rorth
Copy link
Collaborator Author

rorth commented Jun 11, 2025

Completely messed rebase ;-( Will probably drop branch and create new PR.

@Endilll
Copy link
Contributor

Endilll commented Jun 11, 2025

Completely messed rebase ;-( Will probably drop branch and create new PR.

Note that rebases make it significantly harder for reviewers to understand history of changes in given PR.

@rorth
Copy link
Collaborator Author

rorth commented Jun 11, 2025

Completely messed rebase ;-( Will probably drop branch and create new PR.

Note that rebases make it significantly harder for reviewers to understand history of changes in given PR.

I wonder if there's a way to recover from my mess, like (in my local repo)

$ git reset --hard <my last commit>
$ git push -f

and later, once the PR is approved, perform the rebase as described in LLVM GitHub User Guide, Landing your change via git fetch upstream; git rebase upstream/main?

I also wondered why a later commit to one of the files I've only moved unchanged is considered a conflict at all.

@DeinAlptraum
Copy link
Contributor

I wonder if there's a way to recover from my mess, like (in my local repo)

What's preventing you from doing exactly that?

The weird thing is that build_target isn't even documented in Workflow syntax for GitHub Actions

The (original) workflow contains the following:

    uses: ./.github/workflows/llvm-project-tests.yml
    with:
      build_target: check-clang-python
      projects: clang
      # There is an issue running on "windows-2019".
      # See https://github.com/llvm/llvm-project/issues/76601#issuecomment-1873049082.
      os_list: '["ubuntu-24.04"]'
      python_version: ${{ matrix.python-version }}

which is the definition of a single step in the bulidflow. That step executes the /.github/workflows/llvm-project-tests.yml action (an action we've defined ourselves and can be found under that path in the repo). Everything after the with: part are the inputs to that action, i.e. the parameters to that function if you will. You won't find build_target in any official Github documentation because it's an input to that specific action: if you need any details about its definition or usage, you can take a look at the llvm-project-tests action. Happy to answer any questions if anything else is unclear/you need help with any part

@rorth rorth force-pushed the clang-python-test-lit branch from 481ba3c to 0503301 Compare June 12, 2025 07:55
@rorth
Copy link
Collaborator Author

rorth commented Jun 12, 2025

I wonder if there's a way to recover from my mess, like (in my local repo)

What's preventing you from doing exactly that?

Worry that it might make things even worse. Fortunately, it worked just fine, so you can finally see what my current PR is about.

The weird thing is that build_target isn't even documented in Workflow syntax for GitHub Actions

The (original) workflow contains the following:

    uses: ./.github/workflows/llvm-project-tests.yml
    with:
      build_target: check-clang-python
      projects: clang
      # There is an issue running on "windows-2019".
      # See https://github.com/llvm/llvm-project/issues/76601#issuecomment-1873049082.
      os_list: '["ubuntu-24.04"]'
      python_version: ${{ matrix.python-version }}

which is the definition of a single step in the bulidflow. That step executes the /.github/workflows/llvm-project-tests.yml action (an action we've defined ourselves and can be found under that path in the repo). Everything after the with: part are the inputs to that action, i.e. the parameters to that function if you will. You won't find build_target in any official Github documentation because it's an input to that specific action: if you need any details about its definition or usage, you can take a look at the llvm-project-tests action. Happy to answer any questions if anything else is unclear/you need help with any part

Ah, thanks. Things are coming together now. However, as I said I'll probably pursue this after my vacation.

Copy link
Contributor

@DeinAlptraum DeinAlptraum left a comment

Choose a reason for hiding this comment

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

LGTM now. I'd just like the libclang Python CI to run, but for some reason it doesn't seem to trigger...

Copy link
Contributor

@DeinAlptraum DeinAlptraum left a comment

Choose a reason for hiding this comment

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

I think I found the reason why the CI doesn't run. Unfortunately it made me realize that the changes here might require some broader adaptions...

So far the CI was a single-step workflow, that just calls the llvm-project-tests workflow with some parameters. But by design that workflow is meant to be run standalone (it is the only step in the pipeline) so it does one-off things, such as determining itself which runner image to use. But once we add a second step in our own workflow, we cannot reasonably delegate the runner image choice to llvm-project-tests anymore...

Due to that, I'm not sure if the current approach can be made workable at all without larger changes to our CI setup.
@rorth I know this is a bit much to figure out if you don't have experience with GHA. I will do some experiments to try and figure out if/how this can be solved, but I'm a bit busy atm, so not sure when I'll get around to that. Feel free to abuse the CI to try things out yourself, or to ask if you want any guidance.

Comment on lines -36 to +38
build_target: check-clang-python
build_target: libclang
run: |
llvm-lit -v tools/clang/test --filter=bindings.sh
Copy link
Contributor

@DeinAlptraum DeinAlptraum Jun 19, 2025

Choose a reason for hiding this comment

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

Took me a while, the workflow probably doesn't run because there's an error here:
So far the workflow consisted of a single step, so there was no need to open a steps section, but since we have two steps this now becomes necessary. I.e. you'd need something like this:

    steps:
      - name: Bulid Libclang
        uses: ./.github/workflows/llvm-project-tests.yml
        with:
          build_target: libclang
          projects: clang
          # There is an issue running on "windows-2019".
          # See https://github.com/llvm/llvm-project/issues/76601#issuecomment-1873049082.
          os_list: '["ubuntu-24.04"]'
          python_version: ${{ matrix.python-version }}
      - name: Run tests
        run: llvm-lit -v tools/clang/test --filter=bindings.sh

(there's no need to adapt this now, because we still have other issues as described above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:as-a-library libclang and C++ API clang Clang issues not falling into any other category github:workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants