Skip to content

prevent setting CONNEXTDDS_ARCH when a more proper one exists #137

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

Open
WanruXX 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 1 commit
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
32 changes: 18 additions & 14 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 @@ -476,6 +476,7 @@ function(rti_guess_connextdds_arch)
message(STATUS
"Pick first from ${CONNEXTDDS_DIR}/lib/[${architectures_installed}]")
Comment thread
WanruXX marked this conversation as resolved.
Outdated

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})
Comment thread
fujitatomoya marked this conversation as resolved.
endif()
else()
message(STATUS "ignored foreign architecture: ${architecture_name}")
Expand All @@ -529,18 +526,25 @@ 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()
message(STATUS "Selected CONNEXTDDS_ARCH: ${CONNEXTDDS_ARCH}")
endif()
if(NOT CONNEXTDDS_ARCH)
list(GET architecture_candidates 0 CONNEXTDDS_ARCH)

@iuhilnehc-ynos iuhilnehc-ynos Nov 29, 2023

Copy link
Copy Markdown
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?

@WanruXX WanruXX Nov 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

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.

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

Copy link
Copy Markdown
Collaborator

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

@WanruXX WanruXX Nov 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

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()
endif()
Comment thread
WanruXX marked this conversation as resolved.

endif()

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