[CMake] Fix incorrect handling of get_target_property failure

[ctetreau]

Summary

add_unittest was checking that the result of get_target_property was not
"NOTFOUND", but despite what the documentation says, get_target_property
returns <the var>-NOTFOUND on failure.

[serge-sans-paille]

Can you share the setup that triggers the error, so that we can reproduce it locally to test the patch?

[ctetreau]

The following CMake invocation causes the issue for me:

cmake -G "Visual Studio 16 2019" -A x64 -Thost=x64 -DLLVM_TARGETS_TO_BUILD="all" -DLLVM_LIT_ARGS=-v -DLIBCLANG_BUILD_STATIC=ON -DLLVM_ENABLE_ASSERTIONS:BOOL=ON -DLLVM_ENABLE_PROJECTS='all' [llvm repo dir]/llvm

I'm using CMake 3.15.5. Even assuming that this is a behavior change in CMake (nothing in my research while debugging this issue indicates to me that this is the case), "NOTFOUND" and "<varname>-NOTFOUND" are both falsey, and I would assume we aren't naming test suites any of the magic CMake falsey values (0, OFF, NO, FALSE, N, IGNORE, NOTFOUND, the empty string, or ends in the suffix -NOTFOUND), so this should be fine.

[ctetreau]

It looks like after this patch CMake is dumping all the tests at the project root

[ctetreau]

Apparently some falsey values are used elsewhere and it is not safe to just check if (NOT ${test_suite_folder})

[ctetreau]

CMake now documents this behavior correctly: https://cmake.org/cmake/help/git-master/command/get_target_property.html

[serge-sans-paille]

llvm/cmake/modules/AddLLVM.cmake

1425

If I understand https://cmake.org/cmake/help/latest/command/if.html correctly,

if(test_suite_folder)

should work too. Can you test that?

[ctetreau]

address code review issues

[ctetreau]

llvm/cmake/modules/AddLLVM.cmake

1425

I had originally put if(${test_suite_folder}), but it looks like the CMake language treats the string in the if as a variable and dereferences it instead of just checking to see if it's truthy.

Your version works just fine.

[serge-sans-paille]

LGTM, under the assumption you validated it under the relevant setup.

[MaskRay]

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

llvm/cmake/modules/AddLLVM.cmake

function(add_unittest test_suite test_name)
   set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
   # libpthreads overrides some standard library symbols, so main
   # executable must be linked with it in order to provide consistent
   # API for all shared libaries loaded by this executable.
   target_link_libraries(${test_name} PRIVATE gtest_main gtest ${LLVM_PTHREAD_LIB})
 
   add_dependencies(${test_suite} ${test_name})
   get_target_property(test_suite_folder ${test_suite} FOLDER)
-  if (NOT ${test_suite_folder} STREQUAL "NOTFOUND")
+  if (${test_suite_folder})
     set_property(TARGET ${test_name} PROPERTY FOLDER "${test_suite_folder}")
   endif ()
 endfunction()
 
 # Use for test binaries that call llvm::getInputFileDirectory(). Use of this
 # is discouraged.
 function(add_unittest_with_input_files test_suite test_name)
   set(LLVM_UNITTEST_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})