Engine can work with Bazel Remote Execution API - #4910
Conversation
|
I realise this has a few open questions. Off the top of my head:
|
|
I'm also happy to split this into smaller commits if desired, but I figured getting the whole compile flow in would be handy to see where it's going. |
stuhood
left a comment
stuhood
left a comment
There was a problem hiding this comment.
Awesome, thanks Daniel! I'm pre-approving because nothing is really a blocker, although I think it would be good to take a shot at pruning the extra protos.
| @@ -39,7 +39,7 @@ fn main() { | |||
| (command, env) | |||
| }; | |||
|
|
|||
| let result = process_executor::run_command(process_executor::ExecuteProcessRequest { argv, env }).unwrap(); | |||
| let result = process_executor::local::run_command_locally(process_executor::ExecuteProcessRequest { argv, env }).unwrap(); | |||
There was a problem hiding this comment.
Take it or leave it, but: there might already be an advantage to adding a trait Executor (even with only one method), as it will help make it obvious that there should be uniformity between the two implementations moving forward.
There was a problem hiding this comment.
They're actually not quite compatible today (slightly different error types), and they're also not on any struct... I'll pull one out when there's actually a caller :)
There was a problem hiding this comment.
Ok, that's fine too!
For the record, it's still possible to implement a trait, as traits can have "static" (no self arg) methods... you can then use them generically via:
fn execute_thing<E: Executor>(cmd: ...) -> ... {
E::execute(cmd)
}
| } | ||
|
|
||
| let mut action = bazel_protos::remote_execution::Action::new(); | ||
| action.set_command_digest(digest(&command)?); |
There was a problem hiding this comment.
Sidenote: it's interesting that this requires double-serializing things for the purposes of digesting them... perhaps there are "set_field_as_raw_bytes" type APIs on the Java/C++ side to avoid double serializing? *shrug
There was a problem hiding this comment.
Yeah, a little annoying. I guess we can try to hack something clever into place if this becomes noticeable :)
|
|
||
| This dump was taken at git sha e17dbfb19652240490cae8adeb89991d13cf9df7. | ||
|
|
||
| It contains many more protos than we actually use, but performing dependency analysis to work out |
There was a problem hiding this comment.
So, I could understand this argument if it were a dozen files or so, but for a few hundred it doesn't quite jive for me. In particular, the cost continues after the initial commit in that whenever we bump/update these files, we'll have hundreds of edits to things we're not using.
Are you expecting that the number of "live" files here is going to be more than a dozen or so? If not, it would be good to do something like "delete all but the root, and re-add things one by one until it compiles".
There was a problem hiding this comment.
Turns out the deps were actually pretty small. Done.
| @@ -0,0 +1,3 @@ | |||
| remote_execution.rs | |||
| remote_execution_grpc.rs | |||
| status.rs | |||
There was a problem hiding this comment.
If lib.rs is always supposed to be the only live file, you can do something like:
*.rs
!lib.rs
...to ignore all non-lib.rs rs files.
37bd525 to
d680c4a
Compare
I'd rather not check in generated code. I understand the readability argument, but just including a link to a gist of the generated code when we change the protos would seem to go far enough toward reviewing what comes out.
Shell seems fine for now. |
5c2ce03 to
237ce00
Compare
The Weezy image uses a glibc which isn't compatible with the binary_utils binaries we publish for Linux.
Sets up the plumbing to do codegen to get Rust interface with protos. Generates .rs files every build. Doesn't check them in. Adds a test to show that we can interact with protos. Splits process_executor into a local and remote module. Doesn't yet touch gRPC.
237ce00 to
5d3155d
Compare
### Problem We're currently using a "very modern" debian version, which unblocked landing #4910. See #4915 for more info. ### Solution Bump to a more-compatibly-built version of `protoc` created in pantsbuild/binaries#40, and downgrade docker images to Wheezy. ### Result Rust protobuf generation will use newer protoc, and CI will use older debian. Fixes #4915.
3 participants
Sets up the plumbing to do codegen to get Rust interface with protos.
Generates .rs files every build. Doesn't check them in.
Adds a test to show that we can interact with protos.
Splits process_executor into a local and remote module.
Doesn't yet touch gRPC.