[clang][python] Add CLANG_DISABLE_RUN_PYTHON_TESTS CMake option to disable clang python bindings tests

[asb]

We already have hard-coded logic to disable adding these tests to the check-all target in various cases. It's handy to have a CMake option to disable if in an environment where the tests don't work and you want check-all to work as expected.

---

By my understanding there's no ability to treat CMake options as tri-state, where I'd be able to tell if it was explicitly st to ON, explicitly set to OFF, or left to the default. So the new option is just "force disable" rather than allowing a force enable/

CC also @linux4life798 as another contributor I can see from the commit logs.

---

Closed in favour of #111657

[llvmbot]

@llvm/pr-subscribers-clang

Author: Alex Bradbury (asb)

Changes

We already have hard-coded logic to disable adding these tests to the check-all target in various cases. It's handy to have a CMake option to disable if in an environment where the tests don't work and you want check-all to work as expected.

---

By my understanding there's no ability to treat CMake options as tri-state, where I'd be able to tell if it was explicitly st to ON, explicitly set to OFF, or left to the default. So the new option is just "force disable" rather than allowing a force enable/

CC also @linux4life798 as another contributor I can see from the commit logs.

---

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

1 Files Affected:

• (modified) clang/bindings/python/tests/CMakeLists.txt (+6)

diff --git a/clang/bindings/python/tests/CMakeLists.txt b/clang/bindings/python/tests/CMakeLists.txt
index 2543cf739463d9..b97195c24d62b1 100644
--- a/clang/bindings/python/tests/CMakeLists.txt
+++ b/clang/bindings/python/tests/CMakeLists.txt
@@ -47,6 +47,12 @@ if(${LLVM_NATIVE_ARCH} MATCHES "^(AArch64|Hexagon|Sparc|SystemZ)$")
   set(RUN_PYTHON_TESTS FALSE)
 endif()
 
+# Allow user to explicitly disable these tests.
+option(CLANG_DISABLE_RUN_PYTHON_TESTS "Do not run Clang Python bindings tests" OFF)
+if(CLANG_DISABLE_RUN_PYTHON_TESTS)
+  set(RUN_PYTHON_TESTS FALSE)
+endif()
+
 if(RUN_PYTHON_TESTS)
     set_property(GLOBAL APPEND PROPERTY
                  LLVM_ALL_ADDITIONAL_TEST_TARGETS check-clang-python)

[DeinAlptraum]

LGTM, but I'd like @Endilll to take a look as well, since I'm not that familiar with our build infrastructure & cmake

[Endilll]

We already have hard-coded logic to disable adding these tests to the check-all target in various cases.

Why do you have to do that?

[asb]

We already have hard-coded logic to disable adding these tests to the check-all target in various cases.

Why do you have to do that?

I've never worked with the clang python bindings before, but you can basically see the reasoning in https://github.com/llvm/llvm-project/blob/bf7022f71a95313e8ebdd34e5ea7cb7bca90a24d/clang/bindings/python/tests/CMakeLists.txt - there are various targets and configurations where they are expected or known to fail.

[Endilll]

We already have hard-coded logic to disable adding these tests to the check-all target in various cases.

Why do you have to do that?

I've never worked with the clang python bindings before, but you can basically see the reasoning in https://github.com/llvm/llvm-project/blob/bf7022f71a95313e8ebdd34e5ea7cb7bca90a24d/clang/bindings/python/tests/CMakeLists.txt - there are various targets and configurations where they are expected or known to fail.

Sorry, I'm asking for the use case for the particular CMake knob you're adding. If there are more platforms that those tests are not working on, we should simply expand the pre-existing list

llvm-project/clang/bindings/python/tests/CMakeLists.txt

Line 46 in e2dc50c

if(${LLVM_NATIVE_ARCH} MATCHES "^(AArch64|Hexagon|Sparc|SystemZ)$")

Or, better, disable tests individually based on the triple, like we do elsewhere.

[asb]

Sorry, I'm asking for the use case for the particular CMake knob you're adding. If there are more platforms that those tests are not working on, we should simply expand the pre-existing list

Ah sorry for misunderstanding you - good question. In my case it's a cross setup. The host Python3_EXECUTABLE will never successfully run these tests, even if a target python3 (run via qemu-user and binfmt_misc) could. Though now I write this, I'm thinking it would make more sense to set RUN_PYTHON_TESTS to FALSE if CMAKE_CROSSCOMPILING is set. What do you think?

[Endilll]

Though now I write this, I'm thinking it would make more sense to set RUN_PYTHON_TESTS to FALSE if CMAKE_CROSSCOMPILING is set. What do you think?

Yes, I like this direction much more. Probably you should also print a message during configuration saying that those tests are excluded because of cross setup.

Do you know what the state of support for cross environments is in general? If they are generally well-maintained, we're more incentivized to do a proper support for them in Python bindings tests.

[asb]

Though now I write this, I'm thinking it would make more sense to set RUN_PYTHON_TESTS to FALSE if CMAKE_CROSSCOMPILING is set. What do you think?

Yes, I like this direction much more. Probably you should also print a message during configuration saying that those tests are excluded because of cross setup.

Thanks for the review and feedback, definitely a better solution. I've posted it as a separate PR rather than reworking this one because it's a fundamentally different approach and editing this one would be confusing when coming back later. #111657

Do you know what the state of support for cross environments is in general? If they are generally well-maintained, we're more incentivized to do a proper support for them in Python bindings tests.

Now I've landed #111373 a cross-build and then check-all actually works surprisingly well. I'm not sure to what degree I'd consider it officially supported, but I'm working to clean it up a bit as I think there's scope for taking more advantage of this setup for local development, litmus testing for releases, and at least some CI (on the staging buildmaster if not the main one).