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

Support multiple remote execution digest functions #236

Conversation

EdSchouten
Copy link
Collaborator

Even though the cache can announce support for multiple digest functions, the remote execution system cannot. Let's address this inconsistency by adding a new repeated field that should in the (very) long term replace the singular field.

@EdSchouten EdSchouten force-pushed the eschouten/20221117-multiple-execution-digest-functions branch from affd3a3 to 2046975 Compare November 17, 2022 17:45
EdSchouten added a commit to EdSchouten/bazel that referenced this pull request Nov 18, 2022
The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.
@EdSchouten
Copy link
Collaborator Author

Draft PR to let Bazel respect this: bazelbuild/bazel#16791

EdSchouten added a commit to EdSchouten/bazel that referenced this pull request Nov 24, 2022
The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.
@bergsieker bergsieker requested review from sstriker and removed request for buchgr December 13, 2022 14:55
@sstriker
Copy link
Collaborator

As per the meeting yesterday, awaiting changes by @EdSchouten

@EdSchouten EdSchouten force-pushed the eschouten/20221117-multiple-execution-digest-functions branch 2 times, most recently from 55d1e52 to 0c24e8e Compare December 15, 2022 15:46
@EdSchouten
Copy link
Collaborator Author

Hey @sstriker, @EricBurnett, @bergsieker,

As discussed during Tuesday's meeting, I have revised this PR, taking two things into consideration:

  • Ensuring that none of the phrasing in the specification assumes a single digest function: I looked through the spec, and I wasn't able to find such assumptions/contradictions, so I think we're good. Be sure to point any out if you find them!
  • Tighten the phrasing to state that even if multiple digest functions are supported, that only a single one may be used by individual actions.

PTAL!

@EdSchouten EdSchouten force-pushed the eschouten/20221117-multiple-execution-digest-functions branch 2 times, most recently from cf9f2cc to 44f747b Compare December 15, 2022 16:40
@EdSchouten
Copy link
Collaborator Author

Thanks for the review, @mostynb!

@EdSchouten
Copy link
Collaborator Author

@sstriker @bergsieker @EricBurnett Hey there! Would you folks be interested in reviewing this PR, and PR #235? Thanks!

@sstriker sstriker self-assigned this Feb 14, 2023
EdSchouten added a commit to EdSchouten/bazel that referenced this pull request Feb 24, 2023
The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.
@EricBurnett EricBurnett self-requested a review February 27, 2023 19:16
@EricBurnett
Copy link
Collaborator

@EdSchouten apologies for the long delay!

@sstriker I see you added yourself; are you wanting to give input on this? Otherwise I'll give it a couple days and merge, as I think it's as discussed in the monthly.

@tylerwilliams
Copy link
Contributor

Another vote for merging this :)

@sstriker
Copy link
Collaborator

@EdSchouten apologies for the long delay!

@sstriker I see you added yourself; are you wanting to give input on this? Otherwise I'll give it a couple days and merge, as I think it's as discussed in the monthly.

My intent was to land this somewhere after last meeting. Then my 'round tuits went up in thin air. Apologies for the wait.

Even though the cache can announce support for multiple digest
functions, the remote execution system cannot. Let's address this
inconsistency by adding a new repeated field that should in the (very)
long term replace the singular field.
@EdSchouten EdSchouten force-pushed the eschouten/20221117-multiple-execution-digest-functions branch from 61cf445 to 5608a67 Compare March 14, 2023 08:18
@sstriker sstriker merged commit 64cc5e9 into bazelbuild:main Mar 14, 2023
EdSchouten added a commit to EdSchouten/bazel that referenced this pull request Mar 14, 2023
The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this pull request Mar 16, 2023
The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.

Partial commit for third_party/*, see #16791.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
copybara-service bot pushed a commit to bazelbuild/bazel that referenced this pull request Mar 16, 2023
The following PR on the remote-apis side adds support for remote execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more graceful. This change extends Bazel's server capabilities checking code to take the new field in the execution capabilities into account.

Closes #16791.

PiperOrigin-RevId: 517084447
Change-Id: I72afce6c1fae9e624f9e7ed1936c744ae9c81280
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.

Partial commit for third_party/*, see bazelbuild#16791.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
The following PR on the remote-apis side adds support for remote execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more graceful. This change extends Bazel's server capabilities checking code to take the new field in the execution capabilities into account.

Closes bazelbuild#16791.

PiperOrigin-RevId: 517084447
Change-Id: I72afce6c1fae9e624f9e7ed1936c744ae9c81280
keertk pushed a commit to bazelbuild/bazel that referenced this pull request Jul 25, 2023
* Support multiple remote execution digest functions

The following PR on the remote-apis side adds support for remote
execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more
graceful. This change extends Bazel's server capabilities checking code
to take the new field in the execution capabilities into account.

Partial commit for third_party/*, see #16791.

Signed-off-by: Sunil Gowroji <sgowroji@google.com>
(cherry picked from commit d0cba55)

* Support multiple remote execution digest functions

The following PR on the remote-apis side adds support for remote execution services to announce support for multiple digest functions:

bazelbuild/remote-apis#236

This makes migrating from one digest function to the other more graceful. This change extends Bazel's server capabilities checking code to take the new field in the execution capabilities into account.

Closes #16791.

PiperOrigin-RevId: 517084447
Change-Id: I72afce6c1fae9e624f9e7ed1936c744ae9c81280
(cherry picked from commit f8c8875)

---------

Co-authored-by: Ed Schouten <eschouten@apple.com>
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.

6 participants