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

Check for exported packages in build(tool)-export depends, not run-depends. #790

Closed

Conversation

NikolausDemmel
Copy link
Contributor

  • For format 1 packages this makes no difference, since build-export-depends
    are equal to run-depends and buildtool-export-depends are empty.
  • For format 2 packages this has two aspects: Firstly, it now correctly fails if
    a package that is exported in catkin_package is listed only as an
    exec-depend, and not a build(tool)-export depend. Secondly, it now does not
    fail, if a package exported by catkin_package is listed only as a
    buildtool-export-depend.

…pends.

- For format 1 packages this makes no difference, since build-export-depends
  are equal to run-depends and buildtool-export-depends are empty.
- For format 2 packages this has two aspects: Firstly, it now correctly fails if
  a package that is exported in `catkin_package` is listed only as an
  exec-depend, and not a build(tool)-export depend. Secondly, it now does not
  fail, if a package exported by `catkin_package` is listed only as a
  buildtool-export-depend.
@NikolausDemmel
Copy link
Contributor Author

It should be noted, that this might break the compilation of format2 packages that export a package in catkin_package but list it only as an exec_depend not build(tool)_export_depend. Those packages are broken and should be fixed, but they still compile with the current version of catkin.

@NikolausDemmel
Copy link
Contributor Author

If we should only export build(tool)-export depends as suggested in this PR, then the documentation has a bug: It suggests to add message_runtime only as exec-depend, but then also to export it in catkin_package as a CATKIN_DEPENDS. Either we need to add it as build-export-depend, or remove it from CATKIN_DEPENDS. I think the latter. Opinions?

@dirk-thomas
Copy link
Member

message_runtime depends on e.g. rostime which needs to export the library with the same name. Therefore it needs to exported for downstream packages to link against. Since the library is for the target system that is a build export dependency. So removing it from CATKIN_DEPENDS is not an option.

That library is also being used at runtime therefore it is also an exec dependency.

@NikolausDemmel
Copy link
Contributor Author

So the documentation should be changed to either

<depend>message_runtime</depend>

or

<build_export_depend>message_runtime</build_export_depend>
<exec_depend>message_runtime</exec_depend>

Any preferences?

I have a very slight preference for the former because of simplicity. The extra build-depend is superfluous (?), but shouldn't hurt much I guess. But either way is fine IMO.

@dirk-thomas
Copy link
Member

The documentation shouldn't recommend to add message_runtime as a build dependency. Therefore option 2.

NikolausDemmel added a commit to NikolausDemmel/catkin that referenced this pull request Mar 29, 2016
dirk-thomas pushed a commit that referenced this pull request Mar 29, 2016
@NikolausDemmel
Copy link
Contributor Author

It should be noted, that this might break the compilation of format2 packages that export a package in catkin_package but list it only as an exec_depend not build(tool)_export_depend. Those packages are broken and should be fixed, but they still compile with the current version of catkin.

I think we will have to go with a mere warning in such cases to not break builds, at least for a transition period.

@dirk-thomas
Copy link
Member

Closing due to inactivity.

@dirk-thomas dirk-thomas closed this Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants