open-telemetry / opentelemetry-cpp

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

[BUILD] Restore Bazel flag removed from public API #2702

Closed dbolduc closed 2 weeks ago

dbolduc commented 2 weeks ago

A follow up to #2679.

This bool_flag was a part of the public API. If we remove it, anyone using it in downstream code have broken build commands.

So instead of removing it, I think we should mark it as deprecated.

$ bazelist build --//api:with_abseil //api:api

WARNING: opentelemetry-cpp/api/BUILD:8:10: target '//api:with_abseil' is deprecated: The value of this flag is ignored. Bazel builds always use Abseil's pre-adopted `std::` types. You should remove this flag from your build command.
INFO: Analyzed target //api:api (0 packages loaded, 545 targets configured).
INFO: Found 1 target...
Target //api:api up-to-date (nothing to build)
INFO: Elapsed time: 0.574s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

On removing the now useless flag...

I do not know what OpenTelemetry's breaking change policy is. But consider something like:

In my own repo, I would typically open a new issue and tag the code so we remember to clean it up later.

TODO(#XXXX) - Remove this flag when ...
bool_flag( ... )

I am happy to do that here. You will have to tell me when you want to remove it. (If I have convinced y'all to take this PR).

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.67%. Comparing base (497eaf4) to head (cbceed1). Report is 82 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2702/graphs/tree.svg?width=650&height=150&src=pr&token=FJESTYQ2AD&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2702?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #2702 +/- ## ========================================== + Coverage 87.12% 87.67% +0.55% ========================================== Files 200 190 -10 Lines 6109 5853 -256 ========================================== - Hits 5322 5131 -191 + Misses 787 722 -65 ``` [see 106 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2702/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
marcalff commented 2 weeks ago

@keith Please take a look and comment.

I assume keeping a unused flag is ok.

keith commented 2 weeks ago

Keeping and marking deprecated is definitely on. It might be a bit misleading if folks expect it to do something but I definitely understand the issue.