open-telemetry / opentelemetry-proto

OpenTelemetry protocol (OTLP) specification and Protobuf definitions
https://opentelemetry.io/docs/specs/otlp/
Apache License 2.0
545 stars 242 forks source link

Add basic bazel config #536

Closed keith closed 2 months ago

keith commented 3 months ago

This BUILD file was copied from opentelemetry-cpp, this makes it easier to build and test this repo by itself when things are updated, and then push it to the bzlmod BCR repo.

tigrannajaryan commented 3 months ago

The makefile already verifies the cpp build using the tooling here https://github.com/open-telemetry/build-tools/tree/main/protobuf

Can you clarify why we need another way of building for cpp?

keith commented 3 months ago

This is already being used in opentelemetry-cpp here: https://github.com/open-telemetry/opentelemetry-cpp/blob/02e6ad25ada84c0df7a645c954f820e38fa8c887/bazel/repository.bzl#L98-L108 (and envoyproxy here: https://github.com/envoyproxy/envoy/blob/415caf6cfd609cee5a7f24cda577609b3e32b98c/api/bazel/repository_locations.bzl#L117-L129)

So it felt to me like moving the BUILD file from opentelemetry-cpp to this repo makes it easier to test integration of new versions, without having to copy these files to a local checkout manually. Either way those other repos will continue to use bazel I assume, so it's just about this file being in this repo, or copied amongst all the repos that use opentelemetry-proto with bazel.

tigrannajaryan commented 3 months ago

This repository is for defining the protocol and we have the necessary build steps that verify the ProtoBuf definitions already. I don't see the need for having a bazel build from the perspective of this repository.

Unless this is somehow required to solve a particular problem opentelemetry-cpp has I suggest to keep the bazel-related files out of this repo. Adding them here is additional maintenance burden that I would prefer to avoid.

If opentelemetry-cpp sees the need to have a bazel build we can setup a separate repo maintained by Otel cpp maintainers, like we do for other languages e.g. Go, Java.

keith commented 3 months ago

Since this repo is a transitive dependency of multiple other repos out there, for bazel + bzlmod it needs to be added to the bzlmod central repo, which I did here https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/opentelemetry-proto by patching in this BUILD file, which we'd have to do for all future releases whenever opentelemetry-cpp wants to update the dependency on this repo. If this was checked in it would lower the maintenance burden for whoever wants to update this repo in opentelemetry-cpp

tigrannajaryan commented 2 months ago

Sorry, this fell through the cracks.

For now I do not see good enough arguments to bring additional complexity into this repo. If Otel cpp has some special needs that require bazel build to live here we can consider it, but for now if I understand correctly it seems to be merely about convenience.

I am going to close the PR, but please feel free to reopen if there are new arguments.

keith commented 2 months ago

Ok thanks we will continue to manually push this config to the BCR as needed

dmathieu commented 2 months ago

Something the CPP SIG may look into would be what the Go SIG does. There is an opentelemetry-proto-go repository, which holds the Go generated code from the protobuf.

This repository is owned by the SIG, so it maintains the build system. Maybe an opentelemetry-proto-cpp with generated code could allow you make your bazel usage easier?