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

We should move all public definitions in CMakeLists.txt to target_compile_definitions(opentelemetry_api INTERFACE ...) #1311

Closed
owent opened this issue Apr 2, 2022 · 3 comments · Fixed by #1314
Assignees
Labels
bug Something isn't working

Comments

@owent
Copy link
Member

owent commented Apr 2, 2022

Describe your environment

System: Windows
Build System: cmake

Steps to reproduce

cmake <...> -DWITH_ETW=ON

What is the expected behavior?

Now, some definitions are added by add_definitions(...) in CMakeLists.txt and so that these definitions are private. But we should keep the same behaviour between compiling opentelemetry-cpp and using it by users. We can add these public definitions by target_compile_definitions(opentelemetry_api INTERFACE ...) and then these definitions will be auto passed to all cmake targets depend opentelemetry_api.

What is the actual behavior?

Some definitions are missing and library get a different behavior.

Additional context

Should we add prefix to NO_GETENV and NOMINMAX in case of them will conflict with other libraries?

It this's acceptable, it can be assigned to me please.

@owent owent added the bug Something isn't working label Apr 2, 2022
@owent
Copy link
Member Author

owent commented Apr 2, 2022

Can I add OPENTELEMETRY_ prefix for NO_GETENV and NOMINMAX ?

@lalitb
Copy link
Member

lalitb commented Apr 3, 2022

Can I add OPENTELEMETRY_ prefix for NO_GETENV and NOMINMAX ?

Just curious, why specifically for these two macro definitions, as we have lots of other definitions like HAVE_MSGPACK, HAVE_GSL.

@owent
Copy link
Member Author

owent commented Apr 3, 2022

OK, I will just move the definitions into opentelemetry_api and dn not rename them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants