-
Notifications
You must be signed in to change notification settings - Fork 60
[test] Unify path defines and add CPPINTEROP_BIN_DIR env override #1037
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,8 @@ | |
| *.app | ||
|
|
||
| # Directories | ||
| build | ||
| install | ||
| build* | ||
| install* | ||
|
|
||
| # CLion files | ||
| .idea | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,12 @@ | ||
| // Storage definitions and initialization for dispatch function pointers. | ||
| // Linked only by DispatchTests — not by the normal test suite. | ||
|
|
||
| #include "TestPaths.h" | ||
| #include "CppInterOp/Dispatch.h" | ||
|
|
||
| #include <cstdlib> | ||
| #include <iostream> | ||
|
|
||
| // Define storage for all raw dispatch function pointers. | ||
| using namespace Cpp; | ||
| #define CPPINTEROP_API_FUNC(DN, CN, Ret, DeclArgs, CallArgs, RawTypes) \ | ||
|
|
@@ -12,8 +16,18 @@ using namespace Cpp; | |
| namespace { | ||
| struct DispatchInitializer { | ||
| DispatchInitializer() { | ||
| if (!Cpp::LoadDispatchAPI(CPPINTEROP_LIB_PATH)) { | ||
| std::abort(); | ||
| std::string libPath = TestUtils::GetCppInterOpLibPath(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: no header providing "std::string" is directly included [misc-include-cleaner] unittests/CppInterOp/DispatchInit.cpp:9: + #include <string> |
||
| if (libPath.empty()) { | ||
| std::cerr << "DispatchTests: cannot locate libclangCppInterOp: set the " | ||
| "CPPINTEROP_BIN_DIR environment variable to the CppInterOp " | ||
| "artifacts prefix (the directory containing lib/ and " | ||
| "include/).\n"; | ||
| std::exit(EXIT_FAILURE); | ||
| } | ||
| if (!Cpp::LoadDispatchAPI(libPath.c_str())) { | ||
| std::cerr << "DispatchTests: failed to load dispatch API from '" | ||
| << libPath << "'.\n"; | ||
| std::exit(EXIT_FAILURE); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,40 @@ | ||||||||
| #ifndef CPPINTEROP_UNITTESTS_LIBCPPINTEROP_TESTPATHS_H | ||||||||
| #define CPPINTEROP_UNITTESTS_LIBCPPINTEROP_TESTPATHS_H | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: header guard does not follow preferred style [llvm-header-guard]
Suggested change
unittests/CppInterOp/TestPaths.h:39: - #endif // CPPINTEROP_UNITTESTS_LIBCPPINTEROP_TESTPATHS_H
+ #endif // GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_TESTPATHS_H
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these need to be ignored, right? Maybe there is a clang-tidy config update so this doesn't get flagged @vgvassilev?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! |
||||||||
|
|
||||||||
| #include <cstdlib> | ||||||||
| #include <string> | ||||||||
|
|
||||||||
| namespace TestUtils { | ||||||||
|
|
||||||||
| /// CppInterOp artifacts prefix (a directory containing lib/ and include/). | ||||||||
| /// The CPPINTEROP_BIN_DIR environment variable overrides the compile-time | ||||||||
| /// default so relocated test binaries can find the library. Empty if | ||||||||
| /// neither is available. | ||||||||
| inline std::string GetCppInterOpDirPath() { | ||||||||
| if (const char* env = std::getenv("CPPINTEROP_BIN_DIR")) | ||||||||
| return env; | ||||||||
| #ifdef CPPINTEROP_BIN_DIR | ||||||||
| return CPPINTEROP_BIN_DIR; | ||||||||
| #else | ||||||||
| return {}; | ||||||||
| #endif | ||||||||
| } | ||||||||
|
|
||||||||
| /// Full path to libclangCppInterOp under the artifacts prefix; empty if the | ||||||||
| /// prefix is unknown. | ||||||||
| inline std::string GetCppInterOpLibPath() { | ||||||||
| std::string dir = GetCppInterOpDirPath(); | ||||||||
| if (dir.empty()) | ||||||||
| return {}; | ||||||||
| #if defined(_WIN32) | ||||||||
| return dir + "/lib/libclangCppInterOp.dll"; | ||||||||
| #elif defined(__APPLE__) | ||||||||
| return dir + "/lib/libclangCppInterOp.dylib"; | ||||||||
| #else | ||||||||
| return dir + "/lib/libclangCppInterOp.so"; | ||||||||
| #endif | ||||||||
| } | ||||||||
|
|
||||||||
| } // end namespace TestUtils | ||||||||
|
|
||||||||
| #endif // CPPINTEROP_UNITTESTS_LIBCPPINTEROP_TESTPATHS_H | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that change breaks ROOT. @aaronj0, can you review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These defines are
PRIVATEto theTracingTeststarget (test-internal only) — they're not exported to the install tree and can't reach ROOT. I also checked: neither ROOT's own CMake files nor its bundled CppInterOp useCPPINTEROP_DIRorCPPINTEROP_BINARY_DIRas CMake variables. So this rename shouldn't affect ROOT at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMAKE_BINARY_DIRwill evaluate to the build directory of the current project (ROOT), in which CppInterOp is built as a subproject (which is why that was changed to${CMAKE_CURRENT_SOURCE_DIR}/../../) . That will be problematic, since that is not where the CppInterOp binary lives in that build tree.I can test the patch with my ongoing CppInterOp upgrade (root-project/root#22728) and figure out what works for both usecases