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.

Sign up for GitHub

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

Jump to bottom

Update output_paths to be relative to input root. #191

Closed
wants to merge 1 commit into from
Closed

Update output_paths to be relative to input root. #191

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion build/bazel/remote/execution/v2/remote_execution.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ message Command {
// Other files or directories that may be created during command execution
// are discarded.
//
// The paths are relative to the working directory of the action execution.
// The paths are relative to the input root of the action execution.
// The paths are specified using a single forward slash (`/`) as a path
// separator, even if the execution platform natively uses a different
// separator. The path MUST NOT include a trailing slash, nor a leading slash,
Expand Down Expand Up @@ -651,6 +651,17 @@ message Command {
// property is not recognized by the server, the server will return an
// `INVALID_ARGUMENT`.
repeated string output_node_properties = 8;

// True if the server should interpret output_paths as being relative to the
// input root. False if the server should interpret output_paths as being
// relative to the working directory. This is intended to provide a migration
// path from the old (working_directory) to the new (input root) semantics for
// output paths; eventually this field will be deprecated and ignored by the
// server.
//
// Note that this field does NOT affect the semantics of the output_files and
// output_directories fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the goal is for servers to ignore this field after a transition period, wouldn't it make more sense to have this field affect the semantics of all output_* fields such that servers can remove the old (working directory-relative) logic? Or do you expect servers to also drop support for the deprecated output_files and output_directories at that point?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the reasoning is that if clients are going to be patched up to set the boolean flag, they might as well be adjusted to use output_paths. That implies that if servers drop support for the old semantics, they can drop support for output_files and output_directories at the same time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly. I suspected the appetite for patching up a field that's already deprecated would be low, and this would provide a nudge to migrate to output_paths. I'm certainly willing to reconsider that position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having server implementations drop support for output_files and output_directories at the same time as (or earlier than) this new flag makes sense (incrementing deprecated_api_version), so no objections from me. I also see no issues from the BuildBox (BuildGrid worker) implementation perspective.

bool input_root_relative_output_paths = 9;
}

// A `Platform` is a set of requirements, such as hardware, operating system, or
Expand Down