open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
860 stars 410 forks source link

Missing value for flag: --experimental_allow_proto3_optional #1442

Closed sirzooro closed 2 years ago

sirzooro commented 2 years ago

Describe your environment CentOS 8 Stream protobuf-3.5.0-15 gcc 10.3.1 cmake 3.20.2 Latest OpenTelemetry CPP source (removed and cloned repo today)

Steps to reproduce

cmake -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DBUILD_SHARED_LIBS=ON -DWITH_OTLP=ON -DWITH_OTLP_GRPC=OFF -DWITH_ZPAGES=ON -DCMAKE_INSTALL_PREFIX=/usr ..
cmake --build . --target all

What is the expected behavior? Build completes successfully.

What is the actual behavior?

[  1%] Creating directories for 'opentelemetry-proto'
[  1%] Performing download step (git clone) for 'opentelemetry-proto'
Cloning into 'opentelemetry-proto'...
HEAD is now at 5c2fe5d Prepare 0.17.0 release (#389)
[  2%] No update step for 'opentelemetry-proto'
[  2%] No patch step for 'opentelemetry-proto'
[  3%] No configure step for 'opentelemetry-proto'
[  3%] No build step for 'opentelemetry-proto'
[  3%] No install step for 'opentelemetry-proto'
[  3%] Completed 'opentelemetry-proto'
[  3%] Built target opentelemetry-proto
[  3%] Generating generated/third_party/opentelemetry-proto/opentelemetry/proto/common/v1/common.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/common/v1/common.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/resource/v1/resource.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/resource/v1/resource.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/trace/v1/trace.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/trace/v1/trace.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/logs/v1/logs.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/logs/v1/logs.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/metrics/v1/metrics.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/metrics/v1/metrics.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/collector/trace/v1/trace_service.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/collector/trace/v1/trace_service.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/collector/logs/v1/logs_service.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/collector/logs/v1/logs_service.pb.cc, generated/third_party/opentelemetry-proto/opentelemetry/proto/collector/metrics/v1/metrics_service.pb.h, generated/third_party/opentelemetry-proto/opentelemetry/proto/collector/metrics/v1/metrics_service.pb.cc
Missing value for flag: --experimental_allow_proto3_optional
gmake[2]: *** [CMakeFiles/opentelemetry_proto.dir/build.make:74: generated/third_party/opentelemetry-proto/opentelemetry/proto/common/v1/common.pb.h] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1429: CMakeFiles/opentelemetry_proto.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
owent commented 2 years ago

Thanks, this problem is already fixed by #1413

javaccar commented 2 years ago

I ran the "steps to reproduce" on your owent-contrib:merge_async-changes_into_main branch and I still get the "Missing value for flag: --experimental_allow_proto3_optional" error. How does #1413 fix this issue?

lalitb commented 2 years ago

--experimental_allow_proto3_optional flag is supported with protoc version 3.12.0, and optional after 3.15.0. I don't think it will work with earlier versions. This seems to be an issue for the distro like CentOS8/Redhat8 which comes with older protoc compiler.

lalitb commented 2 years ago

@sirzooro, @javaccar - Can you upgrade the protoc? I don't see any other solution for now, as metrics proto files uses optional fields. This needs to be driven further with otel-proto repo owners.

owent commented 2 years ago

I ran the "steps to reproduce" on your owent-contrib:merge_async-changes_into_main branch and I still get the "Missing value for flag: --experimental_allow_proto3_optional" error. How does #1413 fix this issue?

Sorry for my mistake, It's #1383 which solve this problem, it just add --experimental_allow_proto3_optional.

1413 had the same changes as #1383 before.

owent commented 2 years ago

--experimental_allow_proto3_optional flag is supported with protoc version 3.12.0, and optional after 3.15.0. I don't think it will work with earlier versions. This seems to be an issue for the distro like CentOS8/Redhat8 which comes with older protoc compiler.

The minimal version of gRPC we support is 1.33.0, which require protobuf 3.13.So I think all versions of protobuf we support have --experimental_allow_proto3_optional, and it's already added in #1383 .

lalitb commented 2 years ago

@owent - I think the with the build option -DWITH_OTLP=ON -DWITH_OTLP_GRPC=OFF, users have the flexibility to try the older version of protoc compiler(outside of gRPC). But either way - with or without gRPC - protoc version needs to be newer to build metrics proto files.

lalitb commented 2 years ago

Sorry for closing it incorrectly :)

owent commented 2 years ago

@owent - I think the with the build option -DWITH_OTLP=ON -DWITH_OTLP_GRPC=OFF, users have the flexibility to try the older version of protoc compiler(outside of gRPC). But either way - with or without gRPC - protoc version needs to be newer to build metrics proto files.

OK, I can raise another PR to add --experimental_allow_proto3_optional only with protobuf >= 3.12

sirzooro commented 2 years ago

@sirzooro, @javaccar - Can you upgrade the protoc? I don't see any other solution for now, as metrics proto files uses optional fields. This needs to be driven further with otel-proto repo owners.

I can use new version. I would have to switch to it anyway to use OTLP metrics when you release GA version.

lalitb commented 2 years ago

OK, I can raise another PR to add --experimental_allow_proto3_optional only with protobuf >= 3.12

I don't think lower protobuf version needs to be supported, as metrics proto files won't compile with that.

Closing the bug, please reopen if there are any further issues, or any change expected here.