Change header install path

[AugusteBourgois]

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.

[christophfroehlich]

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?

[AugusteBourgois]

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 package1 generating a header param_file.hpp.

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 generate_package_library to your ament_export_dependencies so that other packages have access to GPL's dependencies (rsl, package_traits...). I guess a find_package and ament_export_dependencies` could work, but I'm not sure.

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 lib1.cpp and lib2.hpp using

#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 ?

[christophfroehlich]

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 generate_package_library to your ament_export_dependencies so that other packages have access to GPL's dependencies (rsl, package_traits...). I guess a find_package and ament_export_dependencies` could work, but I'm not sure.

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 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).

The generated header can be included in lib1.cpp and lib2.hpp using

#include <package1/param_file.hpp>

The include line will be the same for other packages.

This is great.

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.

Could we deprecate the files in the "old" path with some compile-time warning? and then remove that after a couple of releases?

[AugusteBourgois]

My last push :

• installs the generated header (and validation header) so that they are accessible via #include <package_name/parameter_file.hpp> from inside and outside the package.
• copies the generated header (and validation header) so they they remain accessible via #include "parameter_file.hpp" as they used to from inside the same package.
• deprecates the local generated header, so that a warning is printed at build time.
• exports the generated header dependencies (fmt...) so that there is no need for ament_export_dependencies(generate_parameter_library) anymore.

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.

[christophfroehlich]

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.")

[christophfroehlich]

I think you don't have to fill it -> this will be filled during the next release.

[AugusteBourgois]

Ok, removed the content of the file. Also corrected the old headers.

[christophfroehlich]

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)

[AugusteBourgois]

The other option is to hide these instructions in the generate_parameter_library cmake command. Which one do you prefer ?

[christophfroehlich]

would there be any drawbacks?

[AugusteBourgois]

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.

[christophfroehlich]

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

[AugusteBourgois]

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.

[AugusteBourgois]

Hi @christophfroehlich, I have pushed something to correct the cmake linter error. colcon test now runs without errors, at least locally.