Bumped openddw to the latest commit. #16
Conversation
|
Seems that the install of dependencies isn't working on windows. |
Try changing the version of vcpkg listed in the yml file |
|
OK... should be good to go now. |
|
This is a necessary update. Is there something I am missing for merge? |
| @@ -137,6 +137,7 @@ target_link_libraries(monitor | |||
|
|
|||
| target_include_directories(monitor PRIVATE | |||
| ${QWT_INCLUDE_DIR} | |||
| ${PROJECT_SOURCE_DIR}/thirdparty/OpenDDW/src | |||
There was a problem hiding this comment.
Linking to OpenDDW should take care of this. What happens without it?
There was a problem hiding this comment.
It doesn't build without it. The directories don't line up right due to the directory layout.
There was a problem hiding this comment.
In CMake if you have to use target_include_directories on a library you're already using target_link_libraries on means something isn't right in that library's CMake files. What is the exact error?
There was a problem hiding this comment.
The cmake is looking in the install directory which doesn't have the include files until the install phase. That works fine when you are using an installed package. But we need to include the source directory manually for using it as a submodule.
There was a problem hiding this comment.
It's part of the monitor build, so there shouldn't be any reason for it to be looking at the installed location. I built this locally to see what it's doing and I see what it's doing now after looking at the -I options it passed to the compiler. It used to be including src before OpenDDS/OpenDDW#13 and now it's just ${CMAKE_CURRENT_SOURCE_DIR}.
Changing that to ${CMAKE_CURRENT_SOURCE_DIR}/src in OpenDDW allows the monitor to build without this added line. I've created a OpenDDW PR that does this: OpenDDS/OpenDDW#15.
There was a problem hiding this comment.
OK but the source directory isn't necessary to build OpenDDW or to use it from an installed location. Its only a concern if you are using it as a submodule.
There was a problem hiding this comment.
The include would be required for every target in the same build that wants to use OpenDDW. If a test was added to OpenDDW it would have the same problem. It's better to fix it once than to require a manual include for every target.
There was a problem hiding this comment.
I'll remove the directory and bump the submodule once the other pull request is merged.
There was a problem hiding this comment.
Any chance this can be moved along. These changes are about as trivial as they come. It would be nice to have a version that builds again.
|
Also it looks the cached builds of OpenDDS here have the same problem as OpenDDS/OpenDDW#11. They need to checkout the submodules. |
|
What is the path forward on this. |
|
I cleared the build caches and triggered a rerun of the CI. |
|
I think this and PR 17 need to get merged for everything to build in CI. |
|
This was not supposed to be merged, @doug1234, can you please open a new PR to remove the unneeded includes? |
|
I will add a new pr that removes the unneeded includes and bumps to the latest openddw commit. |
No description provided.