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

Convert to Ament Build System #22

Closed
davetcoleman opened this issue Feb 22, 2019 · 5 comments
Closed

Convert to Ament Build System #22

davetcoleman opened this issue Feb 22, 2019 · 5 comments

Comments

@davetcoleman
Copy link
Member

davetcoleman commented Feb 22, 2019

Create one or several PRs that apply the same conversions from Catkin to Ament across all moveit repo-based MoveIt packages. Here's a relevant debate on how to do this for moveit2

@vmayoral
Copy link
Contributor

We can take care of this ticket, at least to some extend. Work in progress at https://github.com/AcutronicRobotics/moveit2#milestones

mlautman pushed a commit that referenced this issue Mar 8, 2019
@henningkayser
Copy link
Member

It still seems to be an open issue on how to add multiple dependencies, libraries or other package based properties without adding too much redundant code inside CMakeLists.txt files.
With catkin we simply could use cakin_LIBRARIES, catkin_INCLUDE_DIRS etc.
This feature seems to be somewhat lost with ament.
I found two possible solutions for this:

  1. Ament auto:
    This is a package with ament functions for resolving dependencies, creating executables, linking libraries and code generation.
    Especially ament_auto_find_build_dependencies looks very interesting, since generates variables for properties like package names, libraries, or include directories and calls find_package() for build and buildtool dependencies. The values are loaded from the package.xml so we wouldn't have to define dependencies in the CMakeLists.txt anymore. Related functions ament_auto_add_executable, ament_audo_add_library and ament_auto_generate_code handle all dependencies and linking with the defined variables and ament_auto_package packages all targets accordingly.
    A downside might be that these functions make certain assumptions that are not always in moveit packages so we might have to adjust the packages or modify the scripts.
  2. Use of custom functions:
    Similar to here we could define functions for libraries and executables that rely on the same dependencies. Of course we could still combine these with auto functions from above. For example we could run ament_auto_find_build_dependencies to load all variables from the package.xml and then use these to link libraries, add dependencies or set include directories.

@wjwwood
Copy link
Contributor

wjwwood commented Mar 11, 2019

cakin_LIBRARIES/catkin_INCLUDE_DIRS has the same issue as catkin_simple/ament_cmake_auto, which is that if you have (as an example) dep1 and dep2, and a library lib1 and an executable exe1, and lib1 uses dep1 only but the the exe1 uses both dependencies, then you'll be overlinking lib1 if you use ament_cmake_auto or catkin_LIBRARIES. If you don't care about that then you can use ament_cmake_auto or you could use the variables it uses/creates:

Those could be used to either automate the find_package(...) calls and/or collecting the names of dependencies based on the package.xml.

@davetcoleman
Copy link
Member Author

then you'll be overlinking lib1

The downside here being simply that it compiles a little bit slower?

@wjwwood
Copy link
Contributor

wjwwood commented Mar 16, 2019

then you'll be overlinking lib1

The downside here being simply that it compiles a little bit slower?

Yes, and also at runtime when the linker has to look up libraries to load.

The extra links make it harder to debug, if you're looking a shared library and it has lots of libraries then you need to look at each of them to figure out where a symbol lives, or if you're a package manager like apt or Homebrew then automatic pruning of packages becomes more complicated as you may have libraries that aren't even used.

There are some ways to automatically handle it with "as needed" like options, but they're not perfect.

Also, if you're building on Windows or building static libraries/applications then having lots of redundant links can make linking order a problem.

Finally, it's just sloppy, and someone coming along afterwards may be left thinking "is this library linked for a reason or was it just carelessness"? That can make debugging an issue more difficult. For a similar reasons many style guides and linters encourage you to only include what you use in C++ or import what you use in Python.

@davetcoleman davetcoleman mentioned this issue Dec 3, 2019
7 tasks
MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this issue Aug 15, 2022
* minor fixup to prerequisites

* language cleanup
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

No branches or pull requests

4 participants