ROS2 docs still recommend usage of deprecated ament_export_include_directories, _definitions, _libraries, _link_flags

[RFRIEDM-Trimble]

As part of https://github.com/ament/ament_cmake/issues/365, I noticed all these functions are deprecated, however the docs still recommend users to use them.

Should we start recommending using just native CMake in the migration guide. Also in scope would be the use of ament_target_dependencies.

My proposal is start with recommending to use standard CMake builtin functions whenever possible instead of ament.

I'm happy to submit a PR once the acceptance criteria for this ticket are decided.

[clalancette]

So as far as I know, we never actually deprecated those old-style CMake functions. I think we could do this, but it hasn't yet been done. However, we still use these functions heavily in the core and in the ecosystem, so deprecating them would be a non-trivial task.

That said, I think that the updates in https://github.com/ros2/ros2_documentation/pull/2915 (along with some of my comments) can reflect the current best practices. That is, we recommend you use CMake targets, but we also still support the old CMake way of doing things.

I think the way forward for now is to update that PR, and get that in.

[RFRIEDM-Trimble]

Right, any way I can help? Another pass over that PR to verify consistency with your statement above?

[clalancette]

Right, any way I can help? Another pass over that PR to verify consistency with your statement above?

Yeah, that's the most we can do and to poke @sloretz with a friendly ping :).

[Ryanf55]

@clalancette Unless you are strongly opposed, I'd like to deprecate some of these calls on rolling with message(AUTHOR_WARNING ..) and help submit PR's against the ROS core to remove these features in time for Jazzy release.

[clalancette]

@clalancette Unless you are strongly opposed, I'd like to deprecate some of these calls on rolling with message(AUTHOR_WARNING ..) and help submit PR's against the ROS core to remove these features in time for Jazzy release.

So, first of all, we definitely can't remove these for Jazzy. Our policy is to tick-tock, where in one release we deprecate a feature, and then in the next release we remove it. So the earliest that this could possibly be removed is for K-Turtle.

However, I think even that is very ambitious. Just looking around the packages listed in Rolling, I see the following:

$ grep -rI "ament_export_include_directories" * | wc -l
260
$ grep -rI "ament_export_definitions" * | wc -l
5
$ grep -rI "ament_export_libraries" * | wc -l
242
$ grep -rI "ament_export_link_flags" * | wc -l
8

I don't think we should carelessly do this deprecation and cause a lot of downstream heartache like this.

Instead, I suggest the following:

1. Make sure that none of the core packages (those listed in https://github.com/ros2/ros2/blob/rolling/ros2.repos) depend on any of these features. If they do, submit pull requests to fix them.
2. Make sure that our documentation in this repository doesn't use these features in tutorials/guides. If it does, update those.
3. Deprecate only ament_export_definitions and ament_export_link_flags; there are few enough users of that that it won't cause tons of downstream warnings.
4. Open up a discourse post saying that we are going to deprecate ament_export_include_directories and ament_export_libraries for K-Turtle, and then remove them for L-Turtle.
5. In June (after the Jazzy release), remove ament_export_definitions and ament_export_link_flags, and deprecate ament_export_include_directories and ament_export_libraries. That should give downstream consumers a whole year to fix their code before K-Turtle.

@sloretz What do you think? Does that sound like a reasonable path?

[sloretz]

Sounds like a reasonable path to me!

[Ryanf55]

The plan sounds good.