diff --git a/.github/workflows/root-ci-config/buildconfig/mac-beta.txt b/.github/workflows/root-ci-config/buildconfig/mac-beta.txt index f7de3ebcdb542..c4e6c9003106e 100644 --- a/.github/workflows/root-ci-config/buildconfig/mac-beta.txt +++ b/.github/workflows/root-ci-config/buildconfig/mac-beta.txt @@ -22,3 +22,4 @@ cocoa=ON davix=OFF minuit2_omp=OFF x11=OFF +ROOT_CTEST_CUSTOM_FLAGS=-R ^pyunittests-bindings-cppjit- diff --git a/.github/workflows/root-ci.yml b/.github/workflows/root-ci.yml index 0af636192a117..9477dde216f5a 100644 --- a/.github/workflows/root-ci.yml +++ b/.github/workflows/root-ci.yml @@ -120,11 +120,10 @@ jobs: matrix: # Specify platform + optional build option overrides # Build options: https://root.cern/install/build_from_source/#all-build-options - platform: [mac26, mac27, mac-beta] + # TEMP: only mac-beta, to measure the sporadic cppjit test_datatypes crash + platform: [mac-beta] arch: [ARM64] include: - - platform: mac15 - arch: X86 - platform: mac-beta is_special: true @@ -246,13 +245,7 @@ jobs: build-windows: # For any event that is not a PR, the CI will always run. In PRs, the CI # can be skipped if the tag [skip-ci] or [skip ci] is written in the title. - if: | - (github.repository_owner == 'root-project' && github.event_name != 'pull_request') || - (github.event_name == 'pull_request' && !( - contains(github.event.pull_request.title, '[skip-ci]') || - contains(github.event.pull_request.title, '[skip ci]') || - contains(github.event.pull_request.labels.*.name, 'skip ci') - )) + if: false # TEMP: only mac-beta runs, see build-macos permissions: contents: read @@ -372,13 +365,7 @@ jobs: build-linux: # For any event that is not a PR, the CI will always run. In PRs, the CI # can be skipped if the tag [skip-ci] or [skip ci] is written in the title. - if: | - (github.repository_owner == 'root-project' && github.event_name != 'pull_request') || - (github.event_name == 'pull_request' && !( - contains(github.event.pull_request.title, '[skip-ci]') || - contains(github.event.pull_request.title, '[skip ci]') || - contains(github.event.pull_request.labels.*.name, 'skip ci') - )) + if: false # TEMP: only mac-beta runs, see build-macos permissions: contents: read diff --git a/bindings/pyroot/cppjit/src/cpyrt/CPPMethod.cxx b/bindings/pyroot/cppjit/src/cpyrt/CPPMethod.cxx index 0221f836748ac..ade34612af2c3 100644 --- a/bindings/pyroot/cppjit/src/cpyrt/CPPMethod.cxx +++ b/bindings/pyroot/cppjit/src/cpyrt/CPPMethod.cxx @@ -181,10 +181,12 @@ inline PyObject* cpyrt::CPPMethod::ExecuteFast(void* self, ptrdiff_t offset, result = nullptr; } -// TODO: covers the PyException throw case, which does not seem to work on -// Windows, so instead leaves the error be -#ifdef _WIN32 +#if CPYRT_CALLBACK_NOTHROW + // the callback wrappers leave the Python error set instead of throwing a + // PyException (see CommonDefs.h), so pick it up here, as the catch above + // would have, and flag it as the callee's error for overload resolution if (PyErr_Occurred()) { + ctxt->fFlags |= CallContext::kPyException; if (ResultIsPyObject()) Py_XDECREF(result); result = nullptr; diff --git a/bindings/pyroot/cppjit/src/cpyrt/CommonDefs.h b/bindings/pyroot/cppjit/src/cpyrt/CommonDefs.h index 0db73202137e5..17e27fe8bddc2 100644 --- a/bindings/pyroot/cppjit/src/cpyrt/CommonDefs.h +++ b/bindings/pyroot/cppjit/src/cpyrt/CommonDefs.h @@ -31,4 +31,23 @@ #endif +// How JIT-compiled callback wrappers (function pointers, std::function, and +// virtual overrides implemented in Python) report a failed Python call. The +// default is to throw a cpyrt::PyException from the wrapper, which unwinds +// through the C++ frames in between (JIT-compiled, or built by another +// compiler) up to the try block in CPPMethod::ExecuteFast. That unwind hangs +// on Windows, and on Apple ARM64 exceptions through JIT-compiled frames are +// known not to work (the test suite skips the tests that catch a PyException +// in C++ there) and are the prime suspect for the sporadic crash of +// test_datatypes on the macOS beta CI runner. On those platforms the wrapper +// therefore leaves the Python error set and returns a default value instead; +// the C++ code keeps running and ExecuteFast turns the pending error into a +// failed call once the C++ call returns. The price is that C++ code can not +// catch the Python error as a PyException. +#if defined(_WIN32) || (defined(__APPLE__) && defined(__aarch64__)) +#define CPYRT_CALLBACK_NOTHROW 1 +#else +#define CPYRT_CALLBACK_NOTHROW 0 +#endif + #endif // !CPYRT_COMMONDEFS_H diff --git a/bindings/pyroot/cppjit/src/cpyrt/Utility.cxx b/bindings/pyroot/cppjit/src/cpyrt/Utility.cxx index 9538a57b46260..13ec44b035abd 100644 --- a/bindings/pyroot/cppjit/src/cpyrt/Utility.cxx +++ b/bindings/pyroot/cppjit/src/cpyrt/Utility.cxx @@ -1065,7 +1065,12 @@ void cpyrt::Utility::ConstructCallbackPreamble( } code << " } catch(int) {\n" << " for (auto pyarg : pyargs) Py_XDECREF(pyarg);\n" +#if CPYRT_CALLBACK_NOTHROW + // error is set; see CPYRT_CALLBACK_NOTHROW in CommonDefs.h + << (isVoid ? " return;\n" : " return ret;\n") +#else << " cppjit::cpyrt::PyException pyexc; throw pyexc;\n" +#endif << " }\n"; } } @@ -1095,9 +1100,9 @@ void cpyrt::Utility::ConstructCallbackReturn(const std::string& retType, if (isPtr) code << " }\n"; code << " if (!cOk) {" // assume error set when converter failed -// TODO: On Windows, throwing a C++ exception here makes the code hang; leave -// the error be which allows at least one layer of propagation -#ifdef _WIN32 +// see CPYRT_CALLBACK_NOTHROW in CommonDefs.h: leave the error be, which allows +// at least one layer of propagation +#if CPYRT_CALLBACK_NOTHROW " /* do nothing */ }\n" #else " cppjit::cpyrt::PyException pyexc; throw pyexc; }\n" diff --git a/bindings/pyroot/cppjit/test/CMakeLists.txt b/bindings/pyroot/cppjit/test/CMakeLists.txt index 482c2abb89102..84bf4d10be550 100644 --- a/bindings/pyroot/cppjit/test/CMakeLists.txt +++ b/bindings/pyroot/cppjit/test/CMakeLists.txt @@ -37,7 +37,7 @@ endif() function(CPPJIT_ADD_PYTEST name) cmake_parse_arguments(ARG - "DROP_ROOTMAP" "" "DICTS;FIXTURES_SETUP;FIXTURES_REQUIRED" ${ARGN}) + "DROP_ROOTMAP" "SUFFIX" "DICTS;FIXTURES_SETUP;FIXTURES_REQUIRED" ${ARGN}) set(extra) if(ARG_DICTS) @@ -60,7 +60,7 @@ function(CPPJIT_ADD_PYTEST name) endif() string(REGEX REPLACE "[_]" "-" good_name "${name}") - set(test_name pyunittests-bindings-cppjit-${good_name}) + set(test_name pyunittests-bindings-cppjit-${good_name}${ARG_SUFFIX}) ROOT_ADD_TEST(${test_name} # Upstream marks platform xfails without explicit strict=; the in-tree @@ -115,6 +115,13 @@ CPPJIT_ADD_PYTEST(test_basic_api FIXTURES_REQUIRED cppjit_example01_dict_fixture) CPPJIT_ADD_PYTEST(test_datatypes FIXTURES_REQUIRED cppjit_datatypes_dict_fixture) +# TEMP: 300 more copies of test_datatypes, run by ctest in parallel in this +# directory like the real suite does, to measure its sporadic crash on the +# mac-beta CI runner (about 3% of runs). +foreach(i RANGE 1 300) + CPPJIT_ADD_PYTEST(test_datatypes SUFFIX -rep${i} + FIXTURES_REQUIRED cppjit_datatypes_dict_fixture) +endforeach() CPPJIT_ADD_PYTEST(test_api) CPPJIT_ADD_PYTEST(test_boost)