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

process_executor accepts action digests and buildbarn links #11283

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

illicitonion
Copy link
Contributor

This allows you to take an action which ran (perhaps because you have metadata from the remote cluster) and reproduce it, locally or remotely. It also:

  • Switches process_executor to use structopt instead of clap directly, allowing for cleaner arg parsing.
  • Allows specifying prefixes for commands, so you can run under strace or similar.
  • Allows specifying a CAS server without an execution server, so that you can fetch remote digests but perform execution locally.
  • Broadens expand_local_digests to expand_digests with a local-only mode, so that if you're fetching the digest from a remote CAS, you don't get errors complaining that things it references aren't available locally.

I strongly recommend reviewing commits separately :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 4f327d8 on illicitonion:process-executor into 2656353 on pantsbuild:master.

Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

This looks pretty nice!

Also FYI, I'm finishing up the Tonic/gRPC refactor this week. This PR does not look like it will cause much of a merge conflict. (Although I will need to port the BuildBarn proto generation to Prost.) Will tag you as a reviewer of the Tonic PR once I have it submitted in next day or so.

src/rust/engine/fs/store/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/fs/store/src/lib.rs Show resolved Hide resolved
src/rust/engine/hashing/src/lib.rs Outdated Show resolved Hide resolved
src/rust/engine/process_executor/src/main.rs Show resolved Hide resolved
src/rust/engine/process_executor/src/main.rs Outdated Show resolved Hide resolved
@tdyas
Copy link
Contributor

tdyas commented Dec 11, 2020

Also the structopt change seems mostly unrelated to this PR. Probably worth splitting out into its own PR, and then update that struct with whatever additional options you need for this PR.

@illicitonion
Copy link
Contributor Author

Amazing, thanks for the review @tdyas! I'll definitely wait for your tonic stuff to land, I don't want to make that any harder for you than it already is! Will address all comments, and split off a separate structopt PR :)

@illicitonion
Copy link
Contributor Author

Split off structopt changes in #11306

@illicitonion
Copy link
Contributor Author

All addressed :) Still very happy to wait for your prost/tonic migration to land first, though!

@tdyas
Copy link
Contributor

tdyas commented Dec 14, 2020

All addressed :) Still very happy to wait for your prost/tonic migration to land first, though!

Tonic/Prost refactor is in #11307.

@illicitonion
Copy link
Contributor Author

Congrats! Glad it got in!

I've rebased this onto Tonic/Prost - do you want to take another look, or shall I just merge @tdyas? :)

Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

lgtm

This allows you to take an action which ran (perhaps because you have metadata from the remote cluster) and reproduce it, locally or remotely. It also:
* Allows specifying prefixes for commands, so you can run under `strace` or similar.
* Allows specifying a CAS server without an execution server, so that you can fetch remote digests but perform execution locally.
* Broadens `expand_local_digests` to `expand_digests` with a local-only mode, so that if you're fetching the digest from a remote CAS, you don't get errors complaining that things it references aren't available locally.
@illicitonion illicitonion merged commit e80767f into pantsbuild:master Jan 13, 2021
@illicitonion illicitonion deleted the process-executor branch January 13, 2021 13:30
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.

3 participants