Specify scope of link libraries

[ChrisThrasher]

We know these libraries are only build requirements and not usage requirements so we can explicitly specify as such. This will allow users of these targets to also use these scope keywords in their own target_link_libraries calls instead of being forced to omit them as is the case currently.

Closes #405

[tylerjw]

Thank you for this!

[mjeronimo]

• Linux
• Linux-aarch64
• Windows

[ChrisThrasher]

Looks like there are more downstream packages broken which is what I expected. In the way that I had to fix ament_cmake_gen_version_h/CMakeLists.txt, many other callsites will also need to be fixed.

[ChrisThrasher]

Are we willing to merge this knowing it will require tons of downstream projects to fix their code? I know that's not a small ask but I think it's worth fixing this issue.

[clalancette]

Are we willing to merge this knowing it will require tons of downstream projects to fix their code?

I think we need to know the scope of the breakage. At the very least we'll have to fix all of the core code at the same time (so CI continues to run), but we should also figure out how many packages in Rolling are going to be affected.

[tylerjw]

I think we need to know the scope of the breakage. At the very least we'll have to fix all of the core code at the same time (so CI continues to run), but we should also figure out how many packages in Rolling are going to be affected.

How do we go about doing that test? Is it documented somewhere? What is included in "core code"? I know technically we could stand up our own copy of the Jenkins server but I'm hoping for something easier to run locally to work through fixing this.

[mjcarroll]

In order to build/test a collection of repositories via the CI, the easiest approach for you is going to be make all of your changes in forks/branches and post a custom ros2.repos file here for someone to run through CI.

We consider the entire contents of ros2.repos to be "core".

If there are additional downstream breakages outside of that, that may be something else that needs to be considered before merging (Are all gtest consumers across all ros2 packages broken by this?)

Outline of steps

1. Begin by making a copy of the current ros2.repos in a gist or your own fork of the ros2/ros2 repository.
2. Update URIs and branch names to reflect your PR branches.
3. Post a link to the raw github content of the gist/fork here and someone with CI access can launch the job.

Typically with green CI and approvals across all PRs we will start merging from the bottom-up to minimize buildfarm churn.a

[ChrisThrasher]

that may be something else that needs to be considered before merging (Are all gtest consumers across all ros2 packages broken by this?)

Probably yes. CMake will let you consistently omit scope keywords or requires you always use them. Because ament_camke omitted them, users weren't able to use scope keywords for any extra link libraries they added. Now that we're adding the scope keywords that should have always been there, we're forcing users to fix their code and add scope keywords. The fixes are mostly trivial but they need to happen in many, many places.

[clalancette]

Probably yes. CMake will let you consistently omit scope keywords or requires you always use them. Because ament_camke omitted them, users weren't able to use scope keywords for any extra link libraries they added. Now that we're adding the scope keywords that should have always been there, we're forcing users to fix their code and add scope keywords. The fixes are mostly trivial but they need to happen in many, many places.

If you want to know the true scope of the breakage, you can download all source code that is currently released into Rolling by doing something like the following:

rosinstall_generator --repos ALL --rosdistro rolling --deps

[clalancette]

We discussed this, and we think that the way forward here is to add an optional argument that allows the user to specify the scoping keyword (PUBLIC, PRIVATE, INTERFACE). If the argument is not specified, then the current behavior would be maintained.

This would allow us to slowly start using the options in packages where they are wanted, without breaking all downstream packages. Eventually, if we get enough packages to switch over to this, we could in the future make that a required argument.