rorth wrote: > Thank you for the PR. Several points: > > 1. What happens on Windows?
I have no idea, knowing nothing at all about Windows. I cannot even tell if a similar situation can arise there, i.e. if you can execute 32-bit programs on a 64-bit Windows and, if you can, what the message would be if you tried. However, fixing an issue on one set of targets shouldn't force me to fix it everywhere: if/when such a situation arises elsewhere, any interested party can augment the code as necessary. Btw., Darwin could be similarly affected, but the last Darwin versions that could build and execute 32-bit programs are so old that I have no idea if they are still supported in LLVM. > 2. `check-clang-python` is just one of the ways to run into this. > `cindex.py` is a user-facing interface, so we shouldn't emit messages that > make sense only during development. I tried several different routes: - Add a check if `cdll.LoadLibrary` works to `tests/CMakeLists.txt`, setting `RUN_PYTHON_TESTS` to `FALSE` if not. However, this cannot work since `libclang.so` doesn't even exist at `cmake` time. - Alternatively (although I haven't tried that yet), one could a a similar small python script to the `check-clang-python` rule, not running the actual test if the library cannot be loaded. Besides, one could just change the wording of the warning message: trying to load a 32-bit `libclang.so` into a 64-bit python is always an error, testsuite or no. > 3. Given the fact that `OSError` already contains the message that > indicates the problem, I think we should explore why it's not properly > emitted in the first place, before introducing bespoke ways of delivering > error messages. @DeinAlptraum any thoughts here? Even if the error message were emitted, this doesn't help because `ninja check-all` is still broken, there being no way to disable `check-clang-python` even manually. IMO the testsuite integration of `check-clang-python` is broken in various ways: - Any failure in this target aborts all of the testsuite run, as has been seen several times in the past, even without the issue at hand. - The result is reported in a very non-standard way: instead of emitting (say) `PASS: libclang :: python bindings` or whatever, you just get `Ran 165 tests in ...` unlike any other test. Instead, the tests should be reported like the `PASS` above (or `UNSUPPORTED`, `XFAIL`, ...). A failing test **must not** abort the whole testsuite, just report an appropriate result. https://github.com/llvm/llvm-project/pull/142353 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
