Skip to content
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.

Sign up for GitHub

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

Bumped openddw to the latest commit. #16

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Bumped openddw to the latest commit. #16

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d4db784
Bumped openddw to the latest commit.
Oct 2, 2023
7f59478
Bump VS package version
Oct 2, 2023
7a6f1a8
Fix cmake to work with currend openddw.
Oct 3, 2023
7914776
Fixed the test.
Oct 3, 2023
39c18f7
One more tweak
Oct 3, 2023
1b656a5
Dumped openddw version again.
Oct 9, 2023
16e8840
Removed unnecessary unmanaged include dependency.
Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ jobs:
uses: lukka/run-vcpkg@v10
with:
vcpkgDirectory: '${{ github.workspace }}/vcpkg'
vcpkgGitCommitId: 688ece714204fb5e9ad790ad9ad6d9f571d2b032
vcpkgGitCommitId: 9edb1b8e590cc086563301d735cae4b6e732d2d2
runVcpkgInstall: true

# Set Up Build Environments
Expand Down
7 changes: 4 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ endif()
find_package(Qt5 REQUIRED COMPONENTS Core Widgets Gui PrintSupport Svg OpenGL OPTIONAL_COMPONENTS ${qt_optional_components})

if(${QWT_IS_LOCAL})
set(QWT_LIBRARY ${CMAKE_SOURCE_DIR}/qwt/lib/qwt$<$<CONFIG:DEBUG>:d>.lib)
set(QWT_INCLUDE_DIR ${CMAKE_SOURCE_DIR}/qwt/src)
set(QWT_LIBRARY ${PROJECT_SOURCE_DIR}/qwt/lib/qwt$<$<CONFIG:DEBUG>:d>.lib)
set(QWT_INCLUDE_DIR ${PROJECT_SOURCE_DIR}/qwt/src)
else()
# FindQwt.cmake is in this directory
list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR})
list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR})
find_package(Qwt MODULE REQUIRED)
endif()

Expand Down Expand Up @@ -137,6 +137,7 @@ target_link_libraries(monitor

target_include_directories(monitor PRIVATE
${QWT_INCLUDE_DIR}
${PROJECT_SOURCE_DIR}/thirdparty/OpenDDW/src
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking to OpenDDW should take care of this. What happens without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't build without it. The directories don't line up right due to the directory layout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the directory and bump the submodule once the other pull request is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

configure_file(opendds.ini . COPYONLY)
Expand Down
2 changes: 2 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ target_link_libraries(managed_testapp

target_include_directories(managed_testapp PRIVATE
../src
../thirdparty/OpenDDW/src
)

add_executable(unmanaged_testapp
Expand All @@ -45,4 +46,5 @@ target_link_libraries(unmanaged_testapp

target_include_directories(unmanaged_testapp PRIVATE
../src
../thirdparty/OpenDDW/src
doug1234 marked this conversation as resolved.
Show resolved Hide resolved
)
2 changes: 1 addition & 1 deletion thirdparty/OpenDDW
Open in desktop
Submodule OpenDDW updated 6 files
+5 −4 .github/workflows/build.yml
+67 −2 CMakeLists.txt
+223 −0 idl2library.cmake
+64 −0 openddw-config.in.cmake
+11 −11 src/dds_manager.cpp
+2 −2 src/dds_simple.h
Loading