CMake stackdriver enhancements #447

alichnewsky · 2020-06-19T19:41:54Z

do not merge yet, it is still work in progress.
I probably also left quite a few comments that need removing.

many enhancements to CMake support ( see #244 )

support for exporting package and install ( related to Support find_package and install #256 )
- opencensus_lib() now has a HDRS section just like Bazel.
- opencensus_lib() allows declaration of headers in subdirectories, and installs them in proper directory
- build and deploy stackdriver exporters if gRPC and googleapis CMake packages are installed already, and built with -DBUILD_STACKDRIVER_EXPORTER=ON flag.
(failed) attempt at allowing build as shared libraries ( more on that later )
- added ( gcc specific ) helper code to request no undefined symbols in shared libraries.
- outlined and fixed many missing dependencies declarations (fixed only in CMakeLists.txt not BUILD files ). Mostly around missing the context library as a dependency.
- as of now, if you try to build with shared libraries, I think I always give you static libraries with position independent code...
attempt at find_package() before using FetchContent()
two more examples that builds in repository
- Linux, Google Compute Engine and Stackdriver stats exporter specific...
  parses /proc/meminfo and eventually /proc/vmstat and shoves its contents into stackdriver stats exporter.
  uses cpr as FetchContent() dependency.
- Linux specific : parses /proc/meminfo and eventually /proc/vmstat and shoves its contents to stdout stats exporter.

Ongoing issues / unresolved:

support for exporting package and install
- segregating public and private headers ( HDRS of non PUBLIC opencenus_lib() )
  ( it looks like customer can write code that includes internal files, so I install them )
- issues linking private static libraries together into bigger public ones.
  ( as Bazel does. I deploy the internal libraries as static libs unless marked as PRIVATE in opencensus_lib )
  I can think of a way involving linking manually all internal libs only for build with:

opencensus_lib( 
   drink 
   PUBLIC 
   HDRS 
   drink.h 
   SRCS 
   drink.cc 
   DEPS 
   # publicly visible target
   bar 
   # private targets
   $<BUILD_INTERFACE:speakeasy> 
)

not sure I handle correctly yet CMake builds without Stackdriver.
(failed) attempt at allowing build as shared libraries
- attempt failed because of circular dependencies between opencensus_trace and opencensus_context.
  until those are fixed, no hope for producing shared libraries.
- Stackdriver exporters as shared lib : google-cloud-cpp has deprecated use of cpp-makefiles project ...
  I have a branch for that somewhere, but this work assumes there is a googleapis cmake package that contains
  - the monitoring v3 API libraries for stats
  - the cloud trace and tracing v2 API libraries for trace

Unfortunately I did not read many of the outstanding PRs when I started this work, so it may be very redundant with ongoing efforts.

- support for package export and instal - opencensus_lib() now has a HDRS section just like Bazel. - opencensus_lib() allows declaration of headers in subdirectories, and installs them in proper directory - still issues with segregating public and private headers ( HDRS of non PUBLIC opencenus_lib ) ( it looks like customer can write code that includes internal files ) - still issues linking private static libraries together into bigger public ones. ( as Bazel does ) - attempt at allowing build as shared libraries - added helper code to request no undefined symbols in shared libraries. - attempt failed due to circular dependencies ( opencensus_trace and opencensus_context ). Until those are fixed, only static libs will work. - outlined many missing dependencies declarations, only fixed in CMake for now. ideally similar changes would be done to Bazel build, and Bazel would allow to build shared libs as well... - stackdriver / googleapis / google-cloud-cpp changed recently, does not export monitoring/logging and cloud trace/tracing libs. - attempt at building with FetchContent if and only if no existing package has been found. same ideas as using shared libraries : - reuse and to link ability into programs that may already use some of the same deps. - third party ( opencensus-cpp ) should not require significant alterations in consuming client build system ( hence CMake ) and strategy ( - linux-specifc example : using stackdriver exporter on Google Cloud Engine - parse /proc/meminfo and /proc/vmstat to stackdriver - rely via FetchContent on third party software - cpr ( to implement succintly access go GCE instance metadata server ) - re2 ( to brace against variable level of support/correctness in implementation of <regex> with old GCC versions ) - not yet verified on anything but CentOS 7 - in particular no effort has been yet made so that CI via travis or appveyor actually works. As such, not ready to merge

…ocessor flag

…nstalling

…package() on gRPC and googleapis. No FetchContent yet

…TACKDRIVER_EXPORTER CMake option

… change, fix newest example

… it to pass its own unit tests when used with FetchContent ...

…mat and cmake-format manually as docker-format Dockerfile is obsolete. format check should match travis one

…ast file to add

alichnewsky · 2020-06-20T08:59:50Z

tools/docker-format/Dockerfile is obsolete (#448).
hence, no pre-commit formatting, and significant effort to get the linux/ubuntu ci builds green.

related PR here #449.

alichnewsky · 2020-06-20T10:21:40Z

CMakeLists.txt

+endif()
+# clearly there are problems with cyclical deps in this code ( context, trace )
+if(BUILD_SHARED_LIBS)
+  add_link_options("LINKER:--no-undefined")


this requires a version of cmake that's more recent than the one in the CI containers... consider a platform-aware version of this:

set( CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--no-undefined" )

alichnewsky · 2020-06-20T10:22:28Z

CMakeLists.txt

 include(OpenCensusDeps)

 include(OpenCensusHelpers)

+if(BUILD_SHARED_LIBS)


alichnewsky · 2020-06-20T10:33:24Z

circular depdendency between opencensus_trace.so and opencensus_context.so
building shared libraries with code as is gives a linking error:

[ 70%] Linking CXX shared library libopencensus_trace.so
CMakeFiles/opencensus_trace.dir/internal/context_util.cc.o: In function `opencensus::trace::GetCurrentSpan()':
context_util.cc:(.text+0x5): undefined reference to `opencensus::context::Context::Current()'
CMakeFiles/opencensus_trace.dir/internal/with_span.cc.o: In function `opencensus::trace::WithSpan::WithSpan(opencensus::trace::Span const&, bool, bool)':
with_span.cc:(.text+0x4d): undefined reference to `opencensus::context::Context::InternalMutableCurrent()'
CMakeFiles/opencensus_trace.dir/internal/with_span.cc.o: In function `opencensus::trace::WithSpan::ConditionalSwap()':
with_span.cc:(.text+0xcf): undefined reference to `opencensus::context::Context::InternalMutableCurrent()'
CMakeFiles/opencensus_trace.dir/internal/with_span.cc.o: In function `opencensus::trace::WithSpan::~WithSpan()':
with_span.cc:(.text+0x10d): undefined reference to `opencensus::context::Context::InternalMutableCurrent()'
with_span.cc:(.text+0x1a4): undefined reference to `opencensus::context::Context::InternalMutableCurrent()'
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
opencensus/trace/CMakeFiles/opencensus_trace.dir/build.make:406: recipe for target 'opencensus/trace/libopencensus_trace.so' failed
make[2]: *** [opencensus/trace/libopencensus_trace.so] Error 1
make[2]: Leaving directory '/opencensus-cpp/cmake-out'
CMakeFiles/Makefile2:8650: recipe for target 'opencensus/trace/CMakeFiles/opencensus_trace.dir/all' failed
make[1]: *** [opencensus/trace/CMakeFiles/opencensus_trace.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

adding context as a direct depdendency to opencensus_trace.so would give a configuration error. Cyclic depdendencies.

...
-- Configuring done
CMake Error: The inter-target dependency graph contains the following strongly connected component (cycle):
  "opencensus_context" of type SHARED_LIBRARY
    depends on "opencensus_trace" (weak)
  "opencensus_trace" of type SHARED_LIBRARY
    depends on "opencensus_context" (weak)
At least one of these targets is not a STATIC_LIBRARY.  Cyclic dependencies are allowed only among static libraries.
-- Build files have been written to: /opencensus-cpp/cmake-out
Makefile:2658: recipe for target 'cmake_check_build_system' failed
make: *** [cmake_check_build_system] Error 1
make: Leaving directory '/opencensus-cpp/cmake-out'

As a result, as of today, the only way to assemble this code is with a static library.
Worthy of a separate issue ?

lgruen

Sorry, I'm not working on OpenCensus anymore, but can't remove myself as a reviewer from this PR. Dismissing this review using "request changes" might remove it from my PR queue though.

alichnewsky added 3 commits June 19, 2020 11:05

clang-format

618a630

fix missing } to compile. product name prefix as cli option not prepr…

abb9edc

…ocessor flag

alichnewsky requested review from bogdandrutu, g-easy, helly25, lgruen and nevinheintze as code owners June 19, 2020 19:41

alichnewsky added 9 commits June 19, 2020 16:23

fix exports : no-more exporting of gmock, prometheus-cpp::core when i…

b1af7a6

…nstalling

WIP: fix travis-ci ubuntu/clang build

d21434d

CMake: stackdriver exporter build only behind an option. does a find_…

9262d39

…package() on gRPC and googleapis. No FetchContent yet

make protobuf/grpc/stackdriver dependent target contingent to BUILD_S…

21e7a6b

…TACKDRIVER_EXPORTER CMake option

Fix travis-ci build : abseil as FetchContent means linking rule needs…

1818f72

… change, fix newest example

removed re2 as dependency for the new memory example, as I can\'t get…

357b50b

… it to pass its own unit tests when used with FetchContent ...

finally managed to run tools/format.sh

7221529

finally manually assembled the right versions of buildifier,clang-for…

8675d05

…mat and cmake-format manually as docker-format Dockerfile is obsolete. format check should match travis one

WIP : obviously, still no good fix for formatting docker image. one l…

c1fb529

…ast file to add

alichnewsky commented Jun 20, 2020

View reviewed changes

CMakeLists.txt

include(OpenCensusDeps)

include(OpenCensusHelpers)

if(BUILD_SHARED_LIBS)

Copy link

Author

alichnewsky Jun 20, 2020

Choose a reason for hiding this comment

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

redundant

lgruen suggested changes Mar 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake stackdriver enhancements #447

CMake stackdriver enhancements #447

alichnewsky commented Jun 19, 2020 •

edited

alichnewsky commented Jun 20, 2020 •

edited

alichnewsky Jun 20, 2020

alichnewsky Jun 20, 2020

alichnewsky commented Jun 20, 2020

lgruen left a comment

CMake stackdriver enhancements #447

Are you sure you want to change the base?

CMake stackdriver enhancements #447

Conversation

alichnewsky commented Jun 19, 2020 • edited

alichnewsky commented Jun 20, 2020 • edited

alichnewsky Jun 20, 2020

Choose a reason for hiding this comment

alichnewsky Jun 20, 2020

Choose a reason for hiding this comment

alichnewsky commented Jun 20, 2020

lgruen left a comment

Choose a reason for hiding this comment

alichnewsky commented Jun 19, 2020 •

edited

alichnewsky commented Jun 20, 2020 •

edited