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.

Sign up for GitHub

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

prevent setting CONNEXTDDS_ARCH when a more proper one exists #137

Open
wants to merge 5 commits into
base: rolling
Choose a base branch
from
Open

prevent setting CONNEXTDDS_ARCH when a more proper one exists #137

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c058b0d
prevent setting CONNEXTDDS_ARCH when a more proper one exists
Nov 21, 2023
fc6eb52
Update rti_connext_dds_cmake_module/cmake/rti_build_helper.cmake
WanruXX Nov 28, 2023
609f3ef
Update rti_connext_dds_cmake_module/cmake/rti_build_helper.cmake
WanruXX Nov 28, 2023
01d704f
update message when the guessed architecture exists
Nov 29, 2023
455302d
check if architecture_candidates is empty if no proper architecture i…
Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions rti_connext_dds_cmake_module/cmake/rti_build_helper.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Author

@WanruXX WanruXX Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i changed the message here since it will skip the message in line 546

else()
# If CONNEXTDDS_ARCH is unspecified, the module tries uses the first
# architecture installed by looking under $CONNEXTDDS_DIR/lib that matches
Expand All @@ -474,8 +474,9 @@ function(rti_guess_connextdds_arch)
message(STATUS
"Guessed CONNEXTDDS_ARCH ('${guessed_architecture}') not available.")
message(STATUS
"Pick first from ${CONNEXTDDS_DIR}/lib/[${architectures_installed}]")
"Try to search from ${CONNEXTDDS_DIR}/lib/[${architectures_installed}]")

set(architecture_candidates "")
foreach(architecture_name ${architectures_installed})
# Because the lib folder contains both target libraries and
# Java JAR files, here we exclude the "java" in our algorithm
Expand Down Expand Up @@ -516,11 +517,7 @@ function(rti_guess_connextdds_arch)
endif()
endif()
if(NOT CONNEXTDDS_ARCH)
message(STATUS
"unsupported CMAKE_HOST_SYSTEM_NAME (${CMAKE_HOST_SYSTEM_NAME}) "
"or CMAKE_HOST_SYSTEM_PROCESSOR (${CMAKE_HOST_SYSTEM_PROCESSOR}). "
"Using architecture ${architecture_name} anyway.")
set(CONNEXTDDS_ARCH "${architecture_name}")
list(APPEND architecture_candidates ${architecture_name})
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
endif()
else()
message(STATUS "ignored foreign architecture: ${architecture_name}")
Expand All @@ -529,18 +526,27 @@ function(rti_guess_connextdds_arch)
if(CONNEXTDDS_ARCH)
break()
endif()

endforeach()
endif()

if(NOT CONNEXTDDS_ARCH)
message(WARNING
"CONNEXTDDS_ARCH not specified. Please set "
"-DCONNEXTDDS_ARCH= to specify your RTI Connext DDS "
" architecture")
else()
if(NOT CONNEXTDDS_ARCH)
list(GET architecture_candidates 0 CONNEXTDDS_ARCH)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the architectures_installed is empty, is it safe to get the item from architecture_candidates?

Copy link
Author

@WanruXX WanruXX Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iuhilnehc-ynos iuhilnehc-ynos Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the following error message is expected.

CMake Error at CMakeLists.txt:(line number) (list):
  list GET given empty list

Copy link
Author

@WanruXX WanruXX Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for not examining carefully, i made the change for checking it 455302d#diff-3861ef3a2ee61ce1bc76d23a0207a4b3d235cbbf3b63b91b33e61d58e7527564R535

if(CONNEXTDDS_ARCH)
message(STATUS
"unsupported CMAKE_HOST_SYSTEM_NAME (${CMAKE_HOST_SYSTEM_NAME}) "
"or CMAKE_HOST_SYSTEM_PROCESSOR (${CMAKE_HOST_SYSTEM_PROCESSOR}). "
"Using architecture ${CONNEXTDDS_ARCH} anyway.")
else()
message(WARNING
"CONNEXTDDS_ARCH not specified. Please set "
"-DCONNEXTDDS_ARCH= to specify your RTI Connext DDS "
" architecture")
endif()
else()
message(STATUS "Selected CONNEXTDDS_ARCH: ${CONNEXTDDS_ARCH}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that this status message could not be output if the guessed_architecture is matched, but the original logic did.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, i modified the message in the case the guessed_architecture exists, please refer to below

endif()
endif()
WanruXX marked this conversation as resolved.
Show resolved Hide resolved

endif()

set(CONNEXTDDS_ARCH "${CONNEXTDDS_ARCH}" PARENT_SCOPE)
endfunction()
Expand Down