cconvey commented on code in PR #10891:
URL: https://github.com/apache/tvm/pull/10891#discussion_r841917926
##########
cmake/modules/HexagonSDK.cmake:
##########
@@ -97,11 +132,26 @@ function(_get_hexagon_sdk_property_impl
endif()
elseif(_property STREQUAL "QAIC_EXE")
- _check_path_exists(
- "${_hexagon_sdk_root}/ipc/fastrpc/qaic/Ubuntu18/qaic"
- _qaic_path
- )
- set_parent(${_output_variable} "${_qaic_path}")
+ set(_override $ENV{QAIC_PATH_OVERRIDE})
+ if(_override)
+ set_parent(_qaic_path "${_override}")
+ else()
+ _get_ubuntu_version(_uversion)
+ _check_path_exists(
+ "${_hexagon_sdk_root}/ipc/fastrpc/qaic/${_uversion}/qaic"
+ _qaic_path
+ )
+ endif()
+ _check_path_exists("${_qaic_path}" _qaic_path_found)
+ if(NOT _qaic_path_found)
+ message(
+ SEND_ERROR
Review Comment:
Is there a particular reason to use `SEND_ERROR` instead of `FATAL_ERROR`?
##########
cmake/modules/HexagonSDK.cmake:
##########
@@ -97,11 +132,26 @@ function(_get_hexagon_sdk_property_impl
endif()
elseif(_property STREQUAL "QAIC_EXE")
- _check_path_exists(
- "${_hexagon_sdk_root}/ipc/fastrpc/qaic/Ubuntu18/qaic"
- _qaic_path
- )
- set_parent(${_output_variable} "${_qaic_path}")
+ set(_override $ENV{QAIC_PATH_OVERRIDE})
Review Comment:
Would it be more idiomatic for `QAIC_PATH_OVERRIDE` to be a CMake variable,
rather than an environment variable?
##########
cmake/modules/HexagonSDK.cmake:
##########
@@ -97,11 +132,26 @@ function(_get_hexagon_sdk_property_impl
endif()
elseif(_property STREQUAL "QAIC_EXE")
- _check_path_exists(
- "${_hexagon_sdk_root}/ipc/fastrpc/qaic/Ubuntu18/qaic"
- _qaic_path
- )
- set_parent(${_output_variable} "${_qaic_path}")
+ set(_override $ENV{QAIC_PATH_OVERRIDE})
+ if(_override)
+ set_parent(_qaic_path "${_override}")
+ else()
+ _get_ubuntu_version(_uversion)
+ _check_path_exists(
+ "${_hexagon_sdk_root}/ipc/fastrpc/qaic/${_uversion}/qaic"
+ _qaic_path
+ )
+ endif()
+ _check_path_exists("${_qaic_path}" _qaic_path_found)
+ if(NOT _qaic_path_found)
+ message(
+ SEND_ERROR
+ "The qaic executable cannot be found in '${_qaic_path}'. You can set "
+ "the environment variable QAIC_PATH_OVERRIDE to override the automatic
"
+ "search."
Review Comment:
IIUC, for most/all other properties handled by
`_get_hexagon_sdk_property_impl`, the convention is to set the output variable
to `"${_property}-NOTFOUND"` if property value isn't valid. This is even true
for properties handled by `_check_all_paths_exist`.
Would it make sense to handle this property in the same way, rather than
calling `message(SEND_ERROR, ...)`?
##########
cmake/modules/HexagonSDK.cmake:
##########
@@ -97,11 +132,26 @@ function(_get_hexagon_sdk_property_impl
endif()
elseif(_property STREQUAL "QAIC_EXE")
- _check_path_exists(
- "${_hexagon_sdk_root}/ipc/fastrpc/qaic/Ubuntu18/qaic"
- _qaic_path
- )
- set_parent(${_output_variable} "${_qaic_path}")
+ set(_override $ENV{QAIC_PATH_OVERRIDE})
+ if(_override)
+ set_parent(_qaic_path "${_override}")
+ else()
+ _get_ubuntu_version(_uversion)
+ _check_path_exists(
+ "${_hexagon_sdk_root}/ipc/fastrpc/qaic/${_uversion}/qaic"
+ _qaic_path
+ )
+ endif()
+ _check_path_exists("${_qaic_path}" _qaic_path_found)
Review Comment:
If the `else()` branch (above) is taken, I think this makes _two_ calls to
`_check_path_exists`. Can this easily be restructured to eliminate the
redundancy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
