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

cmake: Add RMW_IMPLEMENTATION_FORCE_POCO #59

Merged
merged 3 commits into from
Mar 26, 2019
Merged

cmake: Add RMW_IMPLEMENTATION_FORCE_POCO #59

merged 3 commits into from
Mar 26, 2019

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Mar 26, 2019 •
edited
Loading

Resolves #58

\cc @dirk-thomas

This change is Reviewable

@tfoote tfoote added the in review Waiting for review (Kanban column) label Mar 26, 2019
@EricCousineau-TRI
cmake: Add RMW_IMPLEMENTATION_FORCE_POCO
9c193e4 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
dirk-thomas
dirk-thomas reviewed Mar 26, 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.

The logic / control flow is pretty hard to read atm. I would suggest to simplify it as follows:

  • if RMW_IMPLEMENTATION_FORCE_POCO and NOT Poco_FOUND: message(FATAL_ERROR ...)
  • RMW_IMPLEMENTATION_USE_POCO = Poco_FOUND
  • if NOT RMW_IMPLEMENTATIONS MATCHES ";" set RMW_IMPLEMENTATION_USE_POCOtoFALSE`
  • if RMW_IMPLEMENTATION_FORCE_POCO set it back to TRUE

rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
@wjwwood
Copy link
Member

wjwwood commented Mar 26, 2019

I'm just curious why _POCO is in the name? The fact that we use poco to implement the shared library loading (rather than using a different dependency or using dlopen directly) is a pretty specific implementation detail. We've already had people asking us to drop the dependency on poco and do this some other way. I wouldn't be surprised if that changed in the near future and in that case the variable name would not make sense any longer. I don't think it should be part of the option name.

Googling briefly, I think the generic term for this is "dynamic loading" (https://en.wikipedia.org/wiki/Dynamic_loading), so maybe RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING is a better option name.

@EricCousineau-TRI
Copy link
Contributor Author

[...] so maybe RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING is a better option name.

SGTM!

@EricCousineau-TRI
Copy link
Contributor Author

Er, also, how do y'all prefer reviews / commit curation?
Do you prefer force pushes with curated commits, or accumulate revision commits until the end?

@EricCousineau-TRI
WIP Address comments
1be6c07 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@EricCousineau-TRI
Copy link
Contributor Author

The logic / control flow is pretty hard to read atm. I would suggest to simplify it as follows:

Done, with minor adjustment for logic of *FOUND* + *FORCE*.

BTW, it would be really nice to make FORCE a tri-state setting: null / unspecified implies use implicit behavior; on implies must be enabled, off means disabled. This avoids the need to try to curate the discovered packages on your system if you have a desired configuration.

Not a huge deal, but may simplify things future usages.
Worth considering or now, should we just punt?

@wjwwood Made the local change for _FORCE for "dynamic loading" as a term.

I can rename RMW_IMPLEMENTATION_SUPPORTS_POCO as well, if it's scope is local to this package.

@EricCousineau-TRI
Copy link
Contributor Author

Do you prefer force pushes with curated commits [...]

Sorry! Just found my answer here: https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#pull-requests

dirk-thomas
dirk-thomas reviewed Mar 26, 2019
View reviewed changes
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
@EricCousineau-TRI
WIP Address comments
8302bd1 
Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>
@dirk-thomas
Copy link
Member

Multiple RMWs: Build Status
Single RMW: Build Status

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit c804be5 into ros2:master Mar 26, 2019
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 26, 2019
mm318 added a commit to aws-ros-dev/rmw_implementation that referenced this pull request Mar 29, 2019
@mm318
[IR] Implementation of QoS Features (#1)
88b36a6 
* add event APIs to wait_set

* Add rmw_take_event

* update rmw interface API

* add section about DCO to CONTRIBUTING.md

* cmake: Add `RMW_IMPLEMENTATION_FORCE_POCO` (ros2#59)

* cmake: Add `RMW_IMPLEMENTATION_FORCE_POCO`

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

* WIP Address comments

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

* WIP Address comments

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

* change rmw_event_t APIs from create()/destroy() to init()/fini() pattern

* Move rmw_*_event_init implementation to rmw
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@EricCousineau-TRI @wjwwood @dirk-thomas @tfoote