[cmake] [unittests] removing scripts for Xcode project test targets #36971
Conversation
…r search paths add target include directories on AddSwiftUnittests.cmake module. currently generated Xcode project doesn't include googletest headers after https://reviews.llvm.org/D86616
|
@varungandhi-apple I am not sure hundred percent this is right method. but this solved my problem that not included gtest header on unittests of generated xcode project. |
|
Hum, interesting ... this could potentially fix the problem people were having https://forums.swift.org/t/gtest-gtest-h-not-found-in-typeref-cpp-while-compiling-the-compiler/44399 ? |
LucianoPAlmeida
requested review from
varungandhi-apple,
shahmishal and
compnerd
| @@ -10,6 +10,9 @@ function(add_swift_unittest test_dirname) | |||
| # function defined by AddLLVM.cmake. | |||
| add_unittest(SwiftUnitTests ${test_dirname} ${ARGN}) | |||
|
|
|||
| target_include_directories(${test_dirname} PUBLIC ${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include) | |||
| target_include_directories(${test_dirname} PUBLIC ${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include) | |||
|
|
|||
There was a problem hiding this comment.
This is incorrect: https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake#L1471
add_unittest already links against the library, which should have the public include directories specified, so that would already setup the include paths properly.
There was a problem hiding this comment.
@compnerd thank you.
now I know this should be include header paths too, some reason Xcode project of llvm-project include header but not swift Xcode project. i'm continuing to figure out the script.
ps. include_directories is working well too, but want to find more elegant way.
There was a problem hiding this comment.
I suspect this is only issue for generating Xcode project so now testing some conditional options.
There was a problem hiding this comment.
@compnerd now, i understand cmake build dependencies from this area. so removing some codes more.
| target_include_directories(${test_dirname} PUBLIC ${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include) | ||
| target_include_directories(${test_dirname} PUBLIC ${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include) |
There was a problem hiding this comment.
PUBLIC is definitely not the right thing here. The unit tests do not define the interfaces for gtest or gmock.
There was a problem hiding this comment.
@compnerd Obviously this script was unnecessary.
FYI, I ran into this issue with a fresh install of macOS 10.15.7 and Xcode 12.3 |
sadly, build with Xcode 12.2 not working too. |
gtest, gtest_main LINK_LIBRARIES dependencies changed by that removed scripts to absolute library file path. as a result losing necessary include path dirs.
There was a problem hiding this comment.
@compnerd remove target_include_directories and unnecessary scripts. @varungandhi-apple @shahmishal @LucianoPAlmeida @abdulajet
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test Linux platform |
|
@swift-ci please test macOS platform |
@rollmind Perhaps the PR title and description should be updated to reflect the new changes :) |
oozoofrog
changed the title
I see, how about that changed title and description. |
|
@compnerd Thanks |
|
@varungandhi-apple @shahmishal |
|
@swift-ci test and merge |
|
@varungandhi-apple Thnx! :) |
|
Can we cherry-pick this into the 5.5 branch so that people trying to build from that branch have a better time? |
Some scripts in the add_swift_unittest function modifies LINK_LIBRARIES for include gtest, gtest_main libs. as a result lose link information that receving from add_unittest.
This PR removes that script, so the test target no longer throws a "fatal error: 'gtest/gtest.h' file not found" from build test targets.
it had been affected by after https://reviews.llvm.org/D86616