use Connext 5.3.1 and Visual Studio 2017#146
Conversation
|
@dhood Since you wrote the |
|
sure thing |
|
I have installed Connext 5.3.1 on all running mac OS (mini1+2+3) and Windows (icecube+portable+windshield) machines:
|
|
@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):
|
| elif result_index == 1: | ||
| child.sendline('y') | ||
| elif result_index == 2: | ||
| child.sendline('n') |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
How come not raise though?
That is exactly what the assert does.
There was a problem hiding this comment.
not if run with -O, right? Again it's "not that we intend to run this optimised, but who knows." 😄
There was a problem hiding this comment.
=> what I meant was that today we don't, but for some unrelated reason in the future we might set PYTHONOPTIMIZE or something
There was a problem hiding this comment.
Usually I use assert for logic checks like this. For cases which "shouldn't happen". But I don't mind either way here.
|
The CI builds are green. Waiting for final review (preferably from multiple people since this not only bumps Connext but also Visual Studio). |
dhood
left a comment
There was a problem hiding this comment.
the connext 5.3.1 changes LGTM, don't have familiarity with the visual studio changes.
|
I didn't follow this closely.
|
|
@dirk-thomas Just checking: Can you confirm that the following been installed on all Windows nodes?
|
Yes.
I will do that once this is merged (to branch as late as possible).
Yes, we do.
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. |
That has already happened for #122 a while ago.
Yes, of course. |
|
Thanks for confirming 👍 |
|
@mikaelarguedas @nuclearsandwich Thank you for reviewing this. Can you please also review the referenced PR ros2/rmw_connext#283. |
For Windows I didn't bother to install the VS 2015 version but go for the VS 2017 version right away.