Use find_package instead of adding include directories 1 by 1

[ricardoamaro]

Use find_package instead of adding include directories 1 by 1

1. Use find_package to find and configure the opentelemetry-cpp package
2. Link against the OpenTelemetry C++ library

Fixes #3974

[svrnm]

@open-telemetry/cpp-approvers please take a look. I will see to find some time the next days to verify as well that this works

[svrnm]

I tried it out but ran into the following:

CMake Error at CMakeLists.txt:26 (target_link_libraries):
  Target "dice-server" links to:

    opentelemetry-cpp::opentelemetry-cpp

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

[ricardoamaro]

Quick note for improvement: It should be mentioned somewhere in the get_started.md that there is a requirement for Google test and Google benchmark packages.

I started the get_started.md in a VM, and got:

ricardo:~/cncf/otel-cpp-starter/opentelemetry-cpp/build (main)
$ cmake ..
-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 11.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building for architecture ARCH=x64
-- OPENTELEMETRY_ABI_VERSION_NO=1
-- OPENTELEMETRY_VERSION=1.14.2
-- Performing Test check_cxx_compiler_flag_-Wno-type-limits
-- Performing Test check_cxx_compiler_flag_-Wno-type-limits - Success
-- Performing Test check_cxx_compiler_flag_-Wno-deprecated-declarations
-- Performing Test check_cxx_compiler_flag_-Wno-deprecated-declarations - Success
-- Performing Test check_cxx_compiler_flag_-Wno-unused-parameter
-- Performing Test check_cxx_compiler_flag_-Wno-unused-parameter - Success
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- GTEST_INCLUDE_DIRS   = 
-- GTEST_BOTH_LIBRARIES = GTest::gtest;GTest::gtest_main
CMake Error at CMakeLists.txt:634 (find_package):
  Could not find a package configuration file provided by "benchmark" with
  any of the following names:

    benchmarkConfig.cmake
    benchmark-config.cmake

  Add the installation prefix of "benchmark" to CMAKE_PREFIX_PATH or set
  "benchmark_DIR" to a directory containing one of the above files.  If
  "benchmark" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!

The error was fixed with

$ sudo apt-get install libgtest-dev libbenchmark-dev libgmock-dev

[ricardoamaro]

Another possible improvement:
When we say "Create a file called CMakeLists.txt to define the Oat++ library directories,
include paths, and link against Oat++ during the compilation process."

Should explicitly say if that file is created in the roll-dice folder or in the otel-cpp-starter folder

[ricardoamaro]

@svrnm this should be working now.
I had to sudo make install to have the libraries visible. Unsure if there is another way.

ricardo:~/cncf/otel-cpp-starter/roll-dice/build
$ cmake ..
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Found opentelemetry-cpp: /usr/local/include  
-- Configuring done
-- Generating done
-- Build files have been written to: /home/ricardo/cncf/otel-cpp-starter/roll-dice/build

[svrnm]

Quick note for improvement: It should be mentioned somewhere in the get_started.md that there is a requirement for Google test and Google benchmark packages.

If I remember correctly there was a cmake flag that allows you to disable tests and by that remove the requirement for those additional libraries (BUILD_TESTING?), I would prefer to minimize the packages that someone needs to install as this getting started should help people to have something working as quickly as possible

Should explicitly say if that file is created in the roll-dice folder or in the otel-cpp-starter folder

This should be created in the roll-dice folder, if this is not clear from the current wording, please update.

I had to sudo make install to have the libraries visible. Unsure if there is another way.

The goal was to have dependencies not globally installed, because people might want to work through the tutorial, try things out and then delete everything without having any remains in their system. (I also saw that there is a sudo make install for oatpp, this should not be there and is also not required)

For the updated CMakeList I get a different error now:

CMake Error at /opt/homebrew/lib/cmake/opentelemetry-cpp/opentelemetry-cpp-target.cmake:75 (set_target_properties):
  The link interface of target "opentelemetry-cpp::api" contains:

    absl::bad_variant_access

  but the target was not found.  Possible reasons include:

I assume this is related to the fact that I build with abseil (see the note in the documentation for that), since I can not get it built with the non-abseil functions

[ricardoamaro]

Thanks @svrnm!

Question: regarding using the option(BUILD_TESTING "Whether to enable tests" ON) are you saying that, instead of mentioning that Google test and benchmark packages are required, we should mention to do cmake .. -DBUILD_TESTING=OFF?

Help request: For the linking, I believe we will need someone from sig:cpp to propose another way instead of make install. That's the only way I saw for now that it works.

[svrnm]

Thanks @svrnm!

Question: regarding using the option(BUILD_TESTING "Whether to enable tests" ON) are you saying that, instead of mentioning that Google test and benchmark packages are required, we should mention to do cmake .. -DBUILD_TESTING=OFF?

Yes, that would be my preferred solution

[ricardoamaro]

@svrnm I think all the changes are good now.

[owent]

LGTM

[marcalff]

LGTM, thanks for the cleanup.

[marcalff]

Hi @ricardoamaro.

Thanks for the PR, it looks good from the opentelemetry-cpp maintainers point of view.

Ok to merge.

[ricardoamaro]

@marcalff I don't have authorization to merge. Can someone, who has that it please merge?

[svrnm]

Please take a look at my requested changes. Verify that neither oatpp nur opentelemetry are installed in any global locations and the cmake is picking them up from there.

[ricardoamaro]

Apparently, minimum required cmake version just got updated to 3.25 in oatpp/oatpp@0623281
Doing new tests with the required changes

[ricardoamaro]

I suspect that maybe this last version of oatpp (1.40) has also some incompatibility with the existing code, due to this error...

$ vi ../main.cpp 
~/cncf/otel-cpp-starter/roll-dice/build
$ cmake --build .
[ 50%] Building CXX object CMakeFiles/dice-server.dir/main.cpp.o
otel-cpp-starter/roll-dice/main.cpp: In function ‘int main()’:
otel-cpp-starter/roll-dice/main.cpp:33:16: error: ‘oatpp::base::Environment’ has not been declared
   33 |   oatpp::base::Environment::init();
      |                ^~~~~~~~~~~
otel-cpp-starter/roll-dice/main.cpp:36:16: error: ‘oatpp::base::Environment’ has not been declared
   36 |   oatpp::base::Environment::destroy();
      |                ^~~~~~~~~~~
gmake[2]: *** [CMakeFiles/dice-server.dir/build.make:76: CMakeFiles/dice-server.dir/main.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/dice-server.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2

[ricardoamaro]

Also fixed this warning:

$ cmake --build .
(..)
otel-cpp-starter/roll-dice/main.cpp:58:29: warning: format ‘%s’ expects argument of type ‘char*’, but argument 4 has type ‘const void*’ [-Wformat=]
   58 |   OATPP_LOGI("Dice Server", "Server running on port %s", connectionProvider->getProperty("port").getData());
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                                                         |
      |                                                                                                         const void*
otel-cpp-starter/roll-dice/../oatpp/src/oatpp/core/base/./Environment.hpp:568:80: note: in definition of macro ‘OATPP_LOGI’
  568 |   oatpp::base::Environment::logFormatted(oatpp::base::Logger::PRIORITY_I, TAG, __VA_ARGS__);
      |                                                                                ^~~~~~~~~~~
otel-cpp-starter/roll-dice/main.cpp:58:54: note: format string is defined here
   58 |   OATPP_LOGI("Dice Server", "Server running on port %s", connectionProvider->getProperty("port").getData());
      |                                                     ~^
      |                                                      |
      |                                                      char*
      |                                                     %p
[100%] Linking CXX executable dice-server
[100%] Built target dice-server

[ricardoamaro]

All warnings and errors fixed:

$ cmake ..
-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 11.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Found opentelemetry-cpp: otel-cpp-starter/otel-cpp/include
-- Configuring done (0.4s)
-- Generating done (0.0s)
-- Build files have been written to: otel-cpp-starter/roll-dice/build

$ cmake --build .
[ 50%] Building CXX object CMakeFiles/dice-server.dir/main.cpp.o
[100%] Linking CXX executable dice-server
[100%] Built target dice-server

$ ./dice-server
 I |2024-04-23 23:28:38 1713911318916284| Dice Server:Server running on port 8080

[svrnm]

thanks @ricardoamaro! I will give it another look within the next few days and if all goes well we should be able to merge this soon!

[svrnm]

please take a look at my comments.

I struggle to build the sample app myself right now, but it feels like it is an issue on my end

[svrnm]

thank you for working through this @ricardoamaro !