-
Notifications
You must be signed in to change notification settings - Fork 8
Better test macro support for 3rd-party components #870
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?
Conversation
CMakeLists.txt
Outdated
| set(BOOST_TEST_LINK_LIBS ${BOOST_TEST_LINK_LIBS} CACHE STRING "Boost test libraries" FORCE) | ||
| add_library(testlibs INTERFACE) | ||
| target_link_libraries(testlibs INTERFACE ${BOOST_TEST_LINK_LIBS}) |
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.
Issue: Overwriting a cached variable
In CMakeLists.txt, you're setting BOOST_TEST_LINK_LIBS as a cached variable with FORCE and then immediately overwriting it:
set(BOOST_TEST_LINK_LIBS ${BOOST_TEST_LINK_LIBS} CACHE STRING "Boost test libraries" FORCE)
add_library(testlibs INTERFACE)
target_link_libraries(testlibs INTERFACE ${BOOST_TEST_LINK_LIBS})
set(BOOST_TEST_LINK_LIBS testlibs)This pattern can lead to unexpected behavior since the cached value is immediately discarded. Consider either:
- Creating a new variable for the testlibs target instead of reusing BOOST_TEST_LINK_LIBS
- Or setting the cache variable after the modification:
set(BOOST_TEST_LINK_LIBS testlibs CACHE STRING "Boost test libraries" FORCE)
cmake/bins.cmake
Outdated
| endif() | ||
|
|
||
| endforeach() | ||
|
|
||
| target_link_libraries(${TEST_NAME} PUBLIC testlibs) |
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.
Issue: Redundant library linking
In cmake/bins.cmake, you're adding a line to link the testlibs library to each test target:
target_link_libraries(${TEST_NAME} PUBLIC testlibs)However, the test function already links BOOST_TEST_LINK_LIBS (which now points to testlibs) in lines 336-337 for non-MSVC or OPENMIND_USE_CONAN builds:
if(NOT MSVC OR OPENMIND_USE_CONAN)
message("NOT MSVC OR OPENMIND_USE_CONAN, using deps ${BOOST_TEST_LINK_LIBS}")
deps(${BOOST_TEST_LINK_LIBS})
endif()This creates a redundant linking situation where testlibs could be linked twice. Consider either:
- Removing the new link statement and letting the existing code handle it
- Or modifying the existing code to avoid the redundancy
| // BOOST_TEST(values); | ||
| } | ||
|
|
||
| #ifdef _WIN32 | ||
| #pragma comment(lib, "User32.lib") // EnableMenuItem / GetSystemMenu |
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.
Issue: Platform-specific code improvements
I notice you've made two related changes to improve Windows platform support:
- Added pragma comment in omnn/math/test/08_System.cpp:
#ifdef _WIN32
#pragma comment(lib, "User32.lib") // EnableMenuItem / GetSystemMenu
#endif // _WIN32- Removed explicit target_link_libraries in omnn/math/test/CMakeLists.txt:
target_link_libraries(08_System PUBLIC
$<$<PLATFORM_ID:Windows>:User32>
)While this change makes sense as part of improving test macro support, it would be helpful to add a comment in the PR description explaining why the pragma approach is preferred over the CMake generator expression approach. This would help reviewers understand the rationale behind this specific change.
| @@ -402,6 +402,9 @@ if(NOT MSVC AND NOT OPENMIND_USE_VCPKG AND NOT OPENMIND_USE_CONAN) | |||
| endif() | |||
| set(BOOST_LINK_LIBS ${BOOST_LINK_LIBS} CACHE STRING "Boost libraries" FORCE) | |||
| set(BOOST_TEST_LINK_LIBS ${BOOST_TEST_LINK_LIBS} CACHE STRING "Boost test libraries" FORCE) | |||
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.
Everything looks good! The issues have already been identified in previous comments.
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.
Everything looks good! The issues have already been identified in previous comments.
9da9feb to
8c63972
Compare
4d6ab3c to
0643e9e
Compare
eae1177 to
c8c1872
Compare
Better test macro support for 3rd-party components