-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Should we build with STL and/or ABSEIL support? #29
Comments
Looks like abseil usage is only if you're not using STL: https://github.com/open-telemetry/opentelemetry-cpp/blob/v1.6.0/api/include/opentelemetry/nostd/variant.h So really this is just if we should use the STL or not. |
We can probably build with abseil and get two bird with one stone (since on C++17, abseil will already mostly use STL for its types). |
Abseil is a non-trivial dependency with less than ideal versioning where I'd be -1 on making that the default build. |
Meh, it's already everywhere (including now protobuf's API). 🤷 Especially as I expect most users of this lib to come through arrow, which already depends on abseil. |
Yeah, and additionally they've said upstream that C++17 support effectively requires WITH_STL. From a quick search, the flag still configures whether it tries to link some abseil libraries into OTel. But it's overridden in the source itself via ifdefs, so I say it's a pointless flag and we just use WITH_STL. (Also, if you don't use WITH_STL and don't use WITH_ABSEIL, it just pulls a vendored copy anyways. 🤦) |
Well, that's because at C++17, abseil effectively delegates everything to the stdlib. So I think I'll stay with abseil for now, because it should have a superset of capabilities than what the stdlib offers (happy to be corrected though if I'm wrong on this).
Yeah, I noticed, which is why we're getting symbol collisions now that abseil is on the library path. We have to bite the bullet anyways. |
Comment:
Looks like we're currently not building with STL or Abseil support. At least enabling STL would be an ABI break so we should probably wait until a patch version to prevent breaking folks if we want to enable it.
See:
The text was updated successfully, but these errors were encountered: