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

consider available rmw implementations at runtime #65

Closed
wants to merge 1 commit into from

Conversation

@dirk-thomas
Copy link
Member

dirk-thomas commented Sep 4, 2019

Alternative to #63.

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
@dirk-thomas dirk-thomas self-assigned this Sep 4, 2019
@dirk-thomas dirk-thomas referenced this pull request Sep 4, 2019
@rotu

This comment has been minimized.

Copy link
Contributor

rotu commented Sep 4, 2019

Any reason to keep the build-time dependency on connext/fastrtps/opensplice?

Also, don't forget to remove this now-obsolete check

elseif(NOT RMW_IMPLEMENTATIONS MATCHES ";")
set(RMW_IMPLEMENTATION_SUPPORTS_POCO FALSE)
endif()

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

dirk-thomas commented Sep 4, 2019

Any reason to keep the build-time dependency on connext/fastrtps/opensplice?

The comment about bloom still applies to them: #63 (comment)

Also, don't forget to remove this now-obsolete check

That check is not obsolete. If there is only a single RMW implementation available at build time of rmw_implementation then it shouldn't use poco and instead directly link against that RMW implementation (which makes choosing the implementation using an environment variable impossible).

@rotu

This comment has been minimized.

Copy link
Contributor

rotu commented Sep 4, 2019

  1. I don't understand the comment about bloom, so I'm gonna assume you've thought about this and that you're right.
  2. If there is 0 or 1 RMW implementations when the underlay is built that doesn't mean there will be a single implementation when rmw_implementation is actually used. It seems like maybe it would make sense to have an option like RMW_IMPLEMENTATION_NO_DYNAMIC_LOADING like the existing RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING option.
  3. This fix seems to work! I'm closing my PRs.
@rotu rotu referenced this pull request Sep 4, 2019
@dirk-thomas

This comment has been minimized.

Copy link
Member Author

dirk-thomas commented Sep 5, 2019

It seems like maybe it would make sense to have an option like RMW_IMPLEMENTATION_NO_DYNAMIC_LOADING like the existing RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING option.

That would be an option to force static dynamic loading even if more than one rmw impl is present. On the other hand if you really care about that optimization you would probably not have more than one rmw impl. in your workspace in the first place. Anyway please consider to create a PR for that enhancement if you think it is valuable.

@rotu

This comment has been minimized.

Copy link
Contributor

rotu commented Sep 5, 2019

Anyway please consider to create a PR for that enhancement if you think it is valuable.

I guess I could, though I still don’t get why the special case is even needed when there is no more than one package. At least one developer found this puzzling #58

It really just strikes me as a mistake that there are now two lists of packages that can affect the build: one calculated when rmw_implementation is built and one calculated when downstream consuming packages are built. I don’t see the necessity of both RMW_IMPLEMENTATIONS and available_rmw_implementations, and I can see it causing subtle build bugs down the line.

Skipping dynamic discovery of middlewares depending on packages present when the underlay is built is one such bug.

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

dirk-thomas commented Sep 5, 2019

CI builds (up to test_communication) with 2 rmw impls:

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

CI builds (up to test_communication) with 1 rmw impl:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@dirk-thomas

This comment has been minimized.

Copy link
Member Author

dirk-thomas commented Sep 5, 2019

It really just strikes me as a mistake that there are now two lists of packages that can affect the build: one calculated when rmw_implementation is built and one calculated when downstream consuming packages are built. I don’t see the necessity of both RMW_IMPLEMENTATIONS and available_rmw_implementations, and I can see it causing subtle build bugs down the line.

I agree that having two different kind of checks is awkward. I just don't see what the alternative should be if the requirement is that the current uses cases should continue to be supported:

  • with a single RMW impl. present at build time rmw_implementation should directly link against it (for efficiency and simplicity)
  • for your use case you want to be able to use an RMW implementation which wasn't available a build time of rmw_implementation and select it using the environment variable

Please feel free to propose an alternative approach.

@rotu

This comment has been minimized.

Copy link
Contributor

rotu commented Sep 5, 2019

Proposed alternate approach in #67

@dirk-thomas

This comment has been minimized.

Copy link
Member Author

dirk-thomas commented Nov 4, 2019

Closing in favor of the merged PR #67.

@dirk-thomas dirk-thomas closed this Nov 4, 2019
@dirk-thomas dirk-thomas deleted the dirk-thomas/dynamically-check-rmw-impls branch Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.