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

Also include buildtool-depends in find_package(catkin ...) #31

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

NikolausDemmel
Copy link
Member

This PR fixes #30 and #7.

It depends on ros/catkin#790.

Changes include:

  • Packages found with find_package that are no cmake packages are now used in
    include_directories and automatically linked to, using the standard
    ..._INCLUDE_DIRS and ..._LIBRARIES variables. Moreover, they are exported
    by being passed as DEPENDS in catkin_package.
  • Buildtool-depends are now also find-packaged.
  • cs_export now also has an option flag-argument ALL_DEPS_REQUIRED.
  • cs_export uses build(tool)-export depends, not run-depends to determine what
    is exported in the catkin_package call.
  • There is now an informative message about build(tool)-(export) depends that
    are listed in package.xml, but not found with find_package. This makes is
    easier to track down typos in catkin package names.
  • package.xml is now format 2

It is not ready to be merged, since it needs some tests at the very least.

@NikolausDemmel NikolausDemmel changed the title Also include buildtool-depends in find_package(catkin ...) Also include buildtool-depends in find_package(catkin ...); fixed #30 Mar 14, 2016
@NikolausDemmel NikolausDemmel changed the title Also include buildtool-depends in find_package(catkin ...); fixed #30 Also include buildtool-depends in find_package(catkin ...) Mar 14, 2016
@wjwwood
Copy link
Member

wjwwood commented Mar 17, 2016

I think that linking the exported targets to executables and libraries could be potentially dangerous. Since a buildtool depend could be providing the host libraries rather than the target system's libraries. This might have been why we didn't include it from the start.

By message generation you mean they are included as dependencies in message generation? In ROS 1, this should be fine since messages only result in python code or headers, but if for example messages suddenly needed to export libraries as well (so that your message's libraries needed to link against the other package's message libraries) that might be dangerous.

I think dynamic reconfigure should be safe. I don't imagine anyway that architecture would affect those activities.

How hard would it be to exclude these activities for just buildtool depends?

Thanks for the pr.

@NikolausDemmel
Copy link
Member Author

What I said was actually not quite right. Let me try again.

With build-depends catkin_simple does the following:

  1. call find_package; if not found, nothing else is done
  2. add it to COMPONENTS in the find_package(catkin ...) call; I have not checked what this implies (probably just calling find_package again and appending the values of each package to catkin_LIBRARIES, catkin_INCLUDE_DIRS, ...)
  3. add the depended package's exported include directories via catkin_INCLUDE_DIRS
  4. add the depended-upon package as a dependency for message generation (if both the depending and depended package have messages)
  5. link all executables against the depended package's exported libraries via catkin_LIBRARIES
  6. add the depended package's exportet targets as dependencies for added executables
  7. link all libraries against the depended package's exported libraries via catkin_LIBRARIES
  8. add the depended package's exportet targets as dependencies for added libraries

I can change the PR to do all, some, or none of these things for buildtool-depends, no problem. Maybe we actually just want to do 1. for buildtool-depends?

What is actually supposed to happen with dependencies that are both build and buildtool depends? Is a double-specification needed in package.xml, or does build-depend imply buildtool-depend?

I'm fishing in the dark here a little bit since I have never used neither catkin nor plain cmake in a cross-compilation situation.

@wjwwood
Copy link
Member

wjwwood commented Mar 18, 2016

I suppose having a key be both a build_depend and a buildtool_depend might indicate that it needs both the host version of the dependency as well as the target version. For example, you could imagine that a package needs the host version of Python as well as the target version. The host version so it can invoke python scripts during the build and the target version so it can link against libpython and embed the target python executable's path in the #! line of any scripts that are installed. That's an extreme example, but just an idea of what it might mean.

@NikolausDemmel
Copy link
Member Author

But buildtool_depend and build_depend are orthogonal? I.e. if I need a package's library during the build I need to specify build_depend regardless of whether I already specified it as buildtool_depend or not. And if I need some cmake extras of a package, I need to specify buildtool_depend, regardless of whether I already specified it as build_depend, right?

Out of the 8 things catkin_simple currently does with build-depends, do you agree that we only want 1. for buildtool-depends (i.e. only find-package them, no more)?

Changes include:

- Packages found with `find_package` that are no cmake packages are now used in
  `include_directories` and automatically linked to, using the standard
  `..._INCLUDE_DIRS` and `..._LIBRARIES` variables. Moreover, they are exported
  by being passed as `DEPENDS` in `catkin_package`.

- Buildtool-depends are now also find-packaged.

- `cs_export` now also has an option flag-argument `ALL_DEPS_REQUIRED`.

- `cs_export` uses build(tool)-export depends, not run-depends to determine what
  is exported in the `catkin_package` call.

- There is now an informative message about build(tool)-(export) depends that
  are listed in package.xml, but not found with `find_package`. This makes is
  easier to track down typos in catkin package names.
@NikolausDemmel
Copy link
Member Author

I updated this PR to only find-package buildtool-depends, and also export buildtool-export-depends, as well as some other changes. I will update the PR description.

It includes the changes discussed in #7 (at least my current understanding) and depends on ros/catkin#790.

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.

find_package buildtool_depends?
2 participants