From 04c2f95df9947f7c4cf14415f0eb59c5e302135d Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 13 Aug 2018 16:59:24 -0700 Subject: [PATCH 1/2] Simplify ExecuteProcessRequest construction Remove create_from_snapshot and create_with_empty_snapshot - they don't pull their weight. Allow default values for all defaultable values. --- .../tasks/jvm_compile/javac/javac_compile.py | 4 +- src/python/pants/engine/isolated_process.py | 58 ++++++------------- .../engine/test_isolated_process.py | 37 +++++++----- 3 files changed, 41 insertions(+), 58 deletions(-) diff --git a/src/python/pants/backend/jvm/tasks/jvm_compile/javac/javac_compile.py b/src/python/pants/backend/jvm/tasks/jvm_compile/javac/javac_compile.py index f53040340c0..4e7328f7804 100644 --- a/src/python/pants/backend/jvm/tasks/jvm_compile/javac/javac_compile.py +++ b/src/python/pants/backend/jvm/tasks/jvm_compile/javac/javac_compile.py @@ -216,9 +216,9 @@ def _execute_hermetic_compile(self, cmd, ctx): os.path.relpath(f.path.replace('.java', '.class'), ctx.target.target_base) for f in input_snapshot.files if f.path.endswith('.java') ) - exec_process_request = ExecuteProcessRequest.create_from_snapshot( + exec_process_request = ExecuteProcessRequest( argv=tuple(cmd), - snapshot=input_snapshot, + input_files=input_snapshot.directory_digest, output_files=output_files, description='Compiling {} with javac'.format(ctx.target.address.spec), ) diff --git a/src/python/pants/engine/isolated_process.py b/src/python/pants/engine/isolated_process.py index ef00a09eab1..078115180d2 100644 --- a/src/python/pants/engine/isolated_process.py +++ b/src/python/pants/engine/isolated_process.py @@ -8,7 +8,7 @@ import six -from pants.engine.fs import EMPTY_SNAPSHOT, DirectoryDigest +from pants.engine.fs import DirectoryDigest from pants.engine.rules import RootRule, rule from pants.engine.selectors import Select from pants.util.objects import Exactly, SubclassesOf, TypeCheckError, datatype @@ -21,21 +21,20 @@ class ExecuteProcessRequest(datatype([ ('argv', tuple), - ('env', tuple), ('input_files', DirectoryDigest), + ('description', SubclassesOf(*six.string_types)), + ('env', tuple), ('output_files', tuple), ('output_directories', tuple), # NB: timeout_seconds covers the whole remote operation including queuing and setup. ('timeout_seconds', Exactly(float, int)), - ('description', SubclassesOf(*six.string_types)), ])): """Request for execution with args and snapshots to extract.""" - @classmethod - def create_from_snapshot( + def __new__( cls, argv, - snapshot, + input_files, description, env=None, output_files=(), @@ -45,50 +44,27 @@ def create_from_snapshot( if env is None: env = () else: - cls._verify_env_is_dict(env) + if not isinstance(env, dict): + raise TypeCheckError( + cls.__name__, + "arg 'env' was invalid: value {} (with type {}) must be a dict".format( + env, + type(env) + ) + ) env = tuple(item for pair in env.items() for item in pair) - return ExecuteProcessRequest( + return super(ExecuteProcessRequest, cls).__new__( + cls, argv=argv, env=env, - input_files=snapshot.directory_digest, + input_files=input_files, + description=description, output_files=output_files, output_directories=output_directories, timeout_seconds=timeout_seconds, - description=description, ) - @classmethod - def create_with_empty_snapshot( - cls, - argv, - description, - env=None, - output_files=(), - output_directories=(), - timeout_seconds=_default_timeout_seconds, - ): - return cls.create_from_snapshot( - argv, - EMPTY_SNAPSHOT, - description, - env, - output_files, - output_directories, - timeout_seconds, - ) - - @classmethod - def _verify_env_is_dict(cls, env): - if not isinstance(env, dict): - raise TypeCheckError( - cls.__name__, - "arg 'env' was invalid: value {} (with type {}) must be a dict".format( - env, - type(env) - ) - ) - class ExecuteProcessResult(datatype(['stdout', 'stderr', 'output_directory_digest'])): """Result of successfully executing a process. diff --git a/tests/python/pants_test/engine/test_isolated_process.py b/tests/python/pants_test/engine/test_isolated_process.py index 34c74748a6d..197f32c48ac 100644 --- a/tests/python/pants_test/engine/test_isolated_process.py +++ b/tests/python/pants_test/engine/test_isolated_process.py @@ -72,9 +72,9 @@ class CatExecutionRequest(datatype([('shell_cat', ShellCat), ('path_globs', Path 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) - process_request = ExecuteProcessRequest.create_from_snapshot( + process_request = ExecuteProcessRequest( argv=cat_bin.argv_from_snapshot(cat_files_snapshot), - snapshot=cat_files_snapshot, + input_files=cat_files_snapshot.directory_digest, description='cat some files', ) cat_process_result = yield Get(ExecuteProcessResult, ExecuteProcessRequest, process_request) @@ -102,9 +102,10 @@ def gen_argv(self): @rule(ExecuteProcessRequest, [Select(JavacVersionExecutionRequest)]) def process_request_from_javac_version(javac_version_exe_req): - yield ExecuteProcessRequest.create_with_empty_snapshot( + yield ExecuteProcessRequest( argv=javac_version_exe_req.gen_argv(), description=javac_version_exe_req.description, + input_files=EMPTY_DIRECTORY_DIGEST, ) @@ -168,9 +169,9 @@ def javac_compile_process_result(javac_compile_req): raise ValueError("Can only compile .java files but got {}".format(java_file)) sources_snapshot = yield Get(Snapshot, PathGlobs, PathGlobs(java_files, ())) output_dirs = tuple({os.path.dirname(java_file) for java_file in java_files}) - process_request = ExecuteProcessRequest.create_from_snapshot( + process_request = ExecuteProcessRequest( argv=javac_compile_req.argv_from_source_snapshot(sources_snapshot), - snapshot=sources_snapshot, + input_files=sources_snapshot.directory_digest, output_directories=output_dirs, description='javac compilation' ) @@ -196,10 +197,11 @@ def create_javac_compile_rules(): class ExecuteProcessRequestTest(unittest.TestCase): def _default_args_execute_process_request(self, argv=tuple(), env=None): env = env or dict() - return ExecuteProcessRequest.create_with_empty_snapshot( + return ExecuteProcessRequest( argv=argv, description='', env=env, + input_files=EMPTY_DIRECTORY_DIGEST, output_files=(), ) @@ -219,7 +221,7 @@ def test_blows_up_on_invalid_args(self): with self.assertRaisesRegexp(TypeCheckError, "env"): ExecuteProcessRequest( argv=('1',), - env=dict(), + env=(), input_files='', output_files=(), output_directories=(), @@ -238,7 +240,7 @@ def test_blows_up_on_invalid_args(self): with self.assertRaisesRegexp(TypeCheckError, "output_files"): ExecuteProcessRequest( argv=('1',), - env=tuple(), + env=dict(), input_files=EMPTY_DIRECTORY_DIGEST, output_files=("blah"), output_directories=(), @@ -248,7 +250,7 @@ def test_blows_up_on_invalid_args(self): with self.assertRaisesRegexp(TypeCheckError, "timeout"): ExecuteProcessRequest( argv=('1',), - env=tuple(), + env=dict(), input_files=EMPTY_DIRECTORY_DIGEST, output_files=("blah"), output_directories=(), @@ -257,10 +259,11 @@ def test_blows_up_on_invalid_args(self): ) def test_create_from_snapshot_with_env(self): - req = ExecuteProcessRequest.create_with_empty_snapshot( + req = ExecuteProcessRequest( argv=('foo',), description="Some process", env={'VAR': 'VAL'}, + input_files=EMPTY_DIRECTORY_DIGEST, ) self.assertEqual(req.env, ('VAR', 'VAL')) @@ -294,10 +297,11 @@ def test_javac_version_example(self): self.assertIn('javac', result.value) def test_write_file(self): - request = ExecuteProcessRequest.create_with_empty_snapshot( + request = ExecuteProcessRequest( argv=("/bin/bash", "-c", "echo -n 'European Burmese' > roland"), description="echo roland", - output_files=("roland",) + output_files=("roland",), + input_files=EMPTY_DIRECTORY_DIGEST, ) execute_process_result = self.scheduler.product_request( @@ -328,10 +332,11 @@ def test_exercise_python_side_of_timeout_implementation(self): # but this allows us to ensure that all of the setup # on the python side does not blow up. - request = ExecuteProcessRequest.create_with_empty_snapshot( + request = ExecuteProcessRequest( argv=("/bin/bash", "-c", "/bin/sleep 1; echo -n 'European Burmese'"), timeout_seconds=0.1, description='sleepy-cat', + input_files=EMPTY_DIRECTORY_DIGEST, ) self.scheduler.product_request(ExecuteProcessResult, [request])[0] @@ -383,9 +388,10 @@ class Broken { self.assertIn("NOT VALID JAVA", e.stderr) def test_fallible_failing_command_returns_exited_result(self): - request = ExecuteProcessRequest.create_with_empty_snapshot( + request = ExecuteProcessRequest( argv=("/bin/bash", "-c", "exit 1"), description='one-cat', + input_files=EMPTY_DIRECTORY_DIGEST, ) result = self.scheduler.product_request(FallibleExecuteProcessResult, [request])[0] @@ -393,9 +399,10 @@ def test_fallible_failing_command_returns_exited_result(self): self.assertEqual(result.exit_code, 1) def test_non_fallible_failing_command_raises(self): - request = ExecuteProcessRequest.create_with_empty_snapshot( + request = ExecuteProcessRequest( argv=("/bin/bash", "-c", "exit 1"), description='one-cat', + input_files=EMPTY_DIRECTORY_DIGEST, ) with self.assertRaises(ExecutionError) as cm: From 4de41d956c1a711ad5999d7ed5f2f696d31db115 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 13 Aug 2018 18:45:18 -0700 Subject: [PATCH 2/2] Fix cloc invocation --- src/python/pants/backend/graph_info/tasks/cloc.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/python/pants/backend/graph_info/tasks/cloc.py b/src/python/pants/backend/graph_info/tasks/cloc.py index ee49f8caf64..36b2411a26e 100644 --- a/src/python/pants/backend/graph_info/tasks/cloc.py +++ b/src/python/pants/backend/graph_info/tasks/cloc.py @@ -75,13 +75,10 @@ def console_output(self, targets): # The cloc script reaches into $PATH to look up perl. Let's assume it's in /usr/bin. req = ExecuteProcessRequest( - cmd, - (), - directory_digest, - ('ignored', 'report'), - (), - 15 * 60, - 'cloc' + argv=cmd, + input_files=directory_digest, + output_files=('ignored', 'report'), + description='cloc', ) exec_result = self.context.execute_process_synchronously(req, 'cloc', (WorkUnitLabel.TOOL,))