Skip to content

Commit

Permalink
Replace if(NOT ${var}) by if(NOT var) (#41924)
Browse files Browse the repository at this point in the history
Summary:
As explained in pytorch/pytorch#41922 using `if(NOT ${var})" is usually wrong and can lead to issues like pytorch/pytorch#41922 where the condition is wrongly evaluated to FALSE instead of TRUE. Instead the unevaluated variable name should be used in all cases, see the CMake docu for details.

This fixes the `NOT ${var}` cases by using a simple regexp replacement. It seems `pybind11_PREFER_third_party` is the only variable really prone to causing an issue as all others are set. However due to CMake evaluating unquoted strings in `if` conditions as variable names I recommend to never use unquoted `${var}` in an if condition. A similar regexp based replacement could be done on the whole codebase but as that does a lot of changes I didn't include this now. Also `if(${var})` will likely lead to a parser error if `var` is unset instead of a wrong result

Fixes pytorch/pytorch#41922

Pull Request resolved: pytorch/pytorch#41924

Reviewed By: seemethere

Differential Revision: D22700229

Pulled By: mrshenli

fbshipit-source-id: e2b3466039e4312887543c2e988270547a91c439
  • Loading branch information
Flamefire authored and facebook-github-bot committed Jul 23, 2020
1 parent dbe6bfb commit a4b831a
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 11 deletions.
10 changes: 5 additions & 5 deletions cmake/Dependencies.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ if(BUILD_TEST OR BUILD_MOBILE_BENCHMARK OR BUILD_MOBILE_TEST)
"-P"
"${CMAKE_CURRENT_LIST_DIR}/GoogleTestPatch.cmake"
RESULT_VARIABLE _exitcode)
if(NOT ${_exitcode} EQUAL 0)
if(NOT _exitcode EQUAL 0)
message(WARNING "Patching failed for Google Test. The build may fail.")
endif()
endif()
Expand Down Expand Up @@ -634,7 +634,7 @@ if(BUILD_TEST OR BUILD_MOBILE_BENCHMARK OR BUILD_MOBILE_TEST)
"-P"
"${CMAKE_CURRENT_LIST_DIR}/GoogleTestPatch.cmake"
RESULT_VARIABLE _exitcode)
if(NOT ${_exitcode} EQUAL 0)
if(NOT _exitcode EQUAL 0)
message(WARNING "Reverting changes failed for Google Test. The build may fail.")
endif()
endif()
Expand Down Expand Up @@ -877,7 +877,7 @@ if(BUILD_PYTHON)
execute_process(
COMMAND "${PYTHON_EXECUTABLE}" "--version"
RESULT_VARIABLE _exitcode OUTPUT_VARIABLE PYTHON_VERSION)
if(NOT ${_exitcode} EQUAL 0)
if(NOT _exitcode EQUAL 0)
message(FATAL_ERROR "The Python executable ${PYTHON_EXECUTABLE} cannot be run. Make sure that it is an absolute path.")
endif()
if(PYTHON_VERSION)
Expand Down Expand Up @@ -955,7 +955,7 @@ if(BUILD_PYTHON)
endif()

# ---[ pybind11
if(NOT ${pybind11_PREFER_third_party})
if(NOT pybind11_PREFER_third_party)
find_package(pybind11 CONFIG)
if(NOT pybind11_FOUND)
find_package(pybind11)
Expand Down Expand Up @@ -1226,7 +1226,7 @@ if(USE_NCCL)
"Not using CUDA/ROCM, so disabling USE_NCCL. Suppress this warning with "
"-DUSE_NCCL=OFF.")
caffe2_update_option(USE_NCCL OFF)
elseif(NOT ${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
elseif(NOT CMAKE_SYSTEM_NAME STREQUAL "Linux")
message(WARNING "NCCL is currently only supported under Linux.")
caffe2_update_option(USE_NCCL OFF)
elseif(USE_CUDA)
Expand Down
2 changes: 1 addition & 1 deletion cmake/Modules_CUDA_fix/upstream/FindCUDA.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1505,7 +1505,7 @@ macro(CUDA_WRAP_SRCS cuda_target format generated_files)
set(_cuda_source_format ${format})
endif()
# If file isn't a .cu file, we need to tell nvcc to treat it as such.
if(NOT ${file} MATCHES "\\.cu$")
if(NOT file MATCHES "\\.cu$")
set(cuda_language_flag -x=cu)
else()
set(cuda_language_flag)
Expand Down
2 changes: 1 addition & 1 deletion cmake/ProtoBuf.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ endmacro()
# coded BUILD_CUSTOM_PROTOBUF, we will hard code the use of custom protobuf
# in the submodule.
if(ANDROID OR IOS)
if(NOT ${BUILD_CUSTOM_PROTOBUF})
if(NOT BUILD_CUSTOM_PROTOBUF)
message(WARNING
"For Android and iOS cross compilation, I am automatically using "
"custom protobuf under third party. Note that this behavior may "
Expand Down
4 changes: 2 additions & 2 deletions cmake/Utils.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ function(dedent outvar text)
INPUT_FILE "${CMAKE_BINARY_DIR}/indented.txt"
RESULT_VARIABLE _dedent_exitcode
OUTPUT_VARIABLE _dedent_text)
if(NOT ${_dedent_exitcode} EQUAL 0)
if(NOT _dedent_exitcode EQUAL 0)
message(ERROR " Failed to remove indentation from: \n\"\"\"\n${text}\n\"\"\"
Python dedent failed with error code: ${_dedent_exitcode}")
message(FATAL_ERROR " Python dedent failed with error code: ${_dedent_exitcode}")
Expand Down Expand Up @@ -202,7 +202,7 @@ function(pycmd outvar cmd)
dedent(_dedent_cmd "${cmd}")
pycmd_no_exit(_output _exitcode "${_dedent_cmd}")

if(NOT ${_exitcode} EQUAL 0)
if(NOT _exitcode EQUAL 0)
message(ERROR " Failed when running python code: \"\"\"\n${_dedent_cmd}\n\"\"\"")
message(FATAL_ERROR " Python command failed with error code: ${_exitcode}")
endif()
Expand Down
2 changes: 1 addition & 1 deletion cmake/public/cuda.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ if(CUDA_FOUND)
message(FATAL_ERROR "Caffe2: Couldn't determine version from header: " ${output_var})
endif()
message(STATUS "Caffe2: Header version is: " ${cuda_version_from_header})
if(NOT ${cuda_version_from_header} STREQUAL ${CUDA_VERSION_STRING})
if(NOT cuda_version_from_header STREQUAL ${CUDA_VERSION_STRING})
# Force CUDA to be processed for again next time
# TODO: I'm not sure if this counts as an implementation detail of
# FindCUDA
Expand Down
2 changes: 1 addition & 1 deletion torch/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ if(MSVC)
if(BUILD_TEST)
list(APPEND TORCH_PYTHON_LINK_LIBRARIES onnx_library)
endif(BUILD_TEST)
if(NOT ${CMAKE_BUILD_TYPE} MATCHES "Release")
if(NOT CMAKE_BUILD_TYPE MATCHES "Release")
string(APPEND TORCH_PYTHON_LINK_FLAGS " /DEBUG:FULL")
endif()
elseif(APPLE)
Expand Down

0 comments on commit a4b831a

Please sign in to comment.