Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUILD] Fixing CMake to build GTest on Windows #1887

Merged
merged 25 commits into from
May 26, 2023
Merged

[BUILD] Fixing CMake to build GTest on Windows #1887

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0d6e552
cmake breaks here because of the REQUIRED clause
bachittle Dec 22, 2022
492285c
Merge branch 'main' into cmake_win_fix
bachittle Jan 3, 2023
877e14c
Merge branch 'main' into cmake_win_fix
lalitb Jan 3, 2023
d9e8473
Merge branch 'cmake_win_fix' of https://github.com/bachittle/opentele…
bachittle Jan 3, 2023
070a675
playing around with vcpkg, got it working but all tests fail (most li…
bachittle Jan 3, 2023
1124c66
Merge branch 'main' into cmake_win_fix
ThomsonTan Jan 3, 2023
e3d8a79
cmake toolchain files can't be set inside cmakelists.txt, maybe at th…
bachittle Jan 4, 2023
22caf23
Merge branch 'cmake_win_fix' of https://github.com/bachittle/opentele…
bachittle Jan 4, 2023
62db735
Merge branch 'main' into cmake_win_fix
lalitb Jan 30, 2023
9cfdac7
Merge branch 'main' into cmake_win_fix
lalitb Jan 31, 2023
2839764
Merge branch 'main' into cmake_win_fix
ThomsonTan Feb 4, 2023
cbbd449
better inclusion of VCPKG_ROOT
bachittle Feb 6, 2023
f7de82c
fixed cmake formatting
bachittle Feb 6, 2023
531f726
Merge branch 'main' into cmake_win_fix
ThomsonTan Feb 6, 2023
8e62fe0
actually fix cmake format (ran the command)
bachittle Feb 6, 2023
f931327
Merge branch 'main' into cmake_win_fix
ThomsonTan Feb 9, 2023
57a0975
Merge branch 'main' into cmake_win_fix
ThomsonTan Feb 10, 2023
7b954d0
Merge branch 'main' into cmake_win_fix
ThomsonTan Feb 11, 2023
a2f4462
Merge branch 'main' into cmake_win_fix
ThomsonTan Feb 18, 2023
0a17a07
Merge branch 'main' into cmake_win_fix
bachittle Feb 27, 2023
596c2b8
Merge branch 'main' into cmake_win_fix
ThomsonTan Mar 9, 2023
2bd872b
trying to include CMAKE_TOOLCHAIN_FILE, still fails...
bachittle Mar 10, 2023
cb439ce
fix cmake format
bachittle May 23, 2023
f6375fb
Merge branch 'main' into cmake_win_fix
ThomsonTan May 23, 2023
7349c5b
Merge branch 'main' into cmake_win_fix
marcalff May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,12 @@ function(install_windows_deps)
set(ENV{ARCH} ${ARCH})
execute_process(
COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/tools/setup-buildtools.cmd)
set(CMAKE_TOOLCHAIN_FILE
${CMAKE_CURRENT_SOURCE_DIR}/tools/vcpkg/scripts/buildsystems/vcpkg.cmake
PARENT_SCOPE)
# set(CMAKE_TOOLCHAIN_FILE
# ${CMAKE_CURRENT_SOURCE_DIR}/tools/vcpkg/scripts/buildsystems/vcpkg.cmake
# PARENT_SCOPE)
message("Make sure that vcpkg.cmake is set as the CMAKE_TOOLCHAIN_FILE at the START of the cmake build process!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we print this message only when we do not set toolchain file?

if(NOT CMAKE_TOOLCHAIN_FILE)
  message(WARNING "Make sure that vcpkg.cmake is set as the CMAKE_TOOLCHAIN_FILE at the START of the cmake build process!
Can be command-line arg (cmake -DCMAKE_TOOLCHAIN_FILE=...) or set in your editor of choice.")
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is the cmakefile tries to set the CMAKE_TOOLCHAIN_FILE in the cache at the start. This does not work unless its at the very start of the build process.

image

So its set, but it wasn't detecting my vcpkg unless I set it manually using cmake -DCMAKE_TOOLCHAIN_FILE=...
It may have to do with the submodules thinking the source directory is different, but gtest was not getting detected even after running install_windows_deps()

Copy link
Member

@owent owent Feb 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use cmake -DCMAKE_TOOLCHAIN_FILE=... to set toolchain, we should also replace NOT DEFINED CMAKE_TOOLCHAIN_FILE by (NOT DEFINED CMAKE_TOOLCHAIN_FILE AND NOT DEFINED CACHE{CMAKE_TOOLCHAIN_FILE}), or maybe NOT CMAKE_TOOLCHAIN_FILE for short.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can also just be removed entirely since it doesn't work

Copy link
Member

@owent owent Feb 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my mistake, DEFINED <VAR NAME> can also be used to check cached variables.
LGTM after format problem is solved.Thanks for your contribution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm the ci format is still failing, any reason as to why?

Hi @bachittle, could you please also take a look at my comment here?

#1887 (comment)

Copy link
Contributor Author

@bachittle bachittle Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I didn't see this, will take a look. The ci format isn't failing anymore, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
these are my cmake flags that are set using cmake-gui. notice the WITH_OTLP is not set, so I am just building the tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bachittle. This does confirm that OTLP is not enabled. Instead of commenting out the set of CMAKE_TOOLCHAIN_FILE, can we add a line like below in the call after find_package(GTest REQUIRED) in the same file?

opentelemetry-cpp/CMakeLists.txt

Line 331 in 5046443

include(${CMAKE_TOOLCHAIN_FILE})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this and it still failed, check latest commit: 2bd872b. Let me know if I should try putting this somewhere else.

Can be command-line arg (cmake -DCMAKE_TOOLCHAIN_FILE=...) or set in your editor of choice.")

endfunction()

if(WITH_JAEGER)
Expand Down Expand Up @@ -500,7 +503,7 @@ if(BUILD_TESTING)
${CMAKE_BINARY_DIR}/lib/libgmock.a)
elseif(WIN32)
# Make sure we are always bootsrapped with vcpkg on Windows
find_package(GTest REQUIRED)
find_package(GTest)
if(NOT (GTEST_FOUND OR GTest_FOUND))
install_windows_deps()
find_package(GTest REQUIRED)
Expand Down
2 changes: 1 addition & 1 deletion test_common/src/http/client/nosend/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ if(${BUILD_TESTING})
endif()

target_link_libraries(
opentelemetry_http_client_nosend ${GTEST_BOTH_LIBRARIES} ${GMOCK_LIB}
opentelemetry_http_client_nosend ${GTEST_BOTH_LIBRARIES}
owent marked this conversation as resolved.
Show resolved Hide resolved
opentelemetry_ext opentelemetry_test_common)

endif()
1 change: 1 addition & 0 deletions tools/setup-buildtools.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ if %ERRORLEVEL% == 1 (
popd
) else (
echo Using existing vcpkg installation...
set "VCPKG_ROOT=%~dp0\vcpkg"
ThomsonTan marked this conversation as resolved.
Show resolved Hide resolved
)

REM Install dependencies
Expand Down