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

Make --incompatible_remote_build_event_upload_respect_no_cache a no-op #17989

Conversation

brentleyjones
Copy link
Contributor

@brentleyjones brentleyjones commented Apr 5, 2023

Also removed unneeded tests.

RELNOTES: --incompatible_remote_build_event_upload_respect_no_cache is now a no-op.

@brentleyjones brentleyjones requested a review from a team as a code owner April 5, 2023 13:53
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Apr 5, 2023
@coeuvre coeuvre added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Apr 5, 2023
@brentleyjones
Copy link
Contributor Author

Any context on the delay of this import @kshyanashree?

@ShreeM01
Copy link
Contributor

Any context on the delay of this import @kshyanashree?

Hi @brentleyjones! There were conflicts where I removed few unneeded tests. Still presubmits are failing, Could you please look into it?

@brentleyjones
Copy link
Contributor Author

I'll rebase soon.

@brentleyjones brentleyjones force-pushed the bj/make-incompatible_remote_build_event_upload_respect_no_cache-a-no-op branch from 8c5c3f2 to 4743c64 Compare April 11, 2023 20:12
@ShreeM01
Copy link
Contributor

I'll rebase soon.

Thank you!

RELNOTES: `--incompatible_remote_build_event_upload_respect_no_cache` is now a no-op.
@brentleyjones brentleyjones force-pushed the bj/make-incompatible_remote_build_event_upload_respect_no_cache-a-no-op branch from 4743c64 to 3448f67 Compare April 19, 2023 15:31
Comment on lines -760 to -761
|| remoteOptions.remoteBuildEventUploadMode != RemoteBuildEventUploadMode.ALL
|| !remoteOptions.incompatibleRemoteBuildEventUploadRespectNoCache) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coeuvre The way this was written, using one flag or the other, meant that this always evaluated to true (so always returned false). The tests are breaking with this change, and to get the hold behavior, I would instead change this to remoteOptions == null || true, or just have the function always return false.

Not sure what to do for this.

Copy link
Member

Choose a reason for hiding this comment

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

always return false SGTM.

@brentleyjones
Copy link
Contributor Author

@kshyanashree all green now

@ShreeM01
Copy link
Contributor

@kshyanashree all green now

Thanks @brentleyjones!

@ShreeM01 ShreeM01 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Apr 25, 2023
@brentleyjones brentleyjones deleted the bj/make-incompatible_remote_build_event_upload_respect_no_cache-a-no-op branch April 25, 2023 12:42
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
Also removed unneeded tests.

RELNOTES: `--incompatible_remote_build_event_upload_respect_no_cache` is now a no-op.

Closes bazelbuild#17989.

PiperOrigin-RevId: 526843711
Change-Id: I1305514cf39c29aca9208beb2569d725370b0179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants