-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
standalone cmake build instructions for opentelemetry-cpp #612
standalone cmake build instructions for opentelemetry-cpp #612
Conversation
Codecov Report
@@ Coverage Diff @@
## main #612 +/- ##
=======================================
Coverage 94.45% 94.45%
=======================================
Files 197 197
Lines 9183 9183
=======================================
Hits 8674 8674
Misses 509 509 |
INSTALL.md
Outdated
|
||
### Incorporating Into An Existing CMake Project | ||
|
||
TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can describe the same method as is used by nginx instrumentation:
find_package(opentelemetry-cpp REQUIRED)
find_package(gRPC REQUIRED)
target_include_directories(my_target PRIVATE ${OPENTELEMETRY_CPP_INCLUDE_DIRS})
target_link_libraries(my_target PRIVATE ${OPENTELEMETRY_CPP_LIBRARIES} gRPC::grpc++)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to keep it as part of #564 fix. But if it's already used in nginx, let me go though that once. Thanks for sharing the configuration here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's done now.
INSTALL.md
Outdated
```console | ||
$ cd opentelemetry-cpp | ||
$ mkdir build && cd build | ||
$ cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-Werror" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a section to enable position independent code on supported compilers? otel-cpp does not enable it by default, might be useful for someone (would have been for me when I started using it) to add a note about enabling it, i.e. cmake .. -DCMAKE_POSITION_INDEPENDENT_CODE=ON
And perhaps a note about enabling OTLP: -DWITH_OTLP=ON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment. Have updated the document accordingly.
# Change to the directory where you want to create the code repository | ||
$ cd ~ | ||
$ mkdir source && cd source | ||
$ git clone https://github.com/open-telemetry/opentelemetry-cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add option --recursive
to clone submodule as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
submodules needn't be cloned for api/sdk build. It would be required for exporters, but better to have separate instructions for them in their respective folders.
INSTALL.md
Outdated
```console | ||
$ cd opentelemetry-cpp | ||
$ mkdir build && cd build | ||
$ cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-Werror" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to specify "Debug" and "-Werror" options? As there could be multi-configuration targets like for Visual Studio. We could rely on the default config for starter (I think it is still "Debug").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I was thinking about single-configurators like Makefile while writing this. But, DCMAKE_CXX_FLAGS should be valid for Visual Studio too right ? In that case we can keep it as:
cmake .. -DCMAKE_CXX_FLAGS="-Werror"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok have removed both -Werror and Debug now. -Werror
too is specific to g++ compiler.
Co-authored-by: Tom Tan <[email protected]>
Co-authored-by: Tom Tan <[email protected]>
Co-authored-by: Tom Tan <[email protected]>
Co-authored-by: Tom Tan <[email protected]>
Fixes #
Changes
Standalone cmake build instructions for opentelemetry-cpp.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes