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 ServerCapabilities into RemoteExecutionClient #18269

Closed

Conversation

sluongng
Copy link
Contributor

In #18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

In bazelbuild#18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.
@sluongng sluongng requested a review from a team as a code owner April 29, 2023 10:27
@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 29, 2023
Comment on lines -201 to +209
/*executionInfo=*/ ImmutableMap.<String, String>of(),
/*runfilesSupplier=*/ null,
/*filesetMappings=*/ ImmutableMap.of(),
/*inputs=*/ NestedSetBuilder.create(
/* executionInfo= */ ImmutableMap.<String, String>of(),
/* runfilesSupplier= */ null,
/* filesetMappings= */ ImmutableMap.of(),
/* inputs= */ NestedSetBuilder.create(
Order.STABLE_ORDER, ActionInputHelper.fromPath("input")),
/*tools=*/ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/*outputs=*/ ImmutableSet.of(
/* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* outputs= */ ImmutableSet.of(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These hunks were added as a result of running

git diff --name-only | grep java$ | xargs google-java-format -i

Please let me know if I should remove them.

sluongng added a commit to sluongng/bazel that referenced this pull request Apr 29, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request Apr 29, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request May 1, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request May 1, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
@sluongng
Copy link
Contributor Author

sluongng commented May 2, 2023

cc: @tjgq I think both this PR and #18270 are ready for review. But I would wait for this to get merged first before rebasing the other one.

@sluongng
Copy link
Contributor Author

sluongng commented May 9, 2023

@tjgq @coeuvre is this something that you folks to review? This change is relatively straight forward 👀

Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. LGTM.

@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 May 10, 2023
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 15, 2023
sluongng added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request May 15, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
@sluongng
Copy link
Contributor Author

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 17, 2023
@iancha1992
Copy link
Member

@bazel-io fork 6.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 17, 2023
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request May 17, 2023
In bazelbuild#18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes bazelbuild#18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
In bazelbuild#18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes bazelbuild#18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0
sluongng added a commit to sluongng/bazel that referenced this pull request May 30, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request May 30, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
copybara-service bot pushed a commit that referenced this pull request Jun 5, 2023
This is a follow up to #18269, toward the discussion in #18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.

Closes #18270.

PiperOrigin-RevId: 537817532
Change-Id: Ie299065a7c91abbfc7a4f181410f6a57471e7dc8
copybara-service bot pushed a commit that referenced this pull request Jun 5, 2023
*** Reason for rollback ***

Breaks remote server which only support REPI v2.0.

*** Original change description ***

Conditionally set output_paths based on Remote Executor capabilities

This is a follow up to #18269, toward the discussion in #18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.

Closes #18270.

PiperOrigin-RevId: 537883626
Change-Id: I13c03cb3f4d64f106dc90767a6e62dfbae4027e2
iancha1992 added a commit that referenced this pull request Jun 6, 2023
In #18202, we discussed the possibility of conditionally using the new
field exclusively based on the Remote Execution Server's capabilities.

Capture Remote Execution Server's capabilities and store it in
RemoteExecutor implementations for furture usage.

Closes #18269.

PiperOrigin-RevId: 531999688
Change-Id: I370869a45c804af1ec499b9c1654c6977c7ab7d0

Co-authored-by: Son Luong Ngoc <[email protected]>
sluongng added a commit to sluongng/bazel that referenced this pull request Jun 13, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request Jun 20, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request Jun 21, 2023
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request Jul 22, 2024
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request Jul 22, 2024
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
sluongng added a commit to sluongng/bazel that referenced this pull request Jul 24, 2024
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
copybara-service bot pushed a commit that referenced this pull request Sep 16, 2024
This is a follow up to #18269, toward the discussion in #18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.

Closes #18270.

PiperOrigin-RevId: 675060530
Change-Id: If08975b48696e06da0da91898ba2dd4b1c6677d2
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.

5 participants