-
Notifications
You must be signed in to change notification settings - Fork 259
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
Add basic bazel config #536
Conversation
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.
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? |
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. |
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. |
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 |
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. |
Ok thanks we will continue to manually push this config to the BCR as needed |
Something the CPP SIG may look into would be what the Go SIG does. This repository is owned by the SIG, so it maintains the build system. |
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.