Build gRPC with CMake for OLTP exporter by ahaeber · Pull Request #369 · open-telemetry/opentelemetry-cpp

ahaeber · 2020-10-18T21:52:44Z

Uses FetchContent to fetch gRPC. Adds the gRPC plugin to the protoc
command to generate gRPC files. Finally, adds the path for the generated
gRPC and protobuf files to the target include directory property.

Fixes #154

codecov · 2020-10-18T21:58:28Z

Codecov Report

Merging #369 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #369   +/-   ##
=======================================
  Coverage   94.91%   94.91%           
=======================================
  Files         155      155           
  Lines        6882     6882           
=======================================
  Hits         6532     6532           
  Misses        350      350

ahaeber · 2020-10-19T06:56:07Z

Would appreciate if someone could help me to resolve the format check error. I've tried to run ./tools/format.sh and commit all changes in exporters/otlp/CMakeLists.txt so I don't know why it fails.

g-easy · 2020-10-19T06:59:12Z

Are you running cmake-format v0.6.5 locally?

ahaeber · 2020-10-19T07:04:30Z

Are you running cmake-format v0.6.5 locally?

I'm running v0.6.13 locally

Uses FetchContent to fetch gRPC. Adds the gRPC plugin to the protoc command to generate gRPC files. Finally, adds the path for the generated gRPC and protobuf files to the target include directory property. Signed-off-by: Andreas Häber <andy@andyandy.info>

lalitb · 2020-10-19T07:24:25Z

@ahaeber - Welcome to the community, and thanks for working on this issue as your first one. It has been open for some time now.

Just couple of comments

While I don't have strong opinion against using FetchContent to clone the repo as part of build process ( and this is also recommended process as per grpc c++ docs ), the issue I see is that its adds dependency on public internal while building locally (even though it is for first build), something we didn't had till now. Did you look into other options like adding grpc as sub-module or building from locally-checkout sources ?
With these changes, can we now enable cmake test for otlp exporter in non-windows (ubuntu) environment here - either in this or separate PR.

lalitb · 2020-10-19T07:37:46Z

Are you running cmake-format v0.6.5 locally?

I'm running v0.6.13 locally

@avsej faced same issue with this PR : #367 with v0.6.13 locally. I think re-running build worked for him.

ThomsonTan · 2020-10-19T07:45:45Z

One issue I can think of for FetchContent is it needs access network at build time which maybe an issue in some build/CI system, network traffic could only be allowed in setup phase.

As @lalitb mentioned we could test OTLP exporter in CI build based on this PR, but we'll need to install protoc in such case.

g-easy · 2020-10-19T07:46:02Z

I don't know off the top of my head if *.13 formats differently from *.5 but try running *.5. There are scripts to do it via Docker if you don't want to change your local setup.

avsej · 2020-10-19T08:01:59Z

actually I removed 0.6.13, and installed 0.6.5

pip3 install --user cmake_format==0.6.5

ahaeber · 2020-10-19T10:41:22Z

I resolved the formatting issue by following @avsej advice. Thanks!
With .13 it formatted the gtest_add_tests a bit differently.

@lalitb @ThomsonTan with regards to using FetchContent vs other approaches, I just followed the gRPC docs as the easiest approach. But I can rewrite to use ExternalProject_Add with the code in submodules instead if that is preferred. In that case should I use git submodule to add the modules (grpc, c-ares, zlib), or follow the same pattern as in third_party/opentelemetry_cpp (a README.md file that records the commit hash it is copied from)?

While looking at third_party/opentelemetry_cpp/README.md I realized that that code is imported from a different repository. Since I've modified Protobuf.cmake in third_party/opentelemetry-proto here, should that change be done in that repository instead?

pyohannes

Thanks a lot for picking up this issue!

pyohannes · 2020-10-19T23:07:02Z

+  gRPC
+  GIT_REPOSITORY https://github.com/grpc/grpc
+  GIT_TAG v1.32.0)
+FetchContent_MakeAvailable(gRPC)


As far as I know this requires CMake 3.14+, we currently have a minimum requirement of 3.1.

Could you please increase the required CMake version in the global CMakeLists.txt file? Then we can review this together with this PR.

Could upgrade CMake to 3.14 causes compatibility issue? In my Ubuntu 18.04 LTS, the default installed one is 3.10.

It'll take 15 minutes of CI build time to build latest CMake from source for Ubuntu 14.xx and Ubuntu 18.xx

I added this script here: https://github.com/open-telemetry/opentelemetry-cpp/blob/master/tools/setup-cmake.sh and the best scenario to run it while creating docker image, to subsequently reuse that docker image (that's how I build locally). We need to be creative for CI. Either publishing docker image or using GitHub cache action (5GB cache) to pre-cache an image that is built once, then we save 15 mins of CMake buld time.... I assume we do not want to use some Ubuntu PPA to host the latest CMake compiled for old Ubuntu..

pyohannes · 2020-10-19T23:08:14Z

+  PUBLIC $<TARGET_PROPERTY:opentelemetry_proto,INTERFACE_INCLUDE_DIRECTORIES>)
 target_link_libraries(opentelemetry_exporter_otprotocol
                      $<TARGET_OBJECTS:opentelemetry_proto>)



For the tests below, could you please add otlp_exporter_test in addition to recordable_test? That should now link properly with the gRPC dependency available.

Sorry for the long delay. I've tried to add the tests, but I found that gRPC actually uses the same flag to enable tests as in this project. That gave me a problem since one of gRPC's tests didn't build correctly. I currently workaround it by renaming the OT flag to BUILD_TESTING_OT. Next it turned out that need to generate more gRPC code for testing, but unfortunately I haven't had time to complete that yet due to lot's of other work. Hopefully will get better time in the coming weeks to get this completed.

Any thoughts on the flag to enable tests since it clashes with gRPC? Since it slows down the build having to also test gRPC and its dependencies it certainly has disadvantages. But it can also be considered as a good thing to know that gRPC works correctly.

I think gRPC uses grpc_BUILD_TEST. In any case, we should avoid running gRPC tests.

If it's necessary, I have no qualms with renaming BUILD_TESTING and adding a OT specific prefix or postfix.

I think you're right, @pyohannes. It's a Abseil, a dependency to gRPC, that uses BUILD_TESTING too.
Unfortunately, I still can't tests to be built. It appears to be a clash between which protobuf compiler I've got installed on my system and what is included in gRPC added with CMake.

At the top of the generated header files for gRPC it adds:

#include <google/protobuf/port_def.inc> #if PROTOBUF_VERSION < 3011000 #error This file was generated by a newer version of protoc which is #error incompatible with your Protocol Buffer headers. Please update #error your headers. #endif #if 3011002 < PROTOBUF_MIN_PROTOC_VERSION #error This file was generated by an older version of protoc which is #error incompatible with your Protocol Buffer headers. Please #error regenerate this file with a newer version of protoc. #endif

One workaround is to also add Protobuf with FetchContent, but I found a project that struggled to make that approach work google/or-tools#2219 which links to a CMake issue at https://gitlab.kitware.com/cmake/cmake/-/issues/17735

I wonder how this clash between protobuf versions comes about. In such a scenario, we should always use the one added with gRPC via CMake (by adding local paths, include paths and library paths before system paths).

alolita · 2020-12-07T23:29:46Z

Triaged Dec 7 2020: @maxgolov will analyze but @ahaeber needs to file a PR in opentelemetry-proto and then refresh.

ahaeber · 2020-12-08T22:20:05Z

Triaged Dec 7 2020: @maxgolov will analyze but @ahaeber needs to file a PR in opentelemetry-proto and then refresh.

I found that the file related to opentelemetry-proto (third_party/opentelemetry-proto/Protobuf.cmake) was actually not part of that repository. After #377 was merged that file was moved to cmake/opentelemetry-proto.cmake. I've rebased my branch on master now. Anyways would be good with a review on changes in that file.

Now trying to swap the CMake code related to "WITH_PROTOBUF" with using protobuf from gRPC, as suggested by @pyohannes earlier.

maxgolov

I think we need to abandon this PR since latest changes from Tom have added gRPC++ ... Maybe we need to move the Protobuf.cmake into mainline opentelemetry repo? Because we are fetching it as submodule right now, so any change to Protobuf.cmake would have to be done in there.

RE. cmake version -- I added a script that builds CMake from source, any version of it, including latest that has the missing feature. Main issue with building it from source on Ubuntu 18.xx is that this step takes considerable time. Unless we pre-cook or cache docker image with that (or use some PPA to download and install latest on old Ubuntu runners), it'd be impractical to upgrade right now.

Ideally we need CMake 3.18.xx anyways for C++20. But that'd be relevant only for newer Ubuntu distros, so the lower we keep it - the faster our CI build loops are 😄

maxgolov · 2020-12-18T04:30:22Z

  list(APPEND PROTOBUF_INCLUDE_FLAGS "-I${IMPORT_DIR}")
 endforeach()

 add_custom_command(


Ideally this file kinda belongs to this repo: https://github.com/open-telemetry/opentelemetry-proto - there is a makefile in there that handles the protobufs for all languages except C++ and CMake :) .. Logical reasoning: if mainline opentelemetry-proto maintains build scripts for other languages and build systems, then the CMake portion has to be contributed in there as well. Then we'd consume it from there using submodule.

maxgolov · 2020-12-18T22:09:02Z

@ThomsonTan - isn't your recent PR kinda address this already, or are we still missing something? We can probably use vcpkg on Linux as well with your change..

ahaeber · 2020-12-28T19:41:11Z

I think we need to abandon this PR since latest changes from Tom have added gRPC++ ... Maybe we need to move the Protobuf.cmake into mainline opentelemetry repo? Because we are fetching it as submodule right now, so any change to Protobuf.cmake would have to be done in there.

No worries, I'll close this PR.

Protobuf.cmake has been renamed & moved to /cmake/opentelemetry-proto.cmake in master.

ahaeber requested a review from a team October 18, 2020 21:52

ahaeber mentioned this pull request Oct 18, 2020

Build gRPC using CMake #154

Closed

ahaeber force-pushed the issue-154_build-oltp-exporter-with-grpc branch 3 times, most recently from b92d5f4 to 97069b0 Compare October 18, 2020 22:25

ahaeber force-pushed the issue-154_build-oltp-exporter-with-grpc branch from 97069b0 to 39f3bd7 Compare October 19, 2020 07:22

pyohannes reviewed Oct 19, 2020

View reviewed changes

maxgolov suggested changes Dec 18, 2020

View reviewed changes

maxgolov reviewed Dec 18, 2020

View reviewed changes

ahaeber closed this Dec 28, 2020

Conversation

ahaeber commented Oct 18, 2020

Uh oh!

codecov Bot commented Oct 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ahaeber commented Oct 19, 2020

Uh oh!

g-easy commented Oct 19, 2020

Uh oh!

ahaeber commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalitb commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lalitb commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomsonTan commented Oct 19, 2020

Uh oh!

g-easy commented Oct 19, 2020

Uh oh!

avsej commented Oct 19, 2020

Uh oh!

ahaeber commented Oct 19, 2020

Uh oh!

pyohannes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alolita commented Dec 7, 2020

Uh oh!

ahaeber commented Dec 8, 2020

Uh oh!

maxgolov left a comment

Choose a reason for hiding this comment

Uh oh!

maxgolov Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxgolov commented Dec 18, 2020

Uh oh!

ahaeber commented Dec 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov Bot commented Oct 18, 2020 •

edited

Loading

ahaeber commented Oct 19, 2020 •

edited

Loading

lalitb commented Oct 19, 2020 •

edited

Loading

lalitb commented Oct 19, 2020 •

edited

Loading

maxgolov Dec 18, 2020 •

edited

Loading