-
Notifications
You must be signed in to change notification settings - Fork 438
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
[BUILD] Support pkg-config
#2269
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2269 +/- ##
==========================================
+ Coverage 87.47% 87.52% +0.06%
==========================================
Files 199 199
Lines 5981 5981
==========================================
+ Hits 5231 5234 +3
+ Misses 750 747 -3 |
Sorry about iterating in the CI. It looks like the "Bazel tsan config" build is failing across the board, so I think this PR is ready for review. |
Can we replace the |
opentelemetry-cpp/cmake/pkgconfig.cmake Line 35 in b275af1
... so I think we are already ensuring that the package name in pkgconfig matches the cmake package name. This also means that we can only call
Sorry if I am misunderstanding your question. I am not so sure what this is. I did not touch any of the OTLP components. We can always address them in a follow up PR. |
Thanks, I think it's fine.
|
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.
LGTM and thanks.
Any chance this feature makes it into the v1.11.0 release? |
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.
Nicely done. Thanks for contributing.
@ThomsonTan - Do you think it can be included in the v1.11.0?
This is missed in the v1.1.0 release. As this is a new feature, I think it will be good to stay in main for a release cycle. |
Ack. Thanks for merging. |
Fixes #1850
Generates and installs pkgconfig (
*.pc
) files for the following targets:opentelemetry_api
opentelemetry_logs
opentelemetry_metrics
opentelemetry_resources
opentelemetry_trace
opentelemetry_version
The API is necessary, because we need to capture cflags, such as
-DWITH_ABSEIL
. I did not think installing anopentelemetry_sdk.pc
was necessary or helpful.Motivation: googleapis/google-cloud-cpp#10795
Testing
I added a test to validate the installed package configs. This does not guarantee that their contents are 100% correct.
I am new to using the CI. I am yolo'ing the changes to
.github/workflows/ci.yml
. I ran the steps locally, though, and they were successful:ci/run_docker.sh # Then inside docker... ci/setup_cmake.sh ci/setup_ci_environment.sh ci/install_abseil.sh ci/do_ci.sh cmake.install.test ci/verify_packages.sh
My test installs
opentelemetry-cpp
built-DWITH_ABSEIL
. The dimensions we care about are: [with/without abseil, with/without gsl] (as far as the install logic foropentelemetry_api
goes). But I did not want to write all these tests, so I just checked the abseil case.Testing while developing
I did some more rigorous testing while developing. The changes are in this commit.
Basically it has an executable that uses OTel types. This executable is built using a Makefile, through the use of
pkg-config
. With it, I was able to confirm that the installed libraries link, the include directories work, the compile definitions work, etc.I tested this with/without abseil for all of the added targets listed above.