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

Prepare dependencies for bouncy release. #41

Merged
merged 4 commits into from Jun 23, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -11,11 +11,20 @@

<build_depend>rcutils</build_depend>
<build_depend>rmw</build_depend>
<build_depend>rmw_connext_cpp</build_depend>
<build_depend>rmw_fastrtps_cpp</build_depend>
<build_depend>rmw_opensplice_cpp</build_depend>

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 21, 2018

Member

Why should these be added upstream? The group dependency below already covers them.

This comment has been minimized.

Copy link
@nuclearsandwich

nuclearsandwich Jun 21, 2018

Author Member

They're redundant with upstream's use of the group tag so they don't change upstream behavior. The other dependency work for this repository will happen after package.xmls are handled so with these changes upstream no package.xml overlay is needed in the release repo.

But since they are redundant I can instead add them to an overlay or patch in the release repositor if that is preferred.

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 21, 2018

Member

It makes sense to add them here is that avoid patching the manifest in the release repo since it doesn't affect the behavior.

Please add a comment above these three line describing why they are in there.


<depend>libpoco-dev</depend>
<depend>poco_vendor</depend>
<depend>rmw_implementation_cmake</depend>
<depend>rmw_fastrtps_cpp</depend>
<!--
Rather than a hard-dependency on any one rmw implementation.
The debian/control template for this package will be modified to depend on either
rmw_fastrtps_cpp or rmw_connext_cpp or rmw_opensplice_cpp with rmw_fastrtps_cpp being
the default choice if no other rmw implementation is currently installed.
<depend>rmw_fastrtps_cpp</depend>

This comment has been minimized.

Copy link
@nuclearsandwich

nuclearsandwich Jun 21, 2018

Author Member

Should I comment this out since there won't be an absolute dependency on rmw_fastrtps_cpp or leave it in for from-source builds?

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 21, 2018

Member

Since the Debian package shouldn't have a hard dependency on FastRTPS it shouldn't be commented in.

I am not sure if the comment here is a good idea. It solely describes packaging customizations. Having them duplicated here just adds the risk of them getting out-of-sync while not adding much value here.

This comment has been minimized.

Copy link
@nuclearsandwich

nuclearsandwich Jun 22, 2018

Author Member

Since the Debian package shouldn't have a hard dependency on FastRTPS it shouldn't be commented in.

I don't quite follow the outcome of this sentence. Should I restore the dependency which will be modified rather than removed at packaging time or leave it commented while modifying the comment above it to reduce bitrot?

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 22, 2018

Member

I would remove this whole comment since it is packaging specific.

-->

<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.