use Connext 5.3.1 and Visual Studio 2017

[dirk-thomas]

For Windows I didn't bother to install the VS 2015 version but go for the VS 2017 version right away.

[dirk-thomas]

@dhood Since you wrote the rti_web_binaries_install_script.py script to install Connext I was wondering if you could patch it to work for the new Connext version. Currently it fails on a new question: see https://ci.ros2.org/job/ci_linux/4200/console

[dhood]

sure thing

[dirk-thomas]

The commit 892373e adds the bits and pieces from #122 to use VS 2017.

[mikaelarguedas]

Lots of bug fixes in there 😋
https://community.rti.com/static/documentation/connext-dds/5.3.1/doc/manuals/connext_dds/RTI_ConnextDDS_CoreLibraries_ReleaseNotes.pdf

[dirk-thomas]

I have installed Connext 5.3.1 on all running mac OS (mini1+2+3) and Windows (icecube+portable+windshield) machines:

• macOS:
• Windows:

[dhood]

@dirk-thomas it installs fine on linux after 479d6fb (and a necessary rebase):

I'll pass this back over to you for QA.

Launched the osx and windows jobs including fastrtps looking for cross-rmw issues (I also haven't seen the osx failure above before so it may be a regression):

• os x:
• windows:
• linux: above includes fastrtps.

[dirk-thomas]

I assume the index should never be not 0, 1, or 2? For safety I would propose to still add (to protect from a potentially infinite loop):

else:
    assert False, 'Unexpected result_index: %d' % result_index

[dhood]

it'll return an index in the list or timeout (which is caught below), but there's no harm in adding it. How come not raise though? (not that we intend to run this optimised, but who knows.)

[dirk-thomas]

How come not raise though?

That is exactly what the assert does.

[dhood]

not if run with -O, right? Again it's "not that we intend to run this optimised, but who knows." 😄

[dhood]

=> what I meant was that today we don't, but for some unrelated reason in the future we might set PYTHONOPTIMIZE or something

[dirk-thomas]

Usually I use assert for logic checks like this. For cases which "shouldn't happen". But I don't mind either way here.

[dirk-thomas]

Added in 1523332.

[dirk-thomas]

The CI builds are green. Waiting for final review (preferably from multiple people since this not only bumps Connext but also Visual Studio).

[dhood]

the connext 5.3.1 changes LGTM, don't have familiarity with the visual studio changes.

[mikaelarguedas]

I didn't follow this closely.
How does it work for future Ardent releases?

• Is the goal to keep VS2015 installed on all machines and to build next Ardent patches with VS2015? If so can you please create an ardent branch here that we can use to that effect?
• Do we also have 5.3.1 for VS2015 ? Or do future Ardent release need 5.3.0 to still be installed?

[mikaelarguedas]

@dirk-thomas Just checking: Can you confirm that the following been installed on all Windows nodes?

• openCV compiled with VS2017
• connext 5.3.1 security plugins

[dirk-thomas]

Is the goal to keep VS2015 installed on all machines and to build next Ardent patches with VS2015?

Yes.

If so can you please create an ardent branch here that we can use to that effect?

I will do that once this is merged (to branch as late as possible).

Do we also have 5.3.1 for VS2015 ?

Yes, we do.

Or do future Ardent release need 5.3.0 to still be installed?

Until someone take the initiative nothing changes - so Ardent releases can still use 5.3.0. If we see a reason to change that we can do that on the to-be-created ardent branch (which will require 5.3.1 for VS2015 to be installed on the build nodes.

[dirk-thomas]

openCV compiled with VS2017

That has already happened for #122 a while ago.

connext 5.3.1 security plugins

Yes, of course.

[mikaelarguedas]

Thanks for confirming 👍

[mikaelarguedas]

lgtm

[dirk-thomas]

@mikaelarguedas @nuclearsandwich Thank you for reviewing this. Can you please also review the referenced PR ros2/rmw_connext#283.