Skip to content
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

[ament_target_dependencies] pick up imported interfaces #178

Closed
wants to merge 1 commit into from

Conversation

stonier
Copy link

@stonier stonier commented Jun 27, 2019

Problem to Solve

Allow this function to work with packages that only use the more recent export/import target style via ament_export_interfaces and ament_export_dependencies.

…aries

Signed-off-by: Daniel Stonier <d.stonier@gmail.com>
@sloretz
Copy link
Contributor

sloretz commented Jul 19, 2019

Is more needed for transitive dependencies? It looks like ament_export_dependencies() doesn't combine ..._INTERFACES from the packages it find_package()s. If A depends on B which depends on C, and A calls ament_target_depdencies(a_target B), A will target_link_libraries(a_target ${B_INTERFACES}), but I think ${B_INTERFACES} won't contain ${C_INTERFACES}.

@stonier
Copy link
Author

stonier commented Jul 21, 2019

Not sure I'm quite catching your concern.

The exporting/importing of targets is going via ament_export_interfaces (ament_cmake_export_interfaces), not ament_export_dependencies.

Transitive dependencies should be fine - all defns, include paths, libraries, compiler flags get wrapped up in the imported targets and passed along via the target_link_libraries call (admittedly, it's initially confusing that target_link_libraries handles it all now). This is different to the older cmake style where you had to manage them all independently.

@sloretz
Copy link
Contributor

sloretz commented Jul 24, 2019

Not sure I'm quite catching your concern.

I mistakenly thought ament_export_dependencies() would need to make the ${PROJECT_NAME}_INTERFACES variable include targets from transitive dependencies so they too would get passed to the target_link_libraries() call, but it looks like that's not necessary. The targets from transitive dependencies just need to exist. I wrote an example to convince myself: sloretz/ament_export_interfaces_example.

The exporting/importing of targets is going via ament_export_interfaces (ament_cmake_export_interfaces), not ament_export_dependencies.

It looks like ament_export_dependencies() is still required when using ament_export_interfaces(). The variables it creates aren't needed, but the find_package() call it adds makes sure targets from transitive dependencies exist.

@sloretz
Copy link
Contributor

sloretz commented Jul 24, 2019

CI (building all packages to make sure no use of ament_target_dependencies() errors, testing just ament_cmake_target_dependencies for cmake linter)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz
Copy link
Contributor

sloretz commented Jul 24, 2019

CI failed with:

10:32:18 CMake Error at CMakeLists.txt:20 (add_library):
10:32:18   Target "resource_retriever" links to target
10:32:18   "ament_index_cpp::export_ament_index_cpp" but the target was not found.
10:32:18   Perhaps a find_package() call is missing for an IMPORTED target, or an
10:32:18   ALIAS target is missing?

Which is caused by ament_index_cpp calling ament_export_interfaces() with an interface name that it doesn't actually export. That will need to be fixed too. However this brings up another problem that the ..._INTERFACES variable only has valid target names if the "export" name is the same as the target name, and there's just one export per target #183.

This PR seems like the right thing to do, but I think it should go in after a fix for #183. Otherwise it may cause configure-time failures of projects that are currently ignoring the ..._INTERFACES variable of their dependencies.

@dirk-thomas
Copy link
Contributor

@stonier @sloretz What is the status of this ticket?

@sloretz
Copy link
Contributor

sloretz commented Feb 3, 2020

What is the status of this ticket?

@dirk-thomas It looks like this is blocked by #183, and there's been no progress on that as far as I know.

@dirk-thomas
Copy link
Contributor

Thanks for creating this PR. #232 implemented the same change just slightly different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants