Review feedback from #5792. (#5796)

Author: Stu Hood

src/python/pants/bin/engine_initializer.py

@@ -13,7 +13,6 @@
 from pants.base.specs import Specs
 from pants.engine.build_files import create_graph_rules
 from pants.engine.fs import create_fs_rules
-from pants.engine.isolated_process import create_process_rules
 from pants.engine.legacy.address_mapper import LegacyAddressMapper
 from pants.engine.legacy.graph import (LegacyBuildGraph, TransitiveHydratedTargets,
                                        create_legacy_graph_tasks)
@@ -182,7 +181,6 @@ def setup_legacy_graph(pants_ignore_patterns,
       create_legacy_graph_tasks(symbol_table) +
       create_fs_rules() +
       create_graph_rules(address_mapper, symbol_table) +
-      create_process_rules() +
       rules
     )
 

src/python/pants/engine/isolated_process.py

@@ -8,7 +8,6 @@
 import logging
 
 from pants.engine.fs import EMPTY_SNAPSHOT
-from pants.engine.rules import RootRule
 from pants.util.objects import datatype
 
 
@@ -56,10 +55,3 @@ def __new__(cls, argv, env, input_files_digest, digest_length):
 
 class ExecuteProcessResult(datatype(['stdout', 'stderr', 'exit_code'])):
   pass
-
-
-def create_process_rules():
-  """Intrinsically replaced on the rust side."""
-  return [
-      RootRule(ExecuteProcessRequest),
-    ]

src/rust/engine/src/nodes.rs

@@ -265,7 +265,7 @@ impl Select {
       .core
       .rule_graph
       .edges_for_inner(entry)
-      .expect("edges for Snapshot exist.");
+      .expect("Expected edges to exist for Snapshot intrinsic.");
     // Compute PathGlobs for the subject.
     let context = context.clone();
     Select::new(
@@ -274,11 +274,7 @@ impl Select {
       self.variants.clone(),
       edges,
     ).run(context.clone())
-      .and_then(move |path_globs_val| {
-        context.get(Snapshot {
-          subject: externs::key_for(path_globs_val),
-        })
-      })
+      .and_then(move |path_globs_val| context.get(Snapshot(externs::key_for(path_globs_val))))
       .to_boxed()
   }
 
@@ -291,7 +287,7 @@ impl Select {
       .core
       .rule_graph
       .edges_for_inner(entry)
-      .expect("edges for ExecuteProcessResult exist.");
+      .expect("Expected edges to exist for ExecuteProcess intrinsic.");
     // Compute an ExecuteProcessRequest for the subject.
     let context = context.clone();
     Select::new(
@@ -713,9 +709,7 @@ impl From<Scandir> for NodeKey {
 /// A Node that captures an fs::Snapshot for a PathGlobs subject.
 ///
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
-pub struct Snapshot {
-  subject: Key,
-}
+pub struct Snapshot(Key);
 
 impl Snapshot {
   fn create(context: Context, path_globs: PathGlobs) -> NodeFuture<fs::Snapshot> {
@@ -812,7 +806,7 @@ impl Node for Snapshot {
   type Output = fs::Snapshot;
 
   fn run(self, context: Context) -> NodeFuture<fs::Snapshot> {
-    match Self::lift_path_globs(&externs::val_for(&self.subject)) {
+    match Self::lift_path_globs(&externs::val_for(&self.0)) {
       Ok(pgs) => Self::create(context, pgs),
       Err(e) => err(throw(&format!("Failed to parse PathGlobs: {}", e))),
     }
@@ -981,7 +975,7 @@ impl NodeKey {
         keystr(&s.subject),
         typstr(&s.product)
       ),
-      &NodeKey::Snapshot(ref s) => format!("Snapshot({})", keystr(&s.subject)),
+      &NodeKey::Snapshot(ref s) => format!("Snapshot({})", keystr(&s.0)),
     }
   }
 

src/rust/engine/src/rule_graph.rs

@@ -64,7 +64,9 @@ impl From<RootEntry> for Entry {
 
 #[derive(Eq, Hash, PartialEq, Clone, Debug)]
 pub enum Rule {
+  // Intrinsic rules are implemented in rust.
   Intrinsic(Intrinsic),
+  // Task rules are implemented in python.
   Task(Task),
 }
 

tests/python/pants_test/engine/test_isolated_process.py

@@ -10,8 +10,7 @@
 import unittest
 
 from pants.engine.fs import PathGlobs, Snapshot, create_fs_rules
-from pants.engine.isolated_process import (ExecuteProcessRequest, ExecuteProcessResult,
-                                           create_process_rules)
+from pants.engine.isolated_process import ExecuteProcessRequest, ExecuteProcessResult
 from pants.engine.rules import RootRule, rule
 from pants.engine.selectors import Get, Select
 from pants.util.objects import TypeCheckError, datatype
@@ -64,26 +63,21 @@ def argv_from_snapshot(self, snapshot):
 class CatExecutionRequest(datatype([('shell_cat', ShellCat), ('path_globs', PathGlobs)])): pass
 
 
-@rule(ExecuteProcessRequest, [Select(CatExecutionRequest)])
-def cat_files_process_request_input_snapshot(cat_exe_req):
+@rule(Concatted, [Select(CatExecutionRequest)])
+def cat_files_process_result_concatted(cat_exe_req):
   cat_bin = cat_exe_req.shell_cat
   cat_files_snapshot = yield Get(Snapshot, PathGlobs, cat_exe_req.path_globs)
-  yield ExecuteProcessRequest.create_from_snapshot(
+  process_request = ExecuteProcessRequest.create_from_snapshot(
     argv=cat_bin.argv_from_snapshot(cat_files_snapshot),
     env=tuple(),
     snapshot=cat_files_snapshot,
   )
-
-
-@rule(Concatted, [Select(CatExecutionRequest)])
-def cat_files_process_result_concatted(cat_exe_req):
-  cat_process_result = yield Get(ExecuteProcessResult, CatExecutionRequest, cat_exe_req)
+  cat_process_result = yield Get(ExecuteProcessResult, ExecuteProcessRequest, process_request)
   yield Concatted(str(cat_process_result.stdout))
 
 
 def create_cat_stdout_rules():
   return [
-    cat_files_process_request_input_snapshot,
     cat_files_process_result_concatted,
     RootRule(CatExecutionRequest),
   ]
@@ -177,17 +171,6 @@ def argv_from_source_snapshot(self, snapshot):
     return (self.bin_path,) + tuple(snapshot_file_paths)
 
 
-@rule(ExecuteProcessRequest, [Select(JavacCompileRequest)])
-def javac_compile_sources_execute_process_request(javac_compile_req):
-  sources_snapshot = yield Get(
-    Snapshot, PathGlobs, javac_compile_req.javac_sources.path_globs)
-  yield ExecuteProcessRequest.create_from_snapshot(
-    argv=javac_compile_req.argv_from_source_snapshot(sources_snapshot),
-    env=tuple(),
-    snapshot=sources_snapshot,
-  )
-
-
 # TODO: make this contain the snapshot(s?) of the output files (or contain
 # something that contains it) once we've made it so processes can make snapshots
 # of the files they produce.
@@ -196,10 +179,13 @@ class JavacCompileResult(object): pass
 
 @rule(JavacCompileResult, [Select(JavacCompileRequest)])
 def javac_compile_process_result(javac_compile_req):
-  javac_proc_req = yield Get(
-    ExecuteProcessRequest, JavacCompileRequest, javac_compile_req)
-  javac_proc_result = yield Get(
-    ExecuteProcessResult, ExecuteProcessRequest, javac_proc_req)
+  sources_snapshot = yield Get(Snapshot, PathGlobs, javac_compile_req.javac_sources.path_globs)
+  process_request = ExecuteProcessRequest.create_from_snapshot(
+    argv=javac_compile_req.argv_from_source_snapshot(sources_snapshot),
+    env=tuple(),
+    snapshot=sources_snapshot,
+  )
+  javac_proc_result = yield Get(ExecuteProcessResult, ExecuteProcessRequest, process_request)
 
   exit_code = javac_proc_result.exit_code
   if exit_code != 0:
@@ -213,7 +199,6 @@ def javac_compile_process_result(javac_compile_req):
 
 def create_javac_compile_rules():
   return [
-    javac_compile_sources_execute_process_request,
     javac_compile_process_result,
     RootRule(JavacCompileRequest),
   ]
@@ -328,5 +313,5 @@ def mk_example_fs_tree(self):
     return fs_tree
 
   def mk_scheduler_in_example_fs(self, rules):
-    rules = list(rules) + create_fs_rules() + create_process_rules()
+    rules = list(rules) + create_fs_rules()
     return self.mk_scheduler(rules=rules, project_tree=self.mk_example_fs_tree())