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.

Sign up for GitHub

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

Jump to bottom

Add rcpputils for find_library #47

Merged
merged 1 commit into from
Feb 1, 2020
Merged

Add rcpputils for find_library #47

merged 1 commit into from
Feb 1, 2020

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Mar 25, 2019 •
edited by mjcarroll
Loading

Signed-off-by: Eric Cousineau [email protected]

Compadre of ros2/rcpputils#4

Er... This is namely for crystal. How do I handle backporting from / forward porting to master?

This change is Reviewable

Connects to ros2/rcpputils#4

@tfoote tfoote added the in review Waiting for review (Kanban column) label Mar 25, 2019
@EricCousineau-TRI EricCousineau-TRI mentioned this pull request Mar 25, 2019
Closed
@dirk-thomas
Copy link
Member

This is namely for crystal. How do I handle backporting from / forward porting to master?

Please target the default branch with any pull request (except if the change is only applicable to the target branch).

In this specific case since the change is only a cleanup there is no need to backport it for Crystal so it is unlikely that this is going to happen.

@EricCousineau-TRI
Copy link
Contributor Author

In this specific case since the change is only a cleanup there is no need to backport it for Crystal so it is unlikely that this is going to happen.

I'm actually using this to lead into ros2/rcutils#143, hoping to get some mileage out of that in Crystal ;P
But will target default branch for now - thanks!

@EricCousineau-TRI EricCousineau-TRI changed the base branch from crystal to master March 25, 2019 21:21
@dirk-thomas dirk-thomas mentioned this pull request Sep 6, 2019
Merged
dirk-thomas
dirk-thomas reviewed Sep 6, 2019
View reviewed changes
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EricCousineau-TRI Just checking if you want to pick this up again with the underlying PR ready?

rosidl_typesupport_cpp/package.xml Outdated
@@ -9,6 +9,7 @@

<buildtool_depend>ament_cmake_ros</buildtool_depend>

<build_depend>rcpputils</build_depend>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rcpputils also needs to be available at runtime since it provides a shared library. Therefore this should use the depend tag instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where? It still is only a build_depend in this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, only did it for the C package in 2c29cb2. Will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e8b0dd3

@EricCousineau-TRI
Copy link
Contributor Author

Working on this now; should I merge in updated branch, rebase, or close and open a new PR?

@dirk-thomas
Squashed commit of the following:
12cdc45 
commit e8b0dd3
Author: Eric Cousineau <[email protected]>
Date:   Mon Oct 7 13:28:05 2019 -0400

    Forgot to update cpp package

    Signed-off-by: Eric Cousineau <[email protected]>

commit 2c29cb2
Author: Eric Cousineau <[email protected]>
Date:   Wed Sep 25 16:40:50 2019 -0400

    Address review

    Signed-off-by: Eric Cousineau <[email protected]>

commit e5d7f0c
Merge: 814b0a8 38eb801
Author: Eric Cousineau <[email protected]>
Date:   Wed Sep 25 16:39:52 2019 -0400

    Merge remote-tracking branch 'upstream/master' into issue/rcpputils_3

    Signed-off-by: Eric Cousineau <[email protected]>

commit 814b0a8
Author: Eric Cousineau <[email protected]>
Date:   Mon Mar 25 17:08:35 2019 -0400

    Add `rcpputils` for `find_library`

    Signed-off-by: Eric Cousineau <[email protected]>

Signed-off-by: Dirk Thomas <[email protected]>
@dirk-thomas
Copy link
Member

Rebased and resolved conflicts:

@dirk-thomas dirk-thomas merged commit cf99ca8 into ros2:master Feb 1, 2020
@clalancette clalancette mentioned this pull request Jan 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

Assignees
No one assigned
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EricCousineau-TRI @dirk-thomas @tfoote