Enable CppInterOp #16814

aaronj0 · 2024-11-04T14:32:57Z

CppInterOp exposes API from Clang and LLVM in a mostly backward compatibe way. The API support downstream tools that utilize interactive C++ by using the compiler as a service.

This PR is the first step in using pure clang based reflection API in meta, and part of eventually integrating the JITCall and DynamicLibraryManager infrastructure.

Adopting more of CppInterOp in ROOT will help simplify the LLVM migration process and allow us to upstream more patches to either CppInterOp or LLVM.

TODO:

Add github workflow to diffcheck upstream
Improve CMake config
Propoagate LLVM_BUILD_TYPE instead of ROOT's config to InterOp's build type

github-actions · 2024-11-04T16:16:07Z

Test Results

18 files 18 suites 3d 22h 13m 24s ⏱️
2 692 tests 2 692 ✅ 0 💤 0 ❌
46 760 runs 46 760 ✅ 0 💤 0 ❌

Results for commit 34a36e5.

♻️ This comment has been updated with latest results.

hahnjo

The direction looks good to me! In addition to the test failures that need fixing, some comments inline for consideration

core/metacling/src/CMakeLists.txt

hahnjo · 2024-11-05T10:10:14Z

interpreter/CMakeLists.txt

+  set(LLVM_DIR "${CMAKE_BINARY_DIR}/interpreter/llvm-project/llvm/lib/cmake/llvm")
+  set(Clang_DIR "${CMAKE_BINARY_DIR}/interpreter/llvm-project/llvm/tools/clang/lib/cmake/clang")
+  set(Cling_DIR "${CMAKE_BINARY_DIR}/interpreter/cling")


This will need adapting for externally built LLVM, Clang, and / or Cling. For LLVM and Clang, I believe this should already be taken care of above, no?

This comment still applies, even if the code was moved: variables related to this are already set above (and will change with #17354)

Thanks, I will look into improving this config for externally built llvm and cling, and test if the existing variables work

hahnjo · 2024-11-05T10:11:39Z

interpreter/CppInterOp/README.md

@@ -0,0 +1,372 @@
+<div align="center">
+
+# CppInterOp


As discussed offline, we probably want a similar workflow as .github/workflows/llvm-diff.yml to make sure our copies are not diverging

I would still strongly recommend implement this.

Thanks! The workflow has been added

core/metacling/src/TClingCallFunc.cxx

interpreter/CppInterOp/unittests/CppInterOp/CMakeLists.txt

core/metacling/src/CMakeLists.txt

hahnjo · 2025-01-20T09:51:43Z

interpreter/CMakeLists.txt

+set(USE_CLING ON CACHE BOOL "Use Cling as backend" FORCE)
+set(USE_REPL OFF CACHE BOOL "Use Clang-repl as backend" FORCE)


Uhm, can CppInterOp be changed to scope its configuration options? Setting this as a global CACHE variable is not great.

Done! We can get away by just setting USE_CLING ON

hahnjo · 2025-01-20T09:53:19Z

interpreter/CppInterOp/unittests/CppInterOp/FunctionReflectionTest.cpp

+
+#ifdef _WIN32
+  GTEST_SKIP() << "Disabled on Windows (Fails with ROOT win-x64)";
+#endif


Is this done upstream in CppInterOp? We should not start by immediately diverging

@aaronj0, do you remember how this used to fail?

hahnjo · 2025-01-20T09:54:15Z

interpreter/CppInterOp/unittests/CppInterOp/InterpreterTest.cpp

-#ifdef LLVM_BINARY_DIR
-TEST(InterpreterTest, DetectResourceDir) {
-#else
+
 TEST(InterpreterTest, DISABLED_DetectResourceDir) {
-#endif // LLVM_BINARY_DIR


Is this done upstream in CppInterOp? We should not start by immediately diverging

This test is only required when shipping CppInterOp externally and doesn't work with ROOT since llvm is not externally built, which is why we opted to disable and track this as a patch to the tests..

Here I think the binary dir is set but it is not what we expect it to be. I think that's part of the problem that LLVM's build system was not designed to be embedded in another build system...

.github/workflows/cppinterop-diff.yml

hahnjo · 2025-01-30T11:14:13Z

.github/workflows/cppinterop-diff.yml

+        run: |
+          rm -rf .github
+      - name: Compare
+        continue-on-error: true


Ah, I was already wondering: With this line you just ignore the error and continue even if the two copies have a difference. That renders the check useless...

Makes sense, I will drop the continue-on-error setting. I will note that we may have to keep a patch disabling a unittest, assuming the other failing test on Windows is fixed and no longer disabled

vgvassilev

This practically looks good to me...

vgvassilev · 2025-01-30T11:30:16Z

interpreter/CppInterOp/unittests/CppInterOp/InterpreterTest.cpp

-#ifdef LLVM_BINARY_DIR
-TEST(InterpreterTest, DetectResourceDir) {
-#else
+
 TEST(InterpreterTest, DISABLED_DetectResourceDir) {
-#endif // LLVM_BINARY_DIR


Here I think the binary dir is set but it is not what we expect it to be. I think that's part of the problem that LLVM's build system was not designed to be embedded in another build system...

vgvassilev · 2025-01-30T11:30:42Z

interpreter/CppInterOp/unittests/CppInterOp/FunctionReflectionTest.cpp

+
+#ifdef _WIN32
+  GTEST_SKIP() << "Disabled on Windows (Fails with ROOT win-x64)";
+#endif


@aaronj0, do you remember how this used to fail?

bellenot · 2025-01-30T13:41:29Z

@vgvassilev about your question, here is the error:

1: [ RUN      ] FunctionReflectionTest.GetFunctionCallWrapper
1: C:\root-dev\github\enable-interop\interpreter\CppInterOp\unittests\CppInterOp\FunctionReflectionTest.cpp(919): error: Expected equality of these values:
1:   output
1:     Which is: ""
1:   s
1:     Which is: "Hello World!\n"
1: C:\root-dev\github\enable-interop\interpreter\CppInterOp\unittests\CppInterOp\FunctionReflectionTest.cpp(944): error: Expected equality of these values:
1:   output
1:     Which is: ""
1:   "Default Ctor Called\n"
1: C:\root-dev\github\enable-interop\interpreter\CppInterOp\unittests\CppInterOp\FunctionReflectionTest.cpp(953): error: Expected equality of these values:
1:   output
1:     Which is: ""
1:   "Dtor Called\n"
1: [  FAILED  ] FunctionReflectionTest.GetFunctionCallWrapper (428 ms)

vgvassilev · 2025-01-30T14:51:57Z

@vgvassilev about your question, here is the error:

1: [ RUN      ] FunctionReflectionTest.GetFunctionCallWrapper
1: C:\root-dev\github\enable-interop\interpreter\CppInterOp\unittests\CppInterOp\FunctionReflectionTest.cpp(919): error: Expected equality of these values:
1:   output
1:     Which is: ""
1:   s
1:     Which is: "Hello World!\n"
1: C:\root-dev\github\enable-interop\interpreter\CppInterOp\unittests\CppInterOp\FunctionReflectionTest.cpp(944): error: Expected equality of these values:
1:   output
1:     Which is: ""
1:   "Default Ctor Called\n"
1: C:\root-dev\github\enable-interop\interpreter\CppInterOp\unittests\CppInterOp\FunctionReflectionTest.cpp(953): error: Expected equality of these values:
1:   output
1:     Which is: ""
1:   "Dtor Called\n"
1: [  FAILED  ] FunctionReflectionTest.GetFunctionCallWrapper (428 ms)

Any chance to debug this? Maybe it is the way we capture streams? I doubt this is as broken to not run the constructors on windows…

bellenot · 2025-01-30T14:57:02Z

Any chance to debug this? Maybe it is the way we capture streams? I doubt this is as broken to not run the constructors on windows…

Sure! I can try to take a look when I find some time for it

vgvassilev · 2025-01-30T18:05:46Z

Any chance to debug this? Maybe it is the way we capture streams? I doubt this is as broken to not run the constructors on windows…

Sure! I can try to take a look when I find some time for it

In this case let's leave this pending and move forward with the PR.

by linking clangCppInterOp in libCling

aaronj0 requested review from bellenot, dpiparo and pcanal as code owners November 4, 2024 14:32

aaronj0 changed the title ~~Enable interop~~ Enable CppInterOp Nov 4, 2024

aaronj0 requested review from hahnjo, vgvassilev and guitargeek November 4, 2024 14:33

aaronj0 force-pushed the enable-interop branch from 9392e07 to 2d038e7 Compare November 4, 2024 14:45

aaronj0 marked this pull request as draft November 4, 2024 14:58

devajithvs assigned aaronj0 Nov 4, 2024

hahnjo requested changes Nov 5, 2024

View reviewed changes

guitargeek added the clean build Ask CI to do non-incremental build on PR label Dec 2, 2024

aaronj0 force-pushed the enable-interop branch 2 times, most recently from ef871f9 to 86246a7 Compare December 2, 2024 10:46

aaronj0 removed the clean build Ask CI to do non-incremental build on PR label Dec 3, 2024

aaronj0 force-pushed the enable-interop branch 2 times, most recently from 196c4ff to 9977082 Compare December 3, 2024 14:43

vgvassilev mentioned this pull request Dec 3, 2024

Enable CppInterOp #16296

Closed

aaronj0 force-pushed the enable-interop branch 3 times, most recently from ab52fd8 to 540c6fc Compare December 9, 2024 10:49

vgvassilev reviewed Dec 9, 2024

View reviewed changes

interpreter/CppInterOp/unittests/CppInterOp/CMakeLists.txt Outdated Show resolved Hide resolved

aaronj0 force-pushed the enable-interop branch 2 times, most recently from c08b40e to 2a684e1 Compare December 20, 2024 06:08

aaronj0 force-pushed the enable-interop branch from 93afacc to b48a22e Compare December 27, 2024 06:20

guitargeek mentioned this pull request Jan 7, 2025

[Python] Avoid using std::regex in CPyCppyy #17364

Closed

vgvassilev reviewed Jan 7, 2025

View reviewed changes

core/metacling/src/CMakeLists.txt Outdated Show resolved Hide resolved

aaronj0 force-pushed the enable-interop branch 2 times, most recently from 6f2ac11 to 7e53b7b Compare January 9, 2025 18:50

aaronj0 marked this pull request as ready for review January 18, 2025 17:38

aaronj0 force-pushed the enable-interop branch from cd493d8 to c25ce54 Compare January 20, 2025 05:03

hahnjo reviewed Jan 20, 2025

View reviewed changes

aaronj0 force-pushed the enable-interop branch 7 times, most recently from be05b00 to ffb1ec6 Compare January 29, 2025 14:05

aaronj0 requested a review from hahnjo January 30, 2025 09:49

hahnjo reviewed Jan 30, 2025

View reviewed changes

.github/workflows/cppinterop-diff.yml Outdated Show resolved Hide resolved

hahnjo reviewed Jan 30, 2025

View reviewed changes

vgvassilev reviewed Jan 30, 2025

View reviewed changes

aaronj0 force-pushed the enable-interop branch 2 times, most recently from 2a58aa5 to 4ee500d Compare February 5, 2025 08:02

aaronj0 added 2 commits February 5, 2025 09:11

Enable the CppInterOp library

f1346ca

Disable GetFunctionCallWrapper test on windows

c221087

aaronj0 force-pushed the enable-interop branch 2 times, most recently from cd575d7 to 34a36e5 Compare February 5, 2025 08:20

[TCling] Allow InterOp to access ROOT's interpreter

42e186f

by linking clangCppInterOp in libCling

aaronj0 force-pushed the enable-interop branch from 34a36e5 to b8c8476 Compare February 6, 2025 12:38

[ci] Add github workflow to check diff against upstream CppInterOp

cc86b50

aaronj0 force-pushed the enable-interop branch from b8c8476 to cc86b50 Compare February 6, 2025 12:39

[debug] Skip Resource Dir test if no Clang binary

f0512ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable CppInterOp #16814

Enable CppInterOp #16814

aaronj0 commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024 •

edited

Loading

hahnjo left a comment

hahnjo Nov 5, 2024

hahnjo Jan 20, 2025

aaronj0 Jan 20, 2025

hahnjo Nov 5, 2024

hahnjo Jan 20, 2025

aaronj0 Jan 29, 2025

hahnjo Jan 20, 2025

aaronj0 Jan 29, 2025

hahnjo Jan 20, 2025

vgvassilev Jan 30, 2025

hahnjo Jan 20, 2025

aaronj0 Jan 20, 2025

vgvassilev Jan 30, 2025

hahnjo Jan 30, 2025

aaronj0 Jan 31, 2025 •

edited

Loading

vgvassilev left a comment

vgvassilev Jan 30, 2025

vgvassilev Jan 30, 2025

bellenot commented Jan 30, 2025

vgvassilev commented Jan 30, 2025

bellenot commented Jan 30, 2025 •

edited

Loading

vgvassilev commented Jan 30, 2025

		set(USE_CLING ON CACHE BOOL "Use Cling as backend" FORCE)
		set(USE_REPL OFF CACHE BOOL "Use Clang-repl as backend" FORCE)

Enable CppInterOp #16814

Are you sure you want to change the base?

Enable CppInterOp #16814

Conversation

aaronj0 commented Nov 4, 2024 • edited Loading

github-actions bot commented Nov 4, 2024 • edited Loading

Test Results

hahnjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronj0 Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellenot commented Jan 30, 2025

vgvassilev commented Jan 30, 2025

bellenot commented Jan 30, 2025 • edited Loading

vgvassilev commented Jan 30, 2025

aaronj0 commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024 •

edited

Loading

aaronj0 Jan 31, 2025 •

edited

Loading

bellenot commented Jan 30, 2025 •

edited

Loading