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

bachittle · 2022-12-22T22:06:53Z

Changes

Trying to compile with CMake on Windows. It breaks because of the Required clause. Removing it gets it working and installs required vcpkg files.

some improvements to consider:

this vcpkg run is very slow
googletest is already added in third_party (default command the docs tell you to run is --recursive, which gets googletest)

CHANGELOG.md updated for non-trivial changes (change is trivial)
Unit tests have been added (comment if this is needed)
Changes in public API reviewed (no changes to public API)

linux-foundation-easycla · 2022-12-22T22:06:56Z

The committers listed above are authorized under a signed CLA.

✅ login: bachittle / name: Bailey Chittle (0d6e552, 492285c, d9e8473, 070a675, e3d8a79, 22caf23)
✅ login: lalitb / name: Lalit Kumar Bhasin (877e14c, 62db735)
✅ login: ThomsonTan / name: Tom Tan (1124c66)

codecov · 2022-12-22T22:28:55Z

Codecov Report

Merging #1887 (7349c5b) into main (5e290c9) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1887      +/-   ##
==========================================
+ Coverage   87.15%   87.17%   +0.03%     
==========================================
  Files         166      166              
  Lines        4777     4777              
==========================================
+ Hits         4163     4164       +1     
+ Misses        614      613       -1

see 1 file with indirect coverage changes

bachittle · 2022-12-23T16:22:47Z

well it seems that even with this change the code still doesn't compile, it can't seem to find the vcpkg libraries afterwards. I might just experiment with including GTest manually like how it is done for linux (just including .so files instead of .a files)

ThomsonTan · 2023-01-03T19:40:06Z

well it seems that even with this change the code still doesn't compile, it can't seem to find the vcpkg libraries afterwards. I might just experiment with including GTest manually like how it is done for linux (just including .so files instead of .a files)

Wondering whether the vcpkg packages were installed correctly to <repo_root>\tools\vcpkg? The vcpkg is added to current CMake search path as below.

https://github.com/open-telemetry/opentelemetry-cpp/blob/main/CMakeLists.txt#L217

…metry-cpp into cmake_win_fix

…kely due to legacy gmock)

bachittle · 2023-01-03T21:47:29Z

@ThomsonTan the vcpkg root was the issue, it would find it on first install but not on consecutive runs. Now I got my code to compile, but the test cases fail. Most likely due to GMOCK_LIB dependency. I couldn't find a vcpkg dependency of something that is similar to libgmock-dev that allowed me to get it to work on linux...

test_common/src/http/client/nosend/CMakeLists.txt

lalitb · 2023-01-04T14:49:26Z

@ThomsonTan the vcpkg root was the issue, it would find it on first install but not on consecutive runs. Now I got my code to compile, but the test cases fail. Most likely due to GMOCK_LIB dependency. I couldn't find a vcpkg dependency of something that is similar to libgmock-dev that allowed me to get it to work on linux...

What is the failure error in tests ? gmock should be installed as part of gtest installation using vcpkg.

…e start?

…metry-cpp into cmake_win_fix

bachittle · 2023-01-04T15:23:41Z

It works now. Only one failed test, which is also what showed up in my linux tests. Setting the toolchain file so late in the build process seems to not have an effect on finding proper packages, so I just set it manually under cmake-gui. Not ideal, but if there are any better solutions let me know!

lalitb · 2023-01-30T21:16:23Z

@bachittle - Can you fix the CLA, so that it can be reviewed and merged?

bachittle · 2023-01-31T14:35:21Z

It says that EasyCLA check passed, but now there are other CICD issues...

owent · 2023-02-01T02:05:16Z

CMakeLists.txt

+  # 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! 


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()

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.

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()

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.

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

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.

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)

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

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.

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})

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

tools/setup-buildtools.cmd

bachittle · 2023-02-06T18:57:07Z

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

ThomsonTan · 2023-02-06T19:00:21Z

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

Here is the diff. Have you run cmake-format on code? If so, the version of cmake-format could make a difference.

https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/4106845839/jobs/7085592898

marcalff · 2023-05-23T15:17:03Z

@bachittle

Please fix the remaining CI failure about cmake-format.

This can be done by:

(1) installing and running cmake-format,
(2) or, copy and apply the patch given by the format failure in the build logs,
(3) or even manually change the code just by reading the diff from the logs, which is what I personally do most of the times.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c3c8b1e..b7cfe62 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -247,8 +247,8 @@ function(install_windows_deps)
   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
-  CACHE FILEPATH "")
+      ${CMAKE_CURRENT_SOURCE_DIR}/tools/vcpkg/scripts/buildsystems/vcpkg.cmake
+      CACHE FILEPATH "")
   message(
     "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."

In this case, there are just two lines to adjust.

marcalff

Almost there, cmake-format needs 2 more spaces.

CMakeLists.txt

cmake breaks here because of the REQUIRED clause

0d6e552

bachittle requested a review from a team December 22, 2022 22:06

marcalff added the pr:waiting-on-cla Waiting on CLA label Jan 2, 2023

bachittle and others added 2 commits January 3, 2023 10:55

Merge branch 'main' into cmake_win_fix

492285c

Merge branch 'main' into cmake_win_fix

877e14c

bachittle and others added 3 commits January 3, 2023 15:51

Merge branch 'cmake_win_fix' of https://github.com/bachittle/opentele…

d9e8473

…metry-cpp into cmake_win_fix

playing around with vcpkg, got it working but all tests fail (most li…

070a675

…kely due to legacy gmock)

Merge branch 'main' into cmake_win_fix

1124c66

owent reviewed Jan 4, 2023

View reviewed changes

test_common/src/http/client/nosend/CMakeLists.txt Outdated Show resolved Hide resolved

bachittle added 2 commits January 4, 2023 10:21

cmake toolchain files can't be set inside cmakelists.txt, maybe at th…

e3d8a79

…e start?

Merge branch 'cmake_win_fix' of https://github.com/bachittle/opentele…

22caf23

…metry-cpp into cmake_win_fix

Merge branch 'main' into cmake_win_fix

62db735

Merge branch 'main' into cmake_win_fix

9cfdac7

owent reviewed Feb 1, 2023

View reviewed changes

ThomsonTan reviewed Feb 4, 2023

View reviewed changes

tools/setup-buildtools.cmd Outdated Show resolved Hide resolved

Merge branch 'main' into cmake_win_fix

2839764

lalitb approved these changes Feb 4, 2023

View reviewed changes

better inclusion of VCPKG_ROOT

cbbd449

marcalff removed the pr:waiting-on-cla Waiting on CLA label Feb 6, 2023

bachittle and others added 2 commits February 6, 2023 11:09

fixed cmake formatting

f7de82c

Merge branch 'main' into cmake_win_fix

531f726

bachittle and others added 3 commits February 6, 2023 14:56

actually fix cmake format (ran the command)

8e62fe0

Merge branch 'main' into cmake_win_fix

f931327

Merge branch 'main' into cmake_win_fix

57a0975

owent approved these changes Feb 11, 2023

View reviewed changes

ThomsonTan and others added 5 commits February 10, 2023 19:51

Merge branch 'main' into cmake_win_fix

7b954d0

Merge branch 'main' into cmake_win_fix

a2f4462

Merge branch 'main' into cmake_win_fix

0a17a07

Merge branch 'main' into cmake_win_fix

596c2b8

trying to include CMAKE_TOOLCHAIN_FILE, still fails...

2bd872b

esigo assigned ThomsonTan Mar 27, 2023

marcalff reviewed May 23, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

bachittle force-pushed the cmake_win_fix branch from a0bdba0 to cb439ce Compare May 23, 2023 19:00

bachittle and others added 2 commits May 23, 2023 15:01

fix cmake format

cb439ce

Merge branch 'main' into cmake_win_fix

f6375fb

ThomsonTan approved these changes May 23, 2023

View reviewed changes

marcalff changed the title ~~fixing CMake to build GTest on Windows~~ [BUILD] Fixing CMake to build GTest on Windows May 26, 2023

Merge branch 'main' into cmake_win_fix

7349c5b

marcalff merged commit 3707bc0 into open-telemetry:main May 26, 2023

chenrui333 mentioned this pull request Jul 14, 2023

opentelemetry-cpp 1.10.0 Homebrew/homebrew-core#136563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bachittle commented Dec 22, 2022

linux-foundation-easycla bot commented Dec 22, 2022 •

edited

Loading

codecov bot commented Dec 22, 2022 •

edited

Loading

bachittle commented Dec 23, 2022

ThomsonTan commented Jan 3, 2023

bachittle commented Jan 3, 2023

lalitb commented Jan 4, 2023

bachittle commented Jan 4, 2023

lalitb commented Jan 30, 2023

bachittle commented Jan 31, 2023

owent Feb 1, 2023

bachittle Feb 1, 2023

owent Feb 3, 2023 •

edited

Loading

bachittle Feb 3, 2023

owent Feb 4, 2023 •

edited

Loading

ThomsonTan Feb 27, 2023

bachittle Feb 27, 2023 •

edited

Loading

bachittle Feb 27, 2023

ThomsonTan Mar 10, 2023

bachittle Mar 10, 2023

bachittle commented Feb 6, 2023

ThomsonTan commented Feb 6, 2023

marcalff commented May 23, 2023

marcalff left a comment

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

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

Conversation

bachittle commented Dec 22, 2022

Changes

linux-foundation-easycla bot commented Dec 22, 2022 • edited Loading

codecov bot commented Dec 22, 2022 • edited Loading

Codecov Report

bachittle commented Dec 23, 2022

ThomsonTan commented Jan 3, 2023

bachittle commented Jan 3, 2023

lalitb commented Jan 4, 2023

bachittle commented Jan 4, 2023

lalitb commented Jan 30, 2023

bachittle commented Jan 31, 2023

owent Feb 1, 2023

Choose a reason for hiding this comment

bachittle Feb 1, 2023

Choose a reason for hiding this comment

owent Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

bachittle Feb 3, 2023

Choose a reason for hiding this comment

owent Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

ThomsonTan Feb 27, 2023

Choose a reason for hiding this comment

bachittle Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

bachittle Feb 27, 2023

Choose a reason for hiding this comment

ThomsonTan Mar 10, 2023

Choose a reason for hiding this comment

bachittle Mar 10, 2023

Choose a reason for hiding this comment

bachittle commented Feb 6, 2023

ThomsonTan commented Feb 6, 2023

marcalff commented May 23, 2023

marcalff left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Dec 22, 2022 •

edited

Loading

codecov bot commented Dec 22, 2022 •

edited

Loading

owent Feb 3, 2023 •

edited

Loading

owent Feb 4, 2023 •

edited

Loading

bachittle Feb 27, 2023 •

edited

Loading