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

Place RTI OpenSSL on the (LD_LIBRARY_)PATH on Linux #263

Merged
merged 7 commits into from
May 4, 2018

Conversation

mikaelarguedas
Copy link
Member

This implements "option a" described in ros2/ros2#481 (comment) as option b was evaluated as not acceptable for users.

This also works on xenial as the env vars stay the same so it can be merged async from ros2/ci#148 (but there's not much incentive to merge it before)

Xenial job: Build Status
Bionic job: Build Status

connects to ros2/ros2#481

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Apr 18, 2018
@mikaelarguedas mikaelarguedas self-assigned this Apr 18, 2018
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 18, 2018
@mikaelarguedas mikaelarguedas mentioned this pull request Apr 18, 2018
31 tasks
dirk-thomas
dirk-thomas previously approved these changes Apr 18, 2018
@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented May 3, 2018

This is broken on windows, moving back to "in progress" for now

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (failure unrelated)

Bionic: Build Status (failure unrelated ros2/rclcpp#470)

@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 3, 2018
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 4, 2018
@mikaelarguedas mikaelarguedas dismissed dirk-thomas’s stale review May 4, 2018 14:49

Rebased and modify logic to pass the build on windows

@mikaelarguedas mikaelarguedas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 4, 2018
just pass unchanged PATH on other platforms

Modify PATH only if connext is being tested

modify path for all connext tests (not sure why it worked before)
@mikaelarguedas
Copy link
Member Author

I changed this to modify the PATH only when testing Connext on Linux to make sure we still exercise OpenSSL 1.1 when using Fast-RTPS.
This doesn't modify the PATH for Windows and MacOS but we could use RTI's OpenSSL libraries on these platforms by just removing this condition. As there is no need for now I didnt do it.
If we were to test RTI's OpenSSL on other platforms we would also need to modify this logic to modify the *LIBRARY_PATH on all platforms to include RTI's OpenSSL libraries.

CI with last changes:

  • Linux Build Status

  • Linux-aarch64 Build Status

  • macOS Build Status

  • Windows Build Status

  • Bionic Build Status

@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels May 4, 2018
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.

Reading the patch I was confused about TEST_PATH_WITH_RTI_BIN assuming it would contain the extra bin for all rmw. Further down I see that it is only populated in the case of Connext. Maybe renaming the variable to something like TEST_PATH would avoid that confusion.

Anyway the patch looks fine to me.

@mikaelarguedas
Copy link
Member Author

Reading the patch I was confused about TEST_PATH_WITH_RTI_BIN assuming it would contain the extra bin for all rmw. Further down I see that it is only populated in the case of Connext. Maybe renaming the variable to something like TEST_PATH would avoid that confusion.

Good point I didnt rename the variable after changing the logic: done in 6dbd610

@mikaelarguedas mikaelarguedas merged commit 19e297f into master May 4, 2018
@mikaelarguedas mikaelarguedas deleted the rti_openssl branch May 4, 2018 17:24
@mikaelarguedas mikaelarguedas removed the in review Waiting for review (Kanban column) label May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants