[opentelemetry-cpp] opentelemetry-cpp/1.14.2: the option -o with_otlp_grpc=True fails to build when compiling with MSVC (Visual Studio)

[JeremyBorys]

Description

We are building opentelemetry-cpp with msvc and grpc support, the recipe is failing to build. We expect that -o with_otlp_grpc=True should successfully build.

I don't know of any workarounds other than to update the recipe with:

if self.settings.compiler == "msvc":
    tc.extra_cxxflags = ["/DNOMINMAX"]

I have narrowed down the issue to MSVC having preprocessor macros for MIN(a, b) and MAX(a, b) and grpc defines min() and max() functions in a class:
https://github.com/grpc/grpc/blob/master/include/grpc/event_engine/memory_request.h#L45-L46
Since this class is included and used by opentelemetry this fails to correctly interpret the header file. The following are errors caused when compiling this package with the option: -o with_otlp_grpc with msvc:

We have tested this with version 1.14.2 and 1.17.0 and the behaviour is the same

I have already fixed this in my fork

Package and Environment Details

• opentelemetry-cpp/[1.14.2..1.17.0]
• Operating System+version: Windows 11
• Compiler+version: msvc 193
• Docker image: (I did not attempt the containers)
• Conan version: conan 2.8.0
• Python version: Python 3.12.7

Conan profile

Build profile:
[settings]
arch=x86_64
build_type=Release
compiler=msvc
compiler.cppstd=14
compiler.runtime=dynamic
compiler.runtime_type=Release
compiler.version=193
os=Windows

Steps to reproduce

conan create recipe/opentelemetry-cpp/all -o with_otlp_grpc=True --version 1.14.2
conan create recipe/opentelemetry-cpp/all -o with_otlp_grpc=True --version 1.17.0

Logs

Click to expand log

otlp_grpc_exporter_options.cc
  .conan2\p\grpcc1fa87c503b46\p\include\grpc\event_engine\memory_request.h(46,10):
warning C4003: not enough arguments for function-like macro invocation 'min'
  (compiling source file '../../../src/exporters/otlp/src/otlp_grpc_exporter.cc')

.conan2\p\grpcc1fa87c503b46\p\include\grpc\event_engine\memory_request.h(46,10):
error C2059: syntax error: ')'
  (compiling source file '../../../src/exporters/otlp/src/otlp_grpc_exporter.cc')

.conan2\p\grpcc1fa87c503b46\p\include\grpc\event_engine\memory_request.h(46,10):
error C2334: unexpected token(s) preceding ':'; skipping apparent function body
  (compiling source file '../../../src/exporters/otlp/src/otlp_grpc_exporter.cc')

.conan2\p\grpcc1fa87c503b46\p\include\grpc\event_engine\memory_request.h(47,10): warning C4003:
not enough arguments for function-like macro invocation 'max'
  (compiling source file '../../../src/exporters/otlp/src/otlp_grpc_exporter.cc')

.conan2\p\grpcc1fa87c503b46\p\include\grpc\event_engine\memory_request.h(70,2):
error C2143: syntax error: missing ')' before ';'
  (compiling source file '../../../src/exporters/otlp/src/otlp_grpc_exporter.cc')

... (dropped the remaining errors for brevity)


opentelemetry-cpp/1.14.2: ERROR:
Package '1899ae5f3cbc5da043d14ce1dd2a89e5b38f7030' build failed
opentelemetry-cpp/1.14.2: WARN: Build folder C:\Users\borys\.conan2\p\b\opent383b20243a6f4\b\build
ERROR: opentelemetry-cpp/1.14.2: Error in build() method, line 320
        cmake.build()
        ConanException: Error 1 while executing```

</details>

[JeremyBorys]

Here is what I did to resolve this issue:

master...JeremyBorys:conan-center-index:master

I can submit this upstream if we are good with this.

[AbrilRBS]

Hi @JeremyBorys thanks a lot for taking the time to report this issue and propose a solution, we appreciate it!

The idea for the fix is probably the way to go, but I'm thinking if this should be a fix in the grpc side of things, and its recipe should be the one telling its consumers to add that definition to their compiler invokations. I'll double check with the team and will let you know once I know more :)

[JeremyBorys]

Thanks @AbrilRBS

There are definitely pros and cons to both approaches I will be looking forward to hearing back from you :)

[AbrilRBS]

I was checking https://github.com/open-telemetry/opentelemetry-cpp/issues, but they have not had a report for this issue on their end, which strikes me as unusual, as this seems like it would be a problem outside of Conan too.

Also, after talking with the team, your current suggestion would be better than doing it on grpc's side, as we don't control/know every TU that ends up being affected, and that can have unintended consequences!

So the current plan is to report this issue upstream, see if they are aware of it! I can do it, but I'm happy to let you report it if your prefer!

Also, some notes while we wait:

• Conan is able to inject definitions like this from profiles/CLI, so that we don't need a modified recipe to fix it for now until we can fix it for good. This would be done by using the tools.build:defines conf. Something like this in your profile:

[confs]
opentelemetry-cpp/*:tools.build:defines=["NOMINMAX"]

The scope would also make it so it only applies to this recipe.

From the CLI, it would be similar: -c=opentelemetry-cpp/*:tools.build:defines=["NOMINMAX"]

With this, no modifications are needed for the recipe and the fix can be added to your profile/invokation while we find the proper fix for the recipe itself!

• Also, in case waiting is a blocker for you, I'd recommend reading https://docs.conan.io/2/devops/using_conancenter.html - It goes over our recommendations to avoid CCI becoming a blocker, in case you haven't

PS: Just noticed that your proposed solution should use tc.preprocessor_definitions["NOMINMAX"] = 1, which has better handling of the definitions on the Conan side, instead of injecting the flag directly :)

[JeremyBorys]

I was checking https://github.com/open-telemetry/opentelemetry-cpp/issues, but they have not had a report for this issue on their end, which strikes me as unusual, as this seems like it would be a problem outside of Conan too.

I agree. In a discussion with a colleague, we noticed that contradiction as well: the build results differ when using Conan compared to building without it. I didn't spend time investigating why that contradiction exists.

Also, after talking with the team, your current suggestion would be better than doing it on grpc's side, as we don't control/know every TU that ends up being affected, and that can have unintended consequences!

I also agree, that was my interpretation as well that grpc won't be able to export NOMINMAX without breaking some small subset of users. Its also why I am unsure if a fix should go in grpc or in opentelemetry-cpp

So the current plan is to report this issue upstream, see if they are aware of it! I can do it, but I'm happy to let you report it if your prefer!

If you can report it I would prefer that. I am a little unclear of who we are planning to report this too, grpc or opentelemetry-cpp

Thank you for the following suggestion:

[confs]
opentelemetry-cpp/*:tools.build:defines=["NOMINMAX"]

I was looking for exactly this but was unable to locate on the documentation. Is this the right page: https://docs.conan.io/2/reference/tools/build.html ?

This is a really useful feature!

tc.preprocessor_definitions["NOMINMAX"] = 1, which has better handling of the definitions on the Conan side, instead of injecting the flag directly :)

Oh yeah that makes much more sense! Thanks for pointing that out!

[jcar87]

I agree. In a discussion with a colleague, we noticed that contradiction as well: the build results differ when using Conan compared to building without it.

Thanks for going over this!
Does this mean that building the exact same versions of both opentelemetry-cpp and grpc - it doesnt fail without Conan? So far the issue does not seem related to the Conan recipes themselves

I also agree, that was my interpretation as well that grpc won't be able to export NOMINMAX without breaking some small subset of users. Its also why I am unsure if a fix should go in grpc or in opentelemetry-cpp

When in doubt - I would say whoever is including Windows.h without guarding it should be the starting point.

Note that opentelemetry actually removed defining NOMINMAX in their build scripts https://github.com/open-telemetry/opentelemetry-cpp/pull/2420/files - and worked around it a different way.
The problem here is that they have no control over grpc headers like this: https://github.com/grpc/grpc/blob/master/include/grpc/event_engine/memory_request.h#L45-L46

On the other hand, grpc's own headers do have some protection for this:
https://github.com/grpc/grpc/blob/bef33bd26e2317365b2bfa177e721148e0173225/include/grpc/support/port_platform.h#L99-L125 - a bit more comprehensive than what opentelemetry does.
Perhaps if opentelemetry followed a similar approach it would be handy.

As it stands - I'd try to find a minimal reproducible example in a C++ source file that includes the relevant header files to reproduce the issue, and report it to opentelemetry first.

Would you mind updating this issue description with the full log so that we can check which exact versions were resolved by Conan? Thanks!

[jcar87]

Note:
it may be possible to patch https://github.com/grpc/grpc/blob/master/include/grpc/event_engine/memory_request.h#L45-L46

with

  size_t (min)() const { return min_; }
  size_t (max)() const { return max_; }

to overcome this particular error - but it may fail down the line depending on whether whoever calls the .min() .max() class methods also have the Windows.h header