Add options to control build_shared_libs and gtest_force_shared_crt for googletest #2388
Conversation
|
CI VulkanTools build queued with queue ID 445153. |
|
CI VulkanTools build # 4485 running. |
|
CI VulkanTools build # 4485 passed. |
charles-lunarg
left a comment
charles-lunarg
left a comment
There was a problem hiding this comment.
Only question is whether the MSVC static CRT should be set per-target or globally - I would say per-target and only for vkconfig, as the layers in the project should ideally use the dynamic CRT since they are 'dlls' that are loaded into another process, while vkconfig is an application on its own.
| if self.name == "googletest" and (self._args.gtest_shared_libs or self._args.gtest_force_shared_crt): | ||
| for index,option in enumerate(self.cmake_options): | ||
| if self._args.gtest_shared_libs and 'build_shared_libs' in option.lower(): | ||
| self.cmake_options[index] = f"-DBUILD_SHARED_LIBS={str(self._args.gtest_shared_libs)}" | ||
| print(f'INFO: Setting googletest option: -DBUILD_SHARED_LIBS={str(self._args.gtest_shared_libs)}') | ||
| if self._args.gtest_force_shared_crt and 'gtest_force_shared_crt' in option.lower(): | ||
| self.cmake_options[index] = f"-Dgtest_force_shared_crt={str(self._args.gtest_force_shared_crt)}" | ||
| print(f'INFO: Setting googletest option: -Dgtest_force_shared_crt={str(self._args.gtest_force_shared_crt)}') |
There was a problem hiding this comment.
Those options are actually broken in googletest currently, doing nothing to change the CRT used.
google/googletest#4731 is an open issue documenting it that and I added my findings about it.
The actual review comments I have is that the code looks reasonable, and there isn't any reason to reject the change just because googletest doesn't currently make use of it.
There was a problem hiding this comment.
After a lot of troubleshooting, it looked liked I needed to turn both these options to off. Otherwise I ended up with linker errors trying to build a static version. Check out https://github.com/google/googletest/blob/571930618fa96eabcd05b573285edbee9fc13bae/googletest/cmake/internal_utils.cmake#L30
Also, originally I planned to just have an option to turn the googletest options both "OFF" (johnzupin@051232c), but I thought maybe it was better to give more control to the user. I don't know if that is a better approach if these options are broken.
There was a problem hiding this comment.
I think the options we want is:
BUILD_SHARED_LIBS=OFF
gtest_force_shared_crt=ON
We can delete "-Dgtest_force_shared_crt=ON", from known_good.json to get that, as OFF is the default value for googletest 1.14. BUILD_SHARED_LIBS is already set to off and can be left alone.
There was a problem hiding this comment.
It has been difficult removing gtest_force_shared_crt from known-good.json, which otherwise forces it to "off." That seems to break running ctest on a non-static build of Qt, which also breaks github actions. I've decided to leave those options for update_deps.sh in and let users control it.
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
0cbcef0 to
8a5acff
Compare
|
CI VulkanTools build queued with queue ID 448816. |
|
CI VulkanTools build # 4517 running. |
|
CI VulkanTools build # 4517 failed. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
8a5acff to
6584c70
Compare
|
CI VulkanTools build queued with queue ID 448942. |
|
CI VulkanTools build # 4520 running. |
|
CI VulkanTools build # 4520 failed. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
6584c70 to
abc4f09
Compare
|
CI VulkanTools build queued with queue ID 448965. |
|
CI VulkanTools build # 4521 running. |
|
CI VulkanTools build # 4521 failed. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
abc4f09 to
6f73cdc
Compare
|
CI VulkanTools build queued with queue ID 448994. |
|
CI VulkanTools build # 4522 running. |
|
CI VulkanTools build # 4522 failed. |
|
CI VulkanTools build queued with queue ID 450968. |
|
CI VulkanTools build # 4546 running. |
|
CI VulkanTools build # 4546 failed. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
93a8624 to
6f73cdc
Compare
|
CI VulkanTools build # 4522 failed. |
|
CI VulkanTools build queued with queue ID 451012. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
0025138 to
97e5d28
Compare
|
CI VulkanTools build queued with queue ID 451023. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
97e5d28 to
5113f0f
Compare
|
CI VulkanTools build queued with queue ID 452690. |
|
CI VulkanTools build # 4573 running. |
|
CI VulkanTools build # 4573 failed. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
2094744 to
b8a1d46
Compare
|
CI VulkanTools build queued with queue ID 452704. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
b8a1d46 to
94c742a
Compare
|
CI VulkanTools build queued with queue ID 452715. |
|
CI VulkanTools build # 4575 running. |
|
CI VulkanTools build # 4575 failed. |
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
94c742a to
0fa88a2
Compare
|
CI VulkanTools build queued with queue ID 452929. |
|
CI VulkanTools build # 4576 running. |
|
CI VulkanTools build # 4576 failed. |
.github/workflows/ci.yml - Add --timeout because of hang when testin static builds of vkconfig CMakeLists.txt: - Move find_package calls so scripts/CMakeLists.txt can use QT_TARGET_TYPE var - Get the property type of Qt build - Remove USE_STATIC_RT_VKCONFIG option - Set property on MSVC_RUNTIME_LIBRARY to MT for vkconfg when qt is static layersvt/test/CMakeLists.txt vkconfig_core/test/CMakeLists.txt - Remove use of USE_STATIC_RT_VKCONFIG - Set property on MSVC_RUNTIME_LIBRARY to MT for vkconfg when qt is static vkconfig_cmd/CMakeLists.txt - Remove use of USE_STATIC_RT_VKCONFIG vkconfig_core/CMakeLists.txt - Remove use of USE_STATIC_RT_VKCONFIG vkconfig_core/test/CMakeLists.txt - Remove use of USE_STATIC_RT_VKCONFIG - Cleanup and formatting
johnzupin
force-pushed
the
add-googletest-options-disable-shared-libs-and-shared-crt
branch
from
0fa88a2 to
eed1118
Compare
|
CI VulkanTools build queued with queue ID 453010. |
|
CI VulkanTools build # 4577 running. |
|
CI VulkanTools build # 4577 failed. |
|
@christophe-lunarg @charles-lunarg When you guys get a chance please re-review. |
christophe-lunarg
left a comment
christophe-lunarg
left a comment
There was a problem hiding this comment.
Looks good to me !
christophe-lunarg
added
P0 - Must Fix
4 participants
Add options to control build_shared_libs and gtest_force_shared_crt for googletest. Update layersvt tests for use of USE_STATIC_RT_VKCONFIG to enable static builds of tests.
scripts/update_deps.py:
---gtest-shared-libsand--gtest-force-shared-crtthat allows a user to specify values for googletest's cmake build
options "BUILD_SHARED_LIBS" and/or "gtest_force_shared_crt"
These values override what is in the known-good.json.
layersvt/test/CMakeLists.txt:
CMAKE_MSVC_RUNTIME_LIBRARY
vkconfig_core/test/CMakeLists.txt: