prevent setting CONNEXTDDS_ARCH when a more proper one exists #137
Conversation
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Wanru <48364194+WanruXX@users.noreply.github.com>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Wanru <48364194+WanruXX@users.noreply.github.com>
|
theoratically we shouldn't have that situation... but unfortunately i don't know how the platform team in our department installed our rti that we have multiple libraries installed in our development environment for 300 developers😅 |
|
@asorbini requesting another review. |
| "-DCONNEXTDDS_ARCH= to specify your RTI Connext DDS " | ||
| " architecture") | ||
| endif() | ||
| else() | ||
| message(STATUS "Selected CONNEXTDDS_ARCH: ${CONNEXTDDS_ARCH}") |
There was a problem hiding this comment.
I am afraid that this status message could not be output if the guessed_architecture is matched, but the original logic did.
There was a problem hiding this comment.
indeed, i modified the message in the case the guessed_architecture exists, please refer to below
| " architecture") | ||
| else() | ||
| if(NOT CONNEXTDDS_ARCH) | ||
| list(GET architecture_candidates 0 CONNEXTDDS_ARCH) |
There was a problem hiding this comment.
what if the architectures_installed is empty, is it safe to get the item from architecture_candidates?
There was a problem hiding this comment.
in this case the CONNEXTDDS_ARCH will be not set so will print out the message in line 540, which means the user didn't install any non-foreign architectures that would fit their operation system. The build will fail in this case and the original logic both.
There was a problem hiding this comment.
I don't think the following error message is expected.
CMake Error at CMakeLists.txt:(line number) (list):
list GET given empty list
There was a problem hiding this comment.
sorry for not examining carefully, i made the change for checking it 455302d#diff-3861ef3a2ee61ce1bc76d23a0207a4b3d235cbbf3b63b91b33e61d58e7527564R535
| @@ -462,7 +462,7 @@ function(rti_guess_connextdds_arch) | |||
| if(EXISTS "${CONNEXTDDS_DIR}/lib/${guessed_architecture}") | |||
| set(CONNEXTDDS_ARCH "${guessed_architecture}") | |||
| message(STATUS | |||
| "Guessed ${CONNEXTDDS_DIR}/lib/${guessed_architecture} exists") | |||
| "Guessed ${CONNEXTDDS_DIR}/lib/${guessed_architecture} exists and is selected") | |||
There was a problem hiding this comment.
i changed the message here since it will skip the message in line 546
The original line 519-523 may result in an improper architect is selected while a proper one can exist.
For example, if you have
CMAKE_HOST_SYSTEM_NAME=Linux
CMAKE_HOST_SYSTEM_PROCESSOR=x86_64
architectures_installed=[armv8Linux4.4gcc5.4.0;x64Linux4gcc7.3.0]
the original cmake will set CONNEXTDDS_ARCH as armv8Linux4.4gcc5.4.0 at line 523 and break the foreach loop in the first entry without looking at the latter possible architectures.
After the change, armv8Linux4.4gcc5.4.0 will be put into a architecture_candidates list and continue searching, if x64Linux4gcc7.3.0 can be found then it will select this one, if not it will select armv8Linux4.4gcc5.4.0 as the best choice.