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

[BUILD] enum CanonicalCode names too generic... conflict with old C defines #2384

Closed
VivekSubr opened this issue Oct 23, 2023 · 1 comment · Fixed by #2385
Closed

[BUILD] enum CanonicalCode names too generic... conflict with old C defines #2384

VivekSubr opened this issue Oct 23, 2023 · 1 comment · Fixed by #2385
Assignees
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@VivekSubr
Copy link

VivekSubr commented Oct 23, 2023

Steps to reproduce
We use opentelemetry together with oss-asn library: https://www.oss.com/

What is the expected behavior?
No Problems

What is the actual behavior?
CanonicalCode enums conflict with #defines in the oss C code,

/usr/include/opentelemetry/trace/canonical_code.h:108:3: error: expected identifier
  OUT_OF_RANGE = 11,
  ^
/usr/include/ossasn1/ossasn1.h:1696:36: note: expanded from macro 'OUT_OF_RANGE'
#define OUT_OF_RANGE               33 /* parameter value range error */

We've been patching CanocicalCode enums with OTEL_ prefix to get around this, ie, OTEL_OUT_OF_RANGE... but the point remains that the enum names are too generic, this can happen if someone does '#define OK 1' as well.

Proposal -> prefix OTEL_ to all the enums in CanonicalCode

@VivekSubr VivekSubr added the bug Something isn't working label Oct 23, 2023
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 23, 2023
@marcalff
Copy link
Member

Thanks for the report.

From a pure technical point of view, the code polluting the namespace here is /usr/include/ossasn1/ossasn1.h, because it uses C preprocessor macros, what can collide with anything.

The opentelemetry-cpp code does not cause pollution, as the enums values are scoped within opentelemetry::trace, and declared as C++ enums instead of C macro defines.

Now, I do understand that the collision causes problems, and given that ASN1 is not likely to change, the opentelemetry-cpp headers will have to account for this.

SetStatus() used to take a trace::CanonicalCode a long time ago, and was changed to use a trace::StatusCode in this commit:

commit cae3511915ab907ab1f30c620b0efb2634fa5862
Author: Lalit Kumar Bhasin <[email protected]>
Date:   Fri Jan 29 13:57:52 2021 +0530

    Update StatusCode as per specs (#535)

A possibility is to remove header file opentelemetry/trace/canonical_code.h from the code base, as enum CanonicalCode does no longer seem to be used anywhere.

@marcalff marcalff self-assigned this Oct 23, 2023
marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue Oct 23, 2023
@marcalff marcalff changed the title enum CanonicalCode names too generic... conflict with old C defines [BUILD] enum CanonicalCode names too generic... conflict with old C defines Oct 23, 2023
@esigo esigo 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 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
3 participants