-
Notifications
You must be signed in to change notification settings - Fork 7.3k
ecal update to 6.0.1, tcp-pubsub update to 2.0.1 #49552
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: master
Are you sure you want to change the base?
Conversation
The yaml-cpp library has been added to the list of dependencies in the ecal vcpkg.json file to ensure it is available during the build process.
dg0yt
left a comment
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 know it is a draft. Feel free to ignore the early comments in early stages.
| set(eCAL_install_bin_dir ${CMAKE_INSTALL_BINDIR}) | ||
| set(eCAL_install_config_dir ${CMAKE_INSTALL_SYSCONFDIR}/ecal) | ||
| -set(eCAL_install_cmake_dir ${CMAKE_INSTALL_LIBDIR}/cmake/eCAL) | ||
| +set(eCAL_install_cmake_dir share/eCAL) |
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.
This change might be not needed, because it is would be handled via the CONFIG_PATH option to vcpkg_cmake_config_fixup().
(In addition, the config should eventually land in the lower-case share/ecal dir which already exists for vcpkg stuff.)
| +if(tclap_INCLUDE_DIR-NOTFOUND) | ||
| + message(FATAL_ERROR "Could not find tclap library") | ||
| + set(tclap_FOUND FALSE) | ||
| +else() | ||
| + set(tclap_FOUND TRUE) | ||
| +endif() |
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.
This a Find module. It shouldn't do FATAL_ERROR and tclap_FOUND like this. find_package_handle_standard_args does the right thing.
Adding Find modules in a patch is at least unusual, and the imported targets will need find_dependency in the installed cmake config.
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.
Adding Find modules in a patch is at least unusual, and the imported targets will need find_dependency in the installed cmake config.
the ecal port use a lot of find_package(tclap) function in CMakeLists,is it ok to change all find_package(tclap) to find_path(tclap_INCLUDE_DIR NAMES tclap/Arg.h) and target_include_directories... , or some easier ways?
also need the same thing for termcolor
| -else (MSVC) | ||
| -set(cmake_functions_install_cmake_dir "${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}-${PROJECT_VERSION}") | ||
| -endif (MSVC) | ||
| +set(cmake_functions_install_cmake_dir "share/${PROJECT_NAME}") |
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 see, another place dealing with cmake config, now in "thirdparty".
| ) | ||
|
|
||
| vcpkg_cmake_install() | ||
| vcpkg_cmake_install(ADD_BIN_TO_PATH) |
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.
ADD_BIN_TO_PATH... Does it run host tools? If yes, where do they come from?
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.
oh, the ecal_core is a dll before... so the ecal_generate_config tool links ecal_core and runs when build, after changed ecal_core to build as static lib, seems no need ADD_BIN_TO_PATH now?
./vcpkg x-add-version --alland committing the result.Fixes #49491