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

Remote: Support the case where output files are under an output direc… #15356

Conversation

coeuvre
Copy link
Member

@coeuvre coeuvre commented Apr 27, 2022

…tory and are omitted from ActionResult.

  • Handle another case when outputs are under output directory.
  • Fix unit test and one more.
  • Add integration test
  • Update RemoteWorker to handle this case.

Supplement for #15329.

@coeuvre coeuvre requested a review from a team as a code owner April 27, 2022 09:08
@coeuvre coeuvre requested a review from tjgq April 27, 2022 09:09
@jmillikin
Copy link
Contributor

I might be misunderstanding, but according to the Remote Execution API protobuf at https://github.com/bazelbuild/remote-apis/blob/main/build/bazel/remote/execution/v2/remote_execution.proto, the ActionResult you describe is an error. If a command has an output file under an output directory, the executor should return an ActionResult containing entries for both.

  // The output files of the action. For each output file requested in the
  // `output_files` or `output_paths` field of the Action, if the corresponding
  // file existed after the action completed, a single entry will be present
  // either in this field, or the `output_file_symlinks` field if the file was
  // a symbolic link to another file (`output_symlinks` field after v2.1).
  // [...]
  repeated OutputFile output_files = 2;

@sgowroji sgowroji added awaiting-user-response Awaiting a response from the author team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Apr 28, 2022
@coeuvre
Copy link
Member Author

coeuvre commented Apr 28, 2022

Technically, yes, executor should include declared output files inside the ActionResult. But there are implementations omit them when I was testing your PR. I am not sure whether they omit them on purpose or is just a bug. I think it wouldn't hurt if Bazel can detect this and continue the build.

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jun 14, 2022
@coeuvre
Copy link
Member Author

coeuvre commented Dec 14, 2022

Closing as this PR is out of date and I don't have the workload to move it forward.

@coeuvre coeuvre closed this Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer 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