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

Add basic bazel config #536

Closed

Conversation

keith
Copy link

@keith keith commented Mar 25, 2024

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.

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.
@keith keith requested a review from a team March 25, 2024 17:19
@keith keith changed the title Add basic bazel config here Add basic bazel config Mar 25, 2024
@tigrannajaryan
Copy link
Member

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
Copy link
Author

keith commented Mar 26, 2024

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
Copy link
Member

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
Copy link
Author

keith commented Mar 26, 2024

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
Copy link
Member

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 keith deleted the ks/add-basic-bazel-config-here branch April 11, 2024 19:59
@keith
Copy link
Author

keith commented Apr 11, 2024

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

@dmathieu
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants