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

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

Merged
merged 4 commits into from
Jun 15, 2024

Conversation

dbolduc
Copy link
Contributor

@dbolduc dbolduc commented Jun 14, 2024

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:

  • removing the flag in the next major version bump (v2.x). That is Google's policy (don't laugh).
  • OR giving the flag a few releases in the deprecated state, so downstream projects have a wider window to remove it.

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).

@dbolduc dbolduc requested a review from a team June 14, 2024 21:21
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@            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

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@marcalff
Copy link
Member

@keith Please take a look and comment.

I assume keeping a unused flag is ok.

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, thanks.

@keith
Copy link
Contributor

keith commented Jun 15, 2024

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.

api/BUILD Outdated Show resolved Hide resolved
@marcalff marcalff changed the title fix(ci): restore Bazel flag removed from public API [BUILD] Restore Bazel flag removed from public API Jun 15, 2024
@marcalff marcalff merged commit 9af3c99 into open-telemetry:main Jun 15, 2024
50 checks passed
@dbolduc dbolduc deleted the restore-public-api-flag-bazel branch June 15, 2024 16:37
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