Skip to content
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

new recipe: OpenTelemetry C++ #7799

Merged
merged 30 commits into from Nov 5, 2021
Merged

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Oct 22, 2021

opentelemetry-cpp/1.0.1

OpenTelemetry C++ can be a bit annoying to build due to the amount of dependencies it has, hopefully a Conan package will simplify its adoption.

This package is a requirement for our own distribution/wrapper of OpenTelemetry C++ #7565 (which will get updated once this is merged).

I'm not the author of OpenTelemetry C++ (but have contributed a bit).


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 requested a review from uilianries October 29, 2021 05:40
@seemk
Copy link
Contributor Author

seemk commented Oct 29, 2021

@uilianries I know you guys are busy, but we are currently blocked due to this PR (#7565 needs it). Is there anything I can do to speed the process up or ping someone? Thanks 🙂

if self.settings.os in ("Linux", "FreeBSD"):
self.cpp_info.system_libs = ["pthread"]

self.cpp_info.libs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like opentelemetry-cpp installs a lot of libraries, and makes these available as cmake targets (in the opentelemetry-cpp:: namespace).
See https://github.com/open-telemetry/opentelemetry-cpp/search?q=install%28TARGETS
and
https://github.com/open-telemetry/opentelemetry-cpp/blob/af75379971bab0fce4a2338edf5b7efcba9967aa/CMakeLists.txt#L432-L435

Shouldn't this recipe use modules to provide this information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate?

Copy link
Contributor

@madebr madebr Oct 30, 2021

Choose a reason for hiding this comment

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

The library allows its users to only use select libraries.
e.g. only use opentelemetry-cpp::opentelemetry_version instead of needing to link to all libraries provided by opentelementry-cpp.
By using conan modules/components, this information can be provided.

When not providing this information, some dependencies might fail to build when these targets are not available.

(perhaps opentelemetry_cpp is a wrong example, but the reason holds for all other libraries)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, although using one lib of it is quite useless. I don't think anyone would do it with Otel C++. Don't really know why they split it up like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@seemk seemk Oct 30, 2021

Choose a reason for hiding this comment

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

I'm kinda lost at package_info at how to achieve it, if the target name for find_package is opentelemetry-cpp, how can I also make Conan (1.42) export OPENTELEMETRY_CPP_INCLUDE_DIRS and OPENTELEMETRY_CPP_LIBRARIES?

Copy link
Contributor

Choose a reason for hiding this comment

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

The easiest way to achieve this is to create a .cmake file in the package method that sets these cmake variables, using the target(s) created by cmake_find_package.
Something like

tools.save(os.path.join(self.package_folder, self._module_file_rel_path), textwrap.dedent("""\
    set(OPENTELEMETRY_CPP_INCLUDE_DIRS )
    set(OPENTELEMETRY_CPP_LIBRARIES opentelemetry-cpp::opentelemetry-cpp)
"""))

And make sure to add self._module_file_rel_path to build_modules and self._module_subfolder to builddirs.

I suggest grepping for some names in the cci repo for examples on how other recipes do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madebr the splunk-opentelemetry PR will get updated after this one, find_package works with opentelemetry-cpp_LIBRARIES, but how can I get it to export OPENTELEMETRY_CPP_LIBRARIES while keeping the package name (for find_package) opentelemetry-cpp?

Copy link
Contributor

@madebr madebr Oct 30, 2021

Choose a reason for hiding this comment

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

See #7799 (comment)
Write a custom .cmake script to the package folder in the package method + add the dir to builddirs + add the cmake script to build_modules.
See the docs + other recipes.

Check e.g. the openssl/3.0 and xapian-core recipes on how this can be done.

Copy link
Contributor Author

@seemk seemk Nov 2, 2021

Choose a reason for hiding this comment

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

@madebr Thanks for the help, I've changed the recipe to use components and added a variables file.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

madebr
madebr previously approved these changes Nov 2, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 30 (527710b5aabf7b9343526dc692e8fb8056283f91):

  • opentelemetry-cpp/1.0.1@:
    All packages built successfully! (All logs)

@seemk
Copy link
Contributor Author

seemk commented Nov 2, 2021

Had to retrigger CI due to stuck CLA check

_cmake = None

def validate(self):
if self.settings.arch != "x86_64":

Choose a reason for hiding this comment

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

What would be needed to provided this for armv8 as well?

Copy link
Contributor Author

@seemk seemk Nov 3, 2021

Choose a reason for hiding this comment

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

Per OpenTelemetry C++ docs only x86-64 is officially supported (and CMake is not officially supported on MacOS): https://github.com/open-telemetry/opentelemetry-cpp#supported-development-platforms

I think it would need to be tested, but that would entail work in OTel C++, so perhaps it might be lifted in newer versions.

@seemk
Copy link
Contributor Author

seemk commented Nov 3, 2021

Had to retrigger CI due to stuck CLA check

@madebr This invalidated your approval, sorry! 👀

@seemk seemk mentioned this pull request Nov 4, 2021
4 tasks
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 6d68936 into conan-io:master Nov 5, 2021
ivanvurbanov pushed a commit to ivanvurbanov/conan-center-index that referenced this pull request Dec 2, 2021
* add opentelemetry-cpp

* debug m1 arm cmake config error

* only support x86_64 for now

* quote protos path

* replace backslashes in protos path

* support VS2019 and up

* attempt removing absl directory amiguity

* fix pep

* enable short_paths

* enable fPIC by default

* disable fPIC option on Windows

* remove debug replace

* use cmake_find_package_multi, remove unused import

* use cmake_find_package_multi for the test package

* support shared libraries, add pthread as a system lib on linux and freebsd

* removed shared option

* reattempt shared libs with a modified opentelemetry_proto cmake

* remove shared lib settings

* enable shared libs, disable on macos

* only allow shared lib on Linux

* export all symbols on Windows

* increase required cmake version

* include ws2_32 syslib on windows

* disable shared libs on everything except Linux

* remove fPIC on Windows

* move source patching to a function, remove comment

* add cmake variables, use components

* remove the need for patching

* remove cxx standard from test_package cmake

* readd test_package cxx standard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants