Skip to content

Add rcpputils for find_library #57

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
Merged
merged 8 commits into from
Jan 22, 2020
Merged

Add rcpputils for find_library #57

merged 8 commits into from
Jan 22, 2020

Conversation

@EricCousineau-TRI
Copy link
Contributor

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

Compadre of ros2/rcpputils#4

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
@nuclearsandwich nuclearsandwich removed the in review Waiting for review (Kanban column) label Aug 15, 2019
dirk-thomas
dirk-thomas reviewed Sep 6, 2019
View reviewed changes
@dirk-thomas
Copy link
Member

dirk-thomas commented Sep 6, 2019

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

@dirk-thomas dirk-thomas mentioned this pull request Sep 6, 2019
Merged
@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Sep 25, 2019

Sorry for the delay; will do that now (especially given that you've already finished the upstream PR - thank you!)

dirk-thomas
dirk-thomas reviewed Oct 7, 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.

Please also rebase against the latest state of the target branch.

@dirk-thomas dirk-thomas added enhancement New feature or request requires-changes and removed backlog labels Oct 7, 2019
dirk-thomas
dirk-thomas reviewed Oct 8, 2019
View reviewed changes
@dirk-thomas
Copy link
Member

dirk-thomas commented Oct 8, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

dirk-thomas
dirk-thomas reviewed Oct 8, 2019
View reviewed changes
@dirk-thomas
Copy link
Member

dirk-thomas commented Oct 9, 2019

The patch breaks Windows - see e.g. https://ci.ros2.org/job/ci_windows/8221/testReport/junit/(root)/projectroot/test_client/

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 10, 2019

The patch breaks Windows - [...]

Unfortunately, I am very slow in debugging on Windows; given other obligations, it may take me a week to circle back to this. The main change that I see triggering this is to change to use get_env, rather than using the builtin _dupenv_s, where we are now doing additional error checking.

@dirk-thomas
Copy link
Member

dirk-thomas commented Oct 11, 2019

it may take me a week to circle back to this.

Just a note: the Eloquent freeze is on Fri Oct 18th.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 17, 2019 •
edited
Loading

Can't access a Windows workstation atm. Will simply revert the getenv changes, but leave the dependencies and a TODO since it's just an internal implementation detail.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 17, 2019

Pushed update.

@EricCousineau-TRI @dirk-thomas
Add rcpputils for find_library
700c70b 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI @dirk-thomas
Address review
9592414 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI @dirk-thomas
Fix dependency declarations
d91c655 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI @dirk-thomas
Fix errors
b350613 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI @dirk-thomas
fix CMake ordering
c903813 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI @dirk-thomas
functions: Revert back to custom get_env_var
9752a8f 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@dirk-thomas dirk-thomas force-pushed the issue/rcpputils_3 branch 2 times, most recently from 7935c32 to c4ccfbf Compare January 15, 2020 22:06
@dirk-thomas
use rcutils_get_env, remove added namespace
1b54007 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas
export new dependencies when necessary
8b1706a 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas mentioned this pull request Jan 17, 2020
Merged
@dirk-thomas
Copy link
Member

dirk-thomas commented Jan 17, 2020 •
edited
Loading

I updated the patch to remove the custom get env logic in favor of using rcutils_get_env() instead. I also removed the unnecessarily added namespace.

CI builds together with ros2/rcutils#201:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed requires-changes labels Jan 17, 2020
dirk-thomas
dirk-thomas approved these changes Jan 17, 2020
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.

Waiting for ros2/rcutils#201.

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Jan 18, 2020

Awesome, thank you!

@dirk-thomas dirk-thomas merged commit 2d5e1e1 into ros2:master Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request in review Waiting for review (Kanban column)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EricCousineau-TRI @dirk-thomas @nuclearsandwich @tfoote