Errors building -DWITH_JAEGER on Windows - Thrift missing

[ghost]

Describe your environment
Windows building with VS 2019 64 bit. Will also want to build Win32.

Steps to reproduce
Add the -DWITH_JAEGER flag to CMake on Windows to a build that was working without the flag set.

What is the expected behavior?
Clean build

Hopefully I am just missing a flag.

What is the actual behavior?

CMake complains that it can't find Thrift.

Set directory to c:\dev\AA3rdPartyExt\Build\opentelemetry-cpp\VS16\Win64
Invoking cmake to create the project files
Arguments = -A "x64" "c:\dev\AA3rdPartyExt\opentelemetry-cpp" "-DCMAKE_TOOLCHAIN_FILE=C:\dev\vcpkg\scripts\buildsystems\vcpkg.cmake" "-DCMAKE_PREFIX_PATH=c:\dev\AA3rdPartyExt\Build\Ninja\VS16\Win64;c:\dev\AA3rdPartyExt\Build\nlohmann_json\VS16\Win64" -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DWITH_JAEGER=ON -DWITH_PROMETHEUS=OFF -DBUILD_TESTING=OFF -DJSON_Install=ON
Executing 'cmake' with arguments '-A "x64" "c:\dev\AA3rdPartyExt\opentelemetry-cpp" "-DCMAKE_TOOLCHAIN_FILE=C:\dev\vcpkg\scripts\buildsystems\vcpkg.cmake" "-DCMAKE_PREFIX_PATH=c:\dev\AA3rdPartyExt\Build\Ninja\VS16\Win64;c:\dev\AA3rdPartyExt\Build\nlohmann_json\VS16\Win64" -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DWITH_JAEGER=ON -DWITH_PROMETHEUS=OFF -DBUILD_TESTING=OFF -DJSON_Install=ON' in working directory 'c:\dev\AA3rdPartyExt\Build\opentelemetry-cpp\VS16\Win64'
Exit code: 1
Command error: Using external nlohmann::json
Building with nostd types...
CMake Error at C:/dev/vcpkg/scripts/buildsystems/vcpkg.cmake:263 (_find_package):
asked CMake to find a package configuration file provided by "Thrift", but
By not providing "FindThrift.cmake" in CMAKE_MODULE_PATH this project has
CMake did not find one.
Could not find a package configuration file provided by "Thrift" with any
of the following names:
ThriftConfig.cmake
thrift-config.cmake
Add the installation prefix of "Thrift" to CMAKE_PREFIX_PATH or set
"Thrift_DIR" to a directory containing one of the above files. If "Thrift"
provides a separate development package or SDK, be sure it has been
installed.
Call Stack (most recent call first):
exporters/jaeger/CMakeLists.txt:3 (find_package)
Output: -- Building for architecture ARCH=x64
-- Could NOT find CURL (missing: CURL_LIBRARY CURL_INCLUDE_DIR)
-- Configuring incomplete, errors occurred!

Additional context
I tried installing Thrift from GitHub and using CMake, but it then complained that FLEX wasn't installed. All comments I found on this assume Linux. I quit digging at that point.

It would be preferable if the chained dependencies were automatically pulled in.

[lalitb]

Thanks for trying it out, and reporting the issue. cc @ThomsonTan

[ThomsonTan]

@Kurt-Rayner-Alpha could you please try install thrift via vcpkg as you are using it? Like c:\dev\vcpkg\vcpkg install thrift:x64-windows?

[maxgolov]

Unfortunately we do not have a snapshot of Thrift in submodules. While for nlohmann::json we could use our local submodule, we can't do that for Thrift. This script sets up the necessary dependencies. You may need to borrow the same / similar logic:

opentelemetry-cpp/tools/setup-buildtools.cmd

Line 58 in 0a3320e

vcpkg install gtest:%ARCH%-windows

set ARCH=x64
vcpkg install gtest:%ARCH%-windows
vcpkg install --overlay-ports=%~dp0ports benchmark:%ARCH%-windows
vcpkg install --overlay-ports=%~dp0ports protobuf:%ARCH%-windows
vcpkg install ms-gsl:%ARCH%-windows
vcpkg install nlohmann-json:%ARCH%-windows
vcpkg install abseil:%ARCH%-windows
vcpkg install gRPC:%ARCH%-windows
vcpkg install prometheus-cpp:%ARCH%-windows
vcpkg install curl:%ARCH%-windows
vcpkg install thrift:%ARCH%-windows

vcpkg , perhaps, the easiest way to handle CMake dependencies on Windows.

[maxgolov]

I think we need to cover this as follows.

There are two documents presently available that describe the build process options:

• Building with vcpkg - for this one we should modify the response file to add dependencies on all packages listed in my previous comment. While it lists some dependencies, I think thrift was added recently and is missing in that document.

• Building with vs2019 - command line build process includes a reference to tools\setup-buildtols.cmd , which uses our local snapshot of vcpkg to build vcpkg first, then install all deps via vcpkg.

Maybe we need to add a separate document for raw CMake ? That, again - is mostly a "disclaimer-level" document about what components should be somehow manually installed before building OpenTelemetry C++ SDK. My personal preference would be, of course, to endorse the usage of vcpkg for package deps. Since building things like gRPC , thrift, etc. is a daunting task - in most cases that task should be outsourced to vcpkg build recipe.

[ghost]

Thanks Max! CMake on a separate clone of the Thrift repository seemed to work, but got stuck on the Flex subcomponent it pulled in. The documentation for Flex only covers Unix/Linux environments. I will try using VCPkg more consistently. I’ve tried to work exclusively with CMake where possible. As with NuGet, we have found VCPkg to be a bit too aggressive in injecting itself into the environment for Visual Studio solutions and projects that don’t need it (or want it). We stay away from the “integrate” command line option… My understanding was that the CMake FetchContent_xxxx macros were supposed to address dependencies with Git Submodules being recursively retrieved, but I’ve run into issues with that as well (possibly incorrect configurations). This is really important for us because we use different versions of open source modules. Thanks again! Kurt “The good thing about standards is that there are so many to choose from.” ― Andrew S. Tanenbaum From: Max Golovanov ***@***.*** Sent: Thursday, June 24, 2021 5:55 PM To: open-telemetry/opentelemetry-cpp Cc: Kurt-Rayner-Alpha; Mention Subject: Re: [open-telemetry/opentelemetry-cpp] Errors building -DWITH_JAEGER on Windows - Thrift missing (#874) Unfortunately we do not have a snapshot of Thrift in submodules. While for nlohmann::json we could use our local submodule, we can't do that for Thrift. This script sets up the necessary dependencies. You may need to borrow the same / similar logic: https://github.com/open-telemetry/opentelemetry-cpp/blob/0a3320ecdf92090d9e9db4aaaa89af5a8ceec87e/tools/setup-buildtools.cmd#L58 set ARCH=x64 vcpkg install gtest:%ARCH%-windows vcpkg install --overlay-ports=%~dp0ports benchmark:%ARCH%-windows vcpkg install --overlay-ports=%~dp0ports protobuf:%ARCH%-windows vcpkg install ms-gsl:%ARCH%-windows vcpkg install nlohmann-json:%ARCH%-windows vcpkg install abseil:%ARCH%-windows vcpkg install gRPC:%ARCH%-windows vcpkg install prometheus-cpp:%ARCH%-windows vcpkg install curl:%ARCH%-windows vcpkg install thrift:%ARCH%-windows — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#874 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN3GCNT2G6EYVMDLO33CELTUOSSLANCNFSM47GPQZYQ> . <https://github.com/notifications/beacon/AAN3GCM24GQ6RI3MDVTXIJLTUOSSLA5CNFSM47GPQZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGO6EKJA.gif>

[maxgolov]

@Kurt-Rayner-Alpha

We stay away from the “integrate” command line option

I totally agree with you on that. That is my experience as well. We don't use vcpkg integrate in OpenTelemetry. Instead we rely on the following:

• local snapshot of vcpkg under the tools\vcpkg directory

• we do respect VCPKG_ROOT environment variable, so if you want to use your own vcpkg, say at C:\vcpkg\, you can do so by setting environment variable set VCPKG_ROOT=C:\vcpkg\ then triggering the build. It should work fine. This is handled in the top-level CMakeListst.txt here:

  opentelemetry-cpp/CMakeLists.txt

  Line 53 in 0a3320e

  if(DEFINED ENV{VCPKG_ROOT} AND NOT DEFINED CMAKE_TOOLCHAIN_FILE)

What I think we may be able to handle a bit better, is the implementation of the call to install_windows_deps in CMake. We verify for the presence of certain dependencies, and if they are not available - we call into install_windows_deps. I think we only do that for protobuf , gRPC and GTest. Not for thrift. I think we can add a few lines of code to perform the auto-installation of thrift the same way. This still relies on vcpkg (that we include internally), but would be a bit less of a hassle. Customers would no longer need to cover that part themselves. @ThomsonTan - we need to do the same as what we do here, but for thrift under WITH_JAEGER :

opentelemetry-cpp/CMakeLists.txt

Line 266 in 0a3320e

install_windows_deps()

[ThomsonTan]

@maxgolov do you mean the below line? Thrift should be covered by install_windows_deps already in vcpkg.

opentelemetry-cpp/tools/setup-buildtools.cmd

Line 67 in 0a3320e

vcpkg install thrift:%ARCH%-windows

[maxgolov]

@ThomsonTan - it's covered if our customers runs setup-buildtools.cmd first. So, technically, it is covered...

But if someone did not run it - we could have invoked the install_windows_deps to install it, the way we handle WITH_OTLP case - pretty much the same way we do it for gRPC and protobuf.

Pseudocode:

if (WITH_JAGER)
  find_package(thrift QUIET)
  if (not_found_thrift) and (WIN32)
     install_windows_deps() // <- install it from our local vcpkg installation
     find_package(thrift REQUIRED)
  endif()
endif()

I'm thinking just Windows now, but the same concept could have been applied to other platforms - forcing the installation of deps if not installed. Going even further with FetchContent_xxxx instead of vcpkg - is probably an overkill, given all deps, etc. plus FetchContent requires a fairly fresh version of CMake.

[ghost]

Hi Max, Those would be very helpful changes! Using vcpkg for Thrift was sufficient to get Jaeger into our build project. We already had vcpkg in place for gRPC on Windows, so CMake was quite happy once Thrift was installed. In the meantime, I’ve extended our common build scripts to honor vcpkg installs as well as Git clone/builds. Testing on Linux now. Thank you again for all of your help! Best, Kurt From: Max Golovanov ***@***.*** Sent: Friday, June 25, 2021 1:15 PM To: open-telemetry/opentelemetry-cpp Cc: Kurt-Rayner-Alpha; Mention Subject: Re: [open-telemetry/opentelemetry-cpp] Errors building -DWITH_JAEGER on Windows - Thrift missing (#874) @Kurt-Rayner-Alpha <https://github.com/Kurt-Rayner-Alpha> We stay away from the “integrate” command line option I totally agree with you on that. That is my experience as well. We don't use vcpkg integrate in OpenTelemetry. Instead we rely on the following: · local snapshot of vcpkg under the tools\vcpkg directory · we do respect VCPKG_ROOT environment variable, so if you want to use your own vcpkg, say at C:\vcpkg\, you can do so by setting environment variable set VCPKG_ROOT=C:\vcpkg\ then triggering the build. It should work fine. This is handled in the top-level CMakeListst.txt here: https://github.com/open-telemetry/opentelemetry-cpp/blob/0a3320ecdf92090d9e9db4aaaa89af5a8ceec87e/CMakeLists.txt#L53 What I think we may be able to handle a bit better, is the implementation of the call to install_windows_deps in CMake. We verify for the presence of certain dependencies, and if they are not available - we call into install_windows_deps. I think we only do that for protobuf , gRPC and GTest. Not for thrift. I think we can add a few lines of code to perform the auto-installation of thrift the same way. This still relies on vcpkg (that we include internally), but would be a bit less of a hassle. Customers would no longer need to cover that part themselves. @ThomsonTan <https://github.com/ThomsonTan> - we need to do the same as what we do here, but for thrift under WITH_JAEGER : https://github.com/open-telemetry/opentelemetry-cpp/blob/0a3320ecdf92090d9e9db4aaaa89af5a8ceec87e/CMakeLists.txt#L266 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#874 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN3GCP7QX6JDZTTWKJEP5DTUS2SNANCNFSM47GPQZYQ> . <https://github.com/notifications/beacon/AAN3GCMVG6G47JRU3NGMK43TUS2SNA5CNFSM47GPQZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGPDYGKA.gif>

[ThomsonTan]

@maxgolov we have this check and try to invoke install_windows_deps if the user is not passing in a separate vcpkg, which means the use is relying on our submodule vcpkg (see below link). If the user provides a standalone vcpkg, the user would be responsible to install all the dependencies via vcpkg install, including gRPC, thrift, etc. Perhaps we could provide some hint in the error output.

opentelemetry-cpp/CMakeLists.txt

Line 205 in 0a3320e

install_windows_deps()

[maxgolov]

@ThomsonTan - Yes. You are right. I think what happened in Kurt's case, he supplied CMAKE_TOOLCHAIN_FILE and that is why we did not run the install_windows_deps , thus the issue with Thrift missing.

[ghost]

@maxgolov <https://github.com/maxgolov> and @ThomsonTan <https://github.com/ThomsonTan> That is exactly right. We had already provided CMake with a toolchain because we had gRPC installed via vcpkg on Windows for our project and were adding OpenTelemetry to an existing gRPC project. I would think that would be a fairly common case. We stumbled on using vcpkg on Windows for gRPC because FetchContent_xxxx macros caused issues with Abseil. On Linux, it seems to work fine. The gRPC site recommended using FetchContent_xxxxx and discouraged a system installation of gRPC because it was considered difficult to work with multiple versions of gRPC (on Linux anyway). https://grpc.io/blog/cmake-improvements/ Lots of moving parts!!! J Thanks again! Kurt From: Max Golovanov ***@***.*** Sent: Friday, June 25, 2021 2:50 PM To: open-telemetry/opentelemetry-cpp Cc: Kurt-Rayner-Alpha; Mention Subject: Re: [open-telemetry/opentelemetry-cpp] Errors building -DWITH_JAEGER on Windows - Thrift missing (#874) @ThomsonTan <https://github.com/ThomsonTan> - Yes. You are right. I think what happened in Kurt's case, he supplied CMAKE_TOOLCHAIN_FILE and that is why we did not run the install_windows_deps , thus the issue with Thrift missing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#874 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN3GCI4KCJWNSHBRKP2YWDTUTFVBANCNFSM47GPQZYQ> . <https://github.com/notifications/beacon/AAN3GCLQCPUEJJ4CKT2UYJTTUTFVBA5CNFSM47GPQZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGPEFBDQ.gif>

[maxgolov]

Maybe we can improve it to try installing from our local vcpkg even if CMAKE_TOOLCHAIN_FILE is defined:

if(WITH_JAEGER)
  find_package(Thrift QUIET CONFIG)
  if(NOT Thrift_FOUND)
    # Install Thrift and propagate via vcpkg toolchain file
    if(WIN32)
      install_windows_deps()
    endif()
  endif()
endif()

However, I have a knowledge gap here. Not sure if we can combine the deps from two different locations - customer-supplied CMAKE_TOOLCHAIN_FILE , plus transitively adding ${CMAKE_CURRENT_SOURCE_DIR}/tools/vcpkg/scripts/buildsystems/vcpkg.cmake in addition to that... this is a bit interesting.

If we can't combine the two, then I think the best outcome is clearly document the prerequisites.

[lalitb]

If we can't combine the two, then I think the best outcome is clearly document the prerequisites.

I would vote for documenting this. If CMAKE_TOOLCHAIN_FILE is supplied, the customer needs to ensure all dependencies are met (gRPC, thrift, and others) through it or separately.

[maxgolov]

Yeah, I think we should update the documentation. The "default" process with setup-buildtools.cmd and vcpkg is already taking care of the necessary dependencies.

[ThomsonTan]

Yes, agree that we should update the document on this. Also we could provide better error message to detect the dependency is missing for the customer provided vcpkg, and prompt the resolution accordingly.