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

Generate a configure-time config.h, included in all other header/.cpp file #3085

Open
Romain-Geissler opened this issue Oct 7, 2024 · 2 comments
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@Romain-Geissler
Copy link

My request is done in the context of people using none of CMake or Bazel build system. It's the case in our company. We do build some open source libraries, and "convert it" to the delivery format expected by our internal build system. The way most open source library works (historically, the ones using autotools) is be generating at configure time some opentelemetry/config.h header file (note: you have already one such file, but it's hardcoded, not generated). This way any non default option (like the WITH_STL version, or things like -DENABLE_ASYNC_EXPORT (mentioned in #3084) are directly available in the installed headers, instead of having to pass these configuration macros to the compiler. For me these macros are more or less part of the ABI, as changing them seems to add/remove/change some symbols found in the generated binaries, so it's more logical (according to me), that the generated headers reflect these configuration options. Today we have to dig a big inside the generated CMake files, or read/understand the CMakeList.txt files, looking at implementation details, and find which macros someone NOT using CMake should define when including these opentelemetry headers.

Doing that means:

  • CMake logic is updated a bit to generate a new config file at configure time
  • CMake logic is changed to stop defining all these macros as compilation flags
  • All files (public headers and private ones) are updated to include this new config file first, so that all see a consistant "configuration".

So it's definitely a change involving small changes in many files.

The added value here is to ease the life of people building the lib with CMake, but somehow packaging it as a deliverable for another build system. I think even Linux distros who package this as an RPM would need that, as people using the rpm have no idea how the pre-built library itself was configured (though it seems for now there is no package in fedora for the opentelemetry C++ library).

Since you seem to try to target a big audience for this lib, even allowing to build without a proper recent >= C++11 STL library, I propose this as it seems something you might accept.

@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 7, 2024
@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 7, 2024
Copy link

github-actions bot commented Dec 8, 2024

This issue was marked as stale due to lack of activity.

@github-actions github-actions bot added the Stale label Dec 8, 2024
@owent
Copy link
Member

owent commented Dec 10, 2024

Good point, it can keep a better compatibility , but is there any best practices for Bazel?

@github-actions github-actions bot removed the Stale label Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

3 participants