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

Migrate from @bazel_tools//platforms to https://github.com/bazelbuild/platforms #1873

Merged
merged 3 commits into from
Dec 23, 2022
Merged

Conversation

davidmandle
Copy link
Contributor

Changes

This MR migrates platforms constraints from the deprecated @bazel_tools//platforms target to instead depend on the https://github.com/bazelbuild/platforms repository, per bazelbuild/bazel#8622

This enables compatibility with the newly released Bazel 6.0.0.

@davidmandle davidmandle requested a review from a team December 19, 2022 21:21
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #1873 (add3020) into main (94a6783) will decrease coverage by 0.08%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1873      +/-   ##
==========================================
- Coverage   85.79%   85.71%   -0.07%     
==========================================
  Files         171      171              
  Lines        5241     5241              
==========================================
- Hits         4496     4492       -4     
- Misses        745      749       +4     
Impacted Files Coverage Δ
ext/src/http/client/curl/http_client_curl.cc 80.31% <0.00%> (-1.13%) ⬇️
sdk/src/trace/batch_span_processor.cc 90.70% <0.00%> (-0.77%) ⬇️

@ThomsonTan
Copy link
Contributor

Could this break some older version of bazel?

@davidmandle
Copy link
Contributor Author

Could this break some older version of bazel?

I don't know for sure, but I do know that it continues to build successfully with the Bazel version specified in .bazelversion (4.2.0), and that other large projects have already successfully undergone this migration (e.g. grpc: grpc/grpc#29842).

@davidmandle
Copy link
Contributor Author

Let me know if there's any actions I can take to address concerns about Bazel backwards compatibility. For what its worth, the grpc library, which has undergone this migration, tests over its set of supported Bazel versions .

@lalitb
Copy link
Member

lalitb commented Dec 21, 2022

I don’t see any issue if this migration enable supporting bazel v6.0.0, is also working with v4.2.0. As of now we haven’t documented minimal supported bazel version - perhaps we can start doing that once we bump up bazel version to 6.0.0.

Also not related to this PR, but we need to think about continuing g++4.8 support, as that support is restricted with lower version of gRPC and bazel.

Approving this, but need one more approval to merge, and may have to wait a bit during holiday season :). Thanks for the PR.

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems also need upgrade gtest to 1.11.0 or upper and upgrade gRPC to 1.49.0 or upper.
Or the dependencies still use @bazel_tools//platforms, which can not compile with latest bazel.

@lalitb
Copy link
Member

lalitb commented Dec 22, 2022

It seems also need upgrade gtest to 1.11.0 or upper and upgrade gRPC to 1.49.0 or upper.

Oh didn't realize that. Thanks for pointing out. Is it better to upgrade gRPC now, or once we bump the bazel version?

@davidmandle
Copy link
Contributor Author

Thanks @owent, I've upgraded gtest to 1.12.1 and gRPC to 1.49.2. Let me know if there's any other actions I should take.

@lalitb
Copy link
Member

lalitb commented Dec 22, 2022

Thanks @owent, I've upgraded gtest to 1.12.1 and gRPC to 1.49.2. Let me know if there's any other actions I should take.

CMake is still using older versions of these libraries, and also third_part_releases file should have these updated versions.

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now.And the gRPC version in ci/setup_grpc.sh and ci/do_ci.sh can also be updated.

@davidmandle
Copy link
Contributor Author

Thanks. I believe now I've updated the versions throughout.

@lalitb lalitb enabled auto-merge (squash) December 23, 2022 18:07
@lalitb lalitb merged commit 9197bf3 into open-telemetry:main Dec 23, 2022
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