-
Notifications
You must be signed in to change notification settings - Fork 45
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
Change header install path #213
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for working on this. I tried now to use your patch. The idea is then to include this with
#include "gpl_cmake_target_name.hpp"
in the same package as well as in dependent packages?
Maybe
#include "package_name/gpl_cmake_target_name.hpp"
would be a better choice? (or both, otherwise existing packages will break).
Currently, the dependencies of GPL are not exported (rsl + parameter_traits). A simple find_package of the package including the package creating the GPL-library is not sufficient. Could they be exported as well?
Hi, thanks for the feedback. I pushed a little patch as I noticed this first version did not work in all cases. Now, say you have You need to link you libs and executables against the generated target to access the include file, and export it so that it can be accessed from other packages. Finally, you need to add find_package(ament_cmake REQUIRED)
find_package(generate_package_library REQUIRED)
generate_package_library(param_lib path_to_param_file.hpp)
# compiled library
add_library(lib1 PUBLIC lib1.cpp)
target_include_directories(lib1 PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
target_link_libraries(lib1 PUBLIC param_lib)
# header only library
add_library(lib2 INTERFACE)
target_include_directories(lib2 INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
target_link_libraries(lib2 PUBLIC param_lib)
# export targets
install(TARGETS lib1 lib2 param_lib EXPORT ${PROJECT_NAME}Targets)
ament_export_targets(${PROJECT_NAME}Targets HAS_LIBRARY_TARGET)
ament_export_dependencies(generate_parameter_library ...)
ament_package() The generated header can be included in #include <package1/param_file.hpp> The include line will be the same for other packages. The current solution breaks existing packages. I can improve it so that it does not, at the cost of duplicating the generated header in the build folder (seems fine to me). I can also try to avoid exporting generate_parameter_library as a dependency, which, I agree, would be ideal. Any thoughts ? |
The easier it is to use generate_parameter_library, the better. If there is a way to avoid additional cmake stuff in the library, which generates the code, I would vote for it. But maybe there is an argument to not exporting it, if it is not reused in a different package -> then I'm fine for a manual export step together with clear instructions in the docs (which is missing now anyways).
This is great.
Could we deprecate the files in the "old" path with some compile-time warning? and then remove that after a couple of releases? |
My last push :
In the next push, I will propose an updated documentation and an extra example package using the generated header from generate_parameter_library_example. The only thing I haven't tried is the python version, since we don't use python at the moment. |
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.
Thanks for the followup, I just tested this within my projects:
It works as described, but I had to fight CMake to export that as expected (see my comments below).
Furthermore, the include paths in the old examples have to be updated:
from src/generate_parameter_library/example/src/minimal_publisher.cpp:29:
/build/generate_parameter_library_example/include/admittance_controller_parameters.hpp:1:179: note: ‘#pragma message: #include "admittance_controller_parameters.hpp" is deprecated. Use #include <generate_parameter_library_example/admittance_controller_parameters.hpp> instead.’
1 | #pragma message("#include \"admittance_controller_parameters.hpp\" is deprecated. Use #include <generate_parameter_library_example/admittance_controller_parameters.hpp> instead.")
example_external/CHANGELOG.rst
Outdated
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 think you don't have to fill it -> this will be filled during the next release.
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.
Ok, removed the content of the file. Also corrected the old headers.
install(TARGETS my_lib | ||
EXPORT ${PROJECT_NAME}Targets | ||
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
RUNTIME DESTINATION lib/${PROJECT_NAME}) | ||
|
||
ament_export_targets(${PROJECT_NAME}Targets HAS_LIBRARY_TARGET) |
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.
The equivalent of these two instructions have to be added in the library, which exports the turtlesim_parameters. Please add this to this section here (it is not part of the minimum CMakeLists snippet)
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.
The other option is to hide these instructions in the generate_parameter_library cmake command. Which one do you prefer ?
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.
would there be any drawbacks?
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.
The only effect would be to export the generated header target even if you don't use it outside of your package, which sounds fine to me (same thing with exported dependencies). The main advantage is to group everything under the generate_parameter_library cmake command, which makes it very easy to use.
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.
again, I vote for the simplest version to use ;) but I'd understand if someone wants to keep the created artifacts as small as possible
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.
Done. To achieve this, I transformed the function into a macro, so that I don't need to globalize every cmake variable set in the function.
Hi @christophfroehlich, I have pushed something to correct the cmake linter error. colcon test now runs without errors, at least locally. |
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 just have tested this again. Could you comment on the changes of your last commit? Where is the problem with a breaking change?
I needed to add the equivalent of
install(TARGETS ${LIB_NAME} EXPORT ${PROJECT_NAME}Targets)
ament_export_targets(${PROJECT_NAME}Targets HAS_LIBRARY_TARGET)
to my code to get that exported, the snippet from the README without ament_export_targets() did not work for me.
README.md
Outdated
@@ -63,6 +63,9 @@ target_link_libraries(minimal_node PRIVATE | |||
rclcpp::rclcpp | |||
turtlesim_parameters | |||
) | |||
|
|||
install(TARGETS minimal_node turtlesim_parameters | |||
EXPORT ${PROJECT_NAME}Targets |
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.
there is a closing parenthesis missing.
Could you explain the difference of
EXPORT ${PROJECT_NAME}Targets
here
and
EXPORT export_generate_parameter_library_example
from the example/CMakeLists.txt please?
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.
Hi,
So the reason why I made these changes is that we tried to package many ros2 packages, and that some failed because of the changes I made in the first place.
The problem with my first implementation was that the generated parameters target was exported automatically by the cmake macro under the name ${PROJECT_NAME}Targets
. Say you had another target, like a library, using the generated parameters one, and exported under a different name than ${PROJECT_NAME}Targets
, cmake would raise an error saying that the generated parameter target was exported multiple times.
So the solution I found was to remove the automatic target export from the macro. So instead of
generate_parameters(params_target params_file.yaml)
add_library(my_lib my_lib.cpp)
target_link_libraries(my_lib SHARED params_target)
install(TARGETS my_lib EXPORT WhatheverExportNameYouWant)
ament_export_targets(WhatheverExportNameYouWant HAS_LIBRARY_TARGET)
You now need to specify params_target
in the TARGETS
list.
This actually make sense : similarly to add_library
, the generate_parameters
macro creates a cmake target. It is up to you to export it if you want, and under whatever name you want.
To answer your last question, these are simply names. You can write whatever you want. You could export multiple targets under different names I guess, although in small packages you tend to export all targets under a single name. At least that's what I encountered so far. ${PROJECT_NAME}Targets
will simply expand to MyProjectTargets
if your cmake project is called MyProject
.
To conclude, this new implementation requires slightly more work since you need to export the generated parameters target manually if you want to use it outside of your package. But it does not break current implementation in many packages such as ros2_control
, which is better if you want this path to be released quickly, and backported (no additional work required) to humble.
Do not hesitate if you have more questions or comments.
Hi everyone,
Thanks for the great work behind generate_parameter_library ! It definitely spares us many lines of code.
A problem met by myself and some users according to #180 and #204 is that the generated header cannot be easily used outside the package that generated them. So I've made a small change to fix this. I'm open to discuss this more in depth if necessary, and add tests if required.
This PR solves #180 and #204 by installed the generated header in the same include folder as the current package, so that it can be used in other packages, and not only in the current one.