use ament_cmake_export_interfaces

[dirk-thomas]

Exports the created library target using ament_cmake_export_interfaces.

[dirk-thomas]

• 
• 
• 

[dirk-thomas]

Waiting for review.

[wjwwood]

What is the purpose of the pr? There's no context here.

[dirk-thomas]

The purpose is to export the library target (instead / beside exporting the include directory and library path). Then the downstream package can use it as an imported target (see referenced PR). This is the "new" way in CMake compared to the "old" way.

[wjwwood]

I can't really tell you if the code is fine, as I've never used most of these features. But I don't recognize anything wrong with the changes.

I still don't get the motivation, you've clarified what the code is doing, but I don't know why you're doing it or why you're just doing it to this target.

Is there something you're unable to do right now that this allows? Are you just trying a new feature out? I have no idea.

[wjwwood]

Why is this necessary? Does $<INSTALL_INTERFACE:include> even exist at build time of this target?

[dirk-thomas]

The BUILD_INTERFACE is being used to build the library. The INSTALL_INTERFACE is being used when downstream packages try to link against this target. See https://cmake.org/cmake/help/v3.5/command/target_include_directories.html

[wjwwood]

Is this necessary, given the code in the ament_export_interfaces code?

https://github.com/ament/ament_cmake/blob/master/ament_cmake_export_interfaces/cmake/ament_cmake_export_interfaces_package_hook.cmake#L28-L33

[dirk-thomas]

This install rule is being "identified" by the name export_${PROJECT_NAME}. ament_export_interfaces(<same_identifier>) creates the necessary part for the CMake config file that the installed targets can be imported in downstream packages.

[wjwwood]

Even after reading the documentation for ament_export_interfaces() I can't tell, is this an input variable or an output variable? Is export_${PROJECT_NAME} set before this is called?

[dirk-thomas]

The argument has to match an EXPORT of an install(..) call. See comment below.

[dirk-thomas]

Using imported targets is a cleaner approach to pass information along to downstream packages. Compared to the classic approach of using *_INCLUDE_DIRS, *-LIBRARIES a downstream package can now:

• link against a specific library if an upstream package provides multiple
• link against the imported target without worrying what is "attached" to it (include dirs, transitive libraries, transitive include dirs, definition, compile flags, anything)
• information are not expanded to absolute paths until they are being used in a downstream package

While ament supported this for a long time we have never used it until now. Imo existing code should in the future move to this pattern instead of the classic approach to leverage these advantages.

[wjwwood]

While ament supported this for a long time we have never used it until now. Imo existing code should in the future move to this pattern instead of the classic approach to leverage these advantages.

I understand and agree it's a cool feature and a better way to expose information, but what you still haven't answered, imo, is why now and why this target in this package rather than all packages or N packages or something else? The motivation seems to be "because".

[dirk-thomas]

I understand and agree it's a cool feature and a better way to expose information, but what you still haven't answered, imo, is why now and why this target in this package rather than all packages or N packages or something else? The motivation seems to be "because".

I try to squeeze in time for reducing technical debt whenever I can. It just happened to be the case that I picked this item (exporting / importing targets). I picked this package since it has exactly one downstream dependency and was therefore easy to convert.

[wjwwood]

I try to squeeze in time for reducing technical debt whenever I can. It just happened to be the case that I picked this item (exporting / importing targets). I picked this package since it has exactly one downstream dependency and was therefore easy to convert.

Ok, that's fine, but in the future, if you want people to review it quickly, I'd recommend you add some context. I hesitated at the time you opened them because I didn't know if it was related to the console logging work or some other issue you were debugging and I didn't know if more was to come. If it's just trying it out or burning down some technical debt, just say so.

[dirk-thomas]

I hesitated at the time you opened them because I didn't know if it was related to the console logging work or some other issue you were debugging and I didn't know if more was to come.

I understand the "in review" label that it means that a PR is ready to be reviewed and mergable. If that is not yet the case the ticket explicitly mentions something like "just for feedback" or "please review but more is coming".

[wjwwood]

I'm just telling you why I didn't review it immediately, which was there was no context or motivation so I was uncertain as to its purpose or priority. I guess others felt the same way as no one else reviewed it. You could avoid this in the future by being more generous with details from the onset. It's just advice you don't have to follow it.