remote: refactor make_execute_request to return a struct instead of…

… tuple (#17358) Refactor `make_execute_request` to return a struct instead of tuple so that (1) it is easier to refer to the returned instances by a more descriptive name than `.2`; (2) avoid call sites needing to know the order in which the values are returned if they are unpacking the result; and (3) make it easier to add new fields later (for example, the `input_root_digest` field to be added by #17290).
pantsbuild · Oct 27, 2022 · db17d8e · db17d8e
1 parent f9e4c7a
commit db17d8e
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 36 deletions.
diff --git a/src/rust/engine/process_execution/src/lib.rs b/src/rust/engine/process_execution/src/lib.rs
@@ -89,6 +89,8 @@ pub use crate::immutable_inputs::ImmutableInputs;
 pub use crate::named_caches::{CacheName, NamedCaches};
 pub use crate::remote_cache::RemoteCacheWarningsBehavior;
 
+use crate::remote::EntireExecuteRequest;
+
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ProcessError {
   /// A Digest was not present in either of the local or remote Stores.
@@ -960,8 +962,9 @@ pub fn digest(
   instance_name: Option<String>,
   process_cache_namespace: Option<String>,
 ) -> Digest {
-  let (_, _, execute_request) =
-    remote::make_execute_request(process, instance_name, process_cache_namespace).unwrap();
+  let EntireExecuteRequest {
+    execute_request, ..
+  } = remote::make_execute_request(process, instance_name, process_cache_namespace).unwrap();
   execute_request.action_digest.unwrap().try_into().unwrap()
 }
 

diff --git a/src/rust/engine/process_execution/src/remote.rs b/src/rust/engine/process_execution/src/remote.rs
@@ -806,7 +806,11 @@ impl crate::CommandRunner for CommandRunner {
     trace!("RE capabilities: {:?}", &capabilities);
 
     // Construct the REv2 ExecuteRequest and related data for this execution request.
-    let (action, command, execute_request) = make_execute_request(
+    let EntireExecuteRequest {
+      action,
+      command,
+      execute_request,
+    } = make_execute_request(
       &request,
       self.instance_name.clone(),
       self.process_cache_namespace.clone(),
@@ -910,11 +914,20 @@ fn maybe_add_workunit(
   }
 }
 
+/// Return type for `make_execute_request`. Contains all of the generated REAPI protobufs for
+/// a particular `Process`.
+#[derive(Clone, Debug, PartialEq)]
+pub struct EntireExecuteRequest {
+  pub action: Action,
+  pub command: Command,
+  pub execute_request: ExecuteRequest,
+}
+
 pub fn make_execute_request(
   req: &Process,
   instance_name: Option<String>,
   cache_key_gen_version: Option<String>,
-) -> Result<(remexec::Action, remexec::Command, remexec::ExecuteRequest), String> {
+) -> Result<EntireExecuteRequest, String> {
   let mut command = remexec::Command {
     arguments: req.argv.clone(),
     ..remexec::Command::default()
@@ -1091,7 +1104,11 @@ pub fn make_execute_request(
     ..remexec::ExecuteRequest::default()
   };
 
-  Ok((action, command, execute_request))
+  Ok(EntireExecuteRequest {
+    action,
+    command,
+    execute_request,
+  })
 }
 
 async fn populate_fallible_execution_result_for_timeout(

diff --git a/src/rust/engine/process_execution/src/remote_cache.rs b/src/rust/engine/process_execution/src/remote_cache.rs
@@ -21,7 +21,9 @@ use workunit_store::{
   in_workunit, Level, Metric, ObservationMetric, RunningWorkunit, WorkunitMetadata,
 };
 
-use crate::remote::{apply_headers, make_execute_request, populate_fallible_execution_result};
+use crate::remote::{
+  apply_headers, make_execute_request, populate_fallible_execution_result, EntireExecuteRequest,
+};
 use crate::{
   check_cache_content, CacheContentBehavior, Context, FallibleProcessResultWithPlatform, Platform,
   Process, ProcessCacheScope, ProcessError, ProcessResultSource,
@@ -466,7 +468,9 @@ impl crate::CommandRunner for CommandRunner {
   ) -> Result<FallibleProcessResultWithPlatform, ProcessError> {
     let cache_lookup_start = Instant::now();
     // Construct the REv2 ExecuteRequest and related data for this execution request.
-    let (action, command, _execute_request) = make_execute_request(
+    let EntireExecuteRequest {
+      action, command, ..
+    } = make_execute_request(
       &request,
       self.instance_name.clone(),
       self.process_cache_namespace.clone(),

diff --git a/src/rust/engine/process_execution/src/remote_cache_tests.rs b/src/rust/engine/process_execution/src/remote_cache_tests.rs
@@ -18,7 +18,7 @@ use store::Store;
 use testutil::data::{TestData, TestDirectory, TestTree};
 use workunit_store::{RunId, RunningWorkunit, WorkunitStore};
 
-use crate::remote::{ensure_action_stored_locally, make_execute_request};
+use crate::remote::{ensure_action_stored_locally, make_execute_request, EntireExecuteRequest};
 use crate::{
   CacheContentBehavior, CommandRunner as CommandRunnerTrait, Context,
   FallibleProcessResultWithPlatform, Platform, Process, ProcessCacheScope, ProcessError,
@@ -160,7 +160,9 @@ async fn create_process(store_setup: &StoreSetup) -> (Process, Digest) {
   let process = Process::new(vec![
     "this process will not execute: see MockLocalCommandRunner".to_string(),
   ]);
-  let (action, command, _exec_request) = make_execute_request(&process, None, None).unwrap();
+  let EntireExecuteRequest {
+    action, command, ..
+  } = make_execute_request(&process, None, None).unwrap();
   let (_command_digest, action_digest) =
     ensure_action_stored_locally(&store_setup.store, &command, &action)
       .await

diff --git a/src/rust/engine/process_execution/src/remote_tests.rs b/src/rust/engine/process_execution/src/remote_tests.rs
@@ -22,7 +22,7 @@ use testutil::{owned_string_vec, relative_paths};
 use tokio::time::{sleep, timeout};
 use workunit_store::{RunId, WorkunitStore};
 
-use crate::remote::{CommandRunner, ExecutionError, OperationOrStatus};
+use crate::remote::{CommandRunner, EntireExecuteRequest, ExecutionError, OperationOrStatus};
 use crate::{
   CommandRunner as CommandRunnerTrait, Context, FallibleProcessResultWithPlatform, InputDigests,
   Platform, Process, ProcessCacheScope, ProcessError, ProcessExecutionStrategy,
@@ -148,7 +148,11 @@ async fn make_execute_request() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, None, None),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -241,7 +245,11 @@ async fn make_execute_request_with_instance_name() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, Some("dark-tower".to_owned()), None,),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -332,7 +340,11 @@ async fn make_execute_request_with_cache_key_gen_version() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, None, Some("meep".to_owned()),),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -398,7 +410,11 @@ async fn make_execute_request_with_jdk() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, None, None),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -488,7 +504,11 @@ async fn make_execute_request_with_jdk_and_extra_platform_properties() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, None, None),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -573,7 +593,11 @@ async fn make_execute_request_with_timeout() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, None, None),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -684,7 +708,11 @@ async fn make_execute_request_using_immutable_inputs() {
 
   assert_eq!(
     crate::remote::make_execute_request(&req, None, None),
-    Ok((want_action, want_command, want_execute_request))
+    Ok(EntireExecuteRequest {
+      action: want_action,
+      command: want_command,
+      execute_request: want_execute_request
+    })
   );
 }
 
@@ -695,7 +723,9 @@ async fn successful_with_only_call_to_execute() {
   let op_name = "gimme-foo".to_string();
 
   let mock_server = {
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -734,7 +764,9 @@ async fn successful_after_reconnect_with_wait_execution() {
   let op_name = "gimme-foo".to_string();
 
   let mock_server = {
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -777,7 +809,9 @@ async fn successful_after_reconnect_from_retryable_error() {
   let op_name_2 = "gimme-bar".to_string();
 
   let mock_server = {
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -835,7 +869,7 @@ async fn dropped_request_cancels() {
       mock::execution_server::MockExecution::new(vec![ExpectedAPICall::Execute {
         execute_request: crate::remote::make_execute_request(&request, None, None)
           .unwrap()
-          .2,
+          .execute_request,
         stream_responses: Ok(vec![
           make_incomplete_operation(&op_name),
           make_delayed_incomplete_operation(&op_name, delayed_operation_time),
@@ -889,7 +923,7 @@ async fn server_rejecting_execute_request_gives_error() {
         None,
       )
       .unwrap()
-      .2,
+      .execute_request,
       stream_responses: Err(Status::invalid_argument("".to_owned())),
     }]),
     None,
@@ -909,7 +943,9 @@ async fn server_sending_triggering_timeout_with_deadline_exceeded() {
   let execute_request = echo_foo_request();
 
   let mock_server = {
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -936,7 +972,9 @@ async fn sends_headers() {
   let op_name = "gimme-foo".to_string();
 
   let mock_server = {
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -1128,9 +1166,10 @@ async fn ensure_inline_stdio_is_stored() {
   let mock_server = {
     let op_name = "cat".to_owned();
 
-    let (_, _, execute_request) =
-      crate::remote::make_execute_request(&echo_roland_request().try_into().unwrap(), None, None)
-        .unwrap();
+    let EntireExecuteRequest {
+      execute_request, ..
+    } = crate::remote::make_execute_request(&echo_roland_request().try_into().unwrap(), None, None)
+      .unwrap();
 
     mock::execution_server::TestServer::new(
       mock::execution_server::MockExecution::new(vec![ExpectedAPICall::Execute {
@@ -1229,7 +1268,7 @@ async fn bad_result_bytes() {
           None,
         )
         .unwrap()
-        .2,
+        .execute_request,
         stream_responses: Ok(vec![
           make_incomplete_operation(&op_name),
           MockOperation::new(Operation {
@@ -1263,7 +1302,9 @@ async fn initial_response_error() {
   let mock_server = {
     let op_name = "gimme-foo".to_string();
 
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -1303,7 +1344,9 @@ async fn initial_response_missing_response_and_error() {
   let mock_server = {
     let op_name = "gimme-foo".to_string();
 
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -1338,7 +1381,9 @@ async fn fails_after_retry_limit_exceeded() {
   let execute_request = echo_foo_request();
 
   let mock_server = {
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -1390,7 +1435,9 @@ async fn fails_after_retry_limit_exceeded_with_stream_close() {
 
   let mock_server = {
     let op_name = "foo-bar".to_owned();
-    let (_, _, execute_request) =
+    let EntireExecuteRequest {
+      execute_request, ..
+    } =
       crate::remote::make_execute_request(&execute_request.clone().try_into().unwrap(), None, None)
         .unwrap();
 
@@ -1445,9 +1492,10 @@ async fn execute_missing_file_uploads_if_known() {
   let mock_server = {
     let op_name = "cat".to_owned();
 
-    let (_, _, execute_request) =
-      crate::remote::make_execute_request(&cat_roland_request().try_into().unwrap(), None, None)
-        .unwrap();
+    let EntireExecuteRequest {
+      execute_request, ..
+    } = crate::remote::make_execute_request(&cat_roland_request().try_into().unwrap(), None, None)
+      .unwrap();
 
     mock::execution_server::TestServer::new(
       mock::execution_server::MockExecution::new(vec![
@@ -1467,7 +1515,7 @@ async fn execute_missing_file_uploads_if_known() {
             None,
           )
           .unwrap()
-          .2,
+          .execute_request,
           stream_responses: Ok(vec![
             make_incomplete_operation(&op_name),
             make_successful_operation(