chore: Modify compilation issues#357
Conversation
Modify compilation issues
Reviewer's GuideThis PR resolves compilation/linking issues by explicitly configuring pthread support in each network plugin’s CMakeLists, ensuring the thread library is correctly detected and linked. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, mhduiy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Hey @caixr23 - I've reviewed your changes - here's some feedback:
- Use find_package(Threads) and link against Threads::Threads instead of manually setting CMAKE_THREAD_LIBS_INIT and related variables.
- Factor out the repeated thread‐setup block into a shared CMake include or macro to avoid duplication across each plugin's CMakeLists.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use find_package(Threads) and link against Threads::Threads instead of manually setting CMAKE_THREAD_LIBS_INIT and related variables.
- Factor out the repeated thread‐setup block into a shared CMake include or macro to avoid duplication across each plugin's CMakeLists.
## Individual Comments
### Comment 1
<location> `network-service-plugin/CMakeLists.txt:24` </location>
<code_context>
project(${PLUGIN_NAME})
+set(CMAKE_THREAD_LIBS_INIT "-lpthread")
+set(CMAKE_HAVE_THREADS_LIBRARY 1)
+set(CMAKE_USE_PTHREADS_INIT 1)
+set(CMAKE_PREFER_PTHREAD_FLAG ON)
+
set(CMAKE_CXX_STANDARD 17)
</code_context>
<issue_to_address>
Consider centralizing this logic and using modern CMake for threading.
This code is duplicated across four `CMakeLists.txt` files; consider moving it to a shared CMake file and including it where needed. Also, prefer `find_package(Threads REQUIRED)` and linking `Threads::Threads` for better portability and modern CMake practices.
Suggested implementation:
```
include(cmake/ThreadsConfig.cmake)
```
1. Create a shared file at `cmake/ThreadsConfig.cmake` with the following content:
```
find_package(Threads REQUIRED)
```
2. In this `CMakeLists.txt`, ensure that your target (e.g., `target_link_libraries(your_target PRIVATE Threads::Threads)`) is linked to `Threads::Threads`.
3. Repeat the `include(cmake/ThreadsConfig.cmake)` and `target_link_libraries(... Threads::Threads)` in the other three `CMakeLists.txt` files as needed.
4. Adjust the path to `ThreadsConfig.cmake` if your project structure differs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| set(CMAKE_THREAD_LIBS_INIT "-lpthread") | ||
| set(CMAKE_HAVE_THREADS_LIBRARY 1) | ||
| set(CMAKE_USE_PTHREADS_INIT 1) | ||
| set(CMAKE_PREFER_PTHREAD_FLAG ON) |
There was a problem hiding this comment.
suggestion: Consider centralizing this logic and using modern CMake for threading.
This code is duplicated across four CMakeLists.txt files; consider moving it to a shared CMake file and including it where needed. Also, prefer find_package(Threads REQUIRED) and linking Threads::Threads for better portability and modern CMake practices.
Suggested implementation:
include(cmake/ThreadsConfig.cmake)
- Create a shared file at
cmake/ThreadsConfig.cmakewith the following content:find_package(Threads REQUIRED) - In this
CMakeLists.txt, ensure that your target (e.g.,target_link_libraries(your_target PRIVATE Threads::Threads)) is linked toThreads::Threads. - Repeat the
include(cmake/ThreadsConfig.cmake)andtarget_link_libraries(... Threads::Threads)in the other threeCMakeLists.txtfiles as needed. - Adjust the path to
ThreadsConfig.cmakeif your project structure differs.
Modify compilation issues
Summary by Sourcery
Build: