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

Add remote output service proto #21140

Closed
wants to merge 6 commits into from

Conversation

coeuvre
Copy link
Member

@coeuvre coeuvre commented Jan 30, 2024

Working towards #20933.

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jan 30, 2024
@EdSchouten
Copy link
Contributor

Hey! Would you want me to continue posting feedback in the document, or should I put it in the PR going forward?

@sgowroji sgowroji added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Jan 30, 2024
@coeuvre
Copy link
Member Author

coeuvre commented Jan 31, 2024

Let's continue the review here!

@meisterT meisterT requested a review from a team February 1, 2024 09:49
Copy link
Contributor

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @coeuvre!

src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service_rev2.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
@coeuvre
Copy link
Member Author

coeuvre commented Feb 29, 2024

@EdSchouten @tjgq Sorry for the delay, I just had a long vacation. I have updated the PR, please take another look!

Copy link
Contributor

@EdSchouten EdSchouten 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 a lot for working on this, @coeuvre!

src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
src/main/protobuf/bazel_output_service.proto Outdated Show resolved Hide resolved
@EdSchouten
Copy link
Contributor

FYI: I just finished a change to reimplement bb_clientd on top of this new protocol. I'll make sure to publish it as soon as this PR gets merged.

@copybara-service copybara-service bot closed this in 4a2c36b Mar 6, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Mar 6, 2024
@brentleyjones
Copy link
Contributor

@coeuvre Should this be cherry-picked into 7.x?

@coeuvre
Copy link
Member Author

coeuvre commented Mar 6, 2024

The plan is to cherry-pick both the protocol and client code into 7.2.

@coeuvre coeuvre deleted the remote_output_service branch March 6, 2024 16:33
EdSchouten added a commit to buildbarn/bb-remote-execution that referenced this pull request Mar 6, 2024
Google has merged a change for adding the Remote Output Service protocol
to their code base: bazelbuild/bazel#21140. They
did however make a couple of changes to it. For example:

- The protocol has been made REv2 agnostic. All explicit coupling to
  REv2 has been moved into a helper protocol.

- BatchCreate() has been renamed to StageArtifacts(). It can only be
  used to create files and directories. Not symlinks. It also doesn't
  provide options to clean directories. This is likely going to hurt
  runfiles directory creation, but we'll see whether that is actually a
  problem in practice.

- BatchStat() no longer provides follow_symlinks and
  include_file_digests. Symlinks are no longer followed, and file
  digests should always be included.

- There is a FinalizeArtifacts() function. This function can be used to
  reliably implement file modification tracking. As we don't implement
  that yet, we can simply let it be a stub for the time being.
EdSchouten added a commit to buildbarn/bb-remote-execution that referenced this pull request Mar 6, 2024
Google has merged a change for adding the Remote Output Service protocol
to their code base: bazelbuild/bazel#21140. They
did however make a couple of changes to it. For example:

- The protocol has been made REv2 agnostic. All explicit coupling to
  REv2 has been moved into a helper protocol.

- BatchCreate() has been renamed to StageArtifacts(). It can only be
  used to create files and directories. Not symlinks. It also doesn't
  provide options to clean directories. This is likely going to hurt
  runfiles directory creation, but we'll see whether that is actually a
  problem in practice.

- BatchStat() no longer provides follow_symlinks and
  include_file_digests. Symlinks are no longer followed, and file
  digests should always be included.

- There is a FinalizeArtifacts() function. This function can be used to
  reliably implement file modification tracking. As we don't implement
  that yet, we can simply let it be a stub for the time being.
EdSchouten added a commit to buildbarn/bb-clientd that referenced this pull request Mar 6, 2024
Google has merged a change for adding the Remote Output Service protocol
to their code base: bazelbuild/bazel#21140. They
did however make a couple of changes to it. For example:

- The protocol has been made REv2 agnostic. All explicit coupling to
  REv2 has been moved into a helper protocol.

- BatchCreate() has been renamed to StageArtifacts(). It can only be
  used to create files and directories. Not symlinks. It also doesn't
  provide options to clean directories. This is likely going to hurt
  runfiles directory creation, but we'll see whether that is actually a
  problem in practice.

- BatchStat() no longer provides follow_symlinks and
  include_file_digests. Symlinks are no longer followed, and file
  digests should always be included.

- There is a FinalizeArtifacts() function. This function can be used to
  reliably implement file modification tracking. As we don't implement
  that yet, we can simply let it be a stub for the time being.
@coeuvre
Copy link
Member Author

coeuvre commented Mar 11, 2024

@bazel-io fork 7.2.0

iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Mar 25, 2024
Working towards bazelbuild#20933.

Closes bazelbuild#21140.

PiperOrigin-RevId: 613219486
Change-Id: Ie4e02e04e2dcfc444234fb164a73865edf8d9ba0
iancha1992 added a commit that referenced this pull request Mar 26, 2024
Working towards #20933.

Closes #21140.

PiperOrigin-RevId: 613219486
Change-Id: Ie4e02e04e2dcfc444234fb164a73865edf8d9ba0

Commit
4a2c36b

Co-authored-by: Chi Wang <chiwang@google.com>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 7.2.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.2.0rc1. Thanks!

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.

None yet

6 participants