From 275e8fc8406de2da2be574eee759786f8f75febe Mon Sep 17 00:00:00 2001 From: Jack O'Connor Date: Sun, 20 Jul 2014 00:37:54 -0700 Subject: [PATCH] create the "plugins" field for user-defined plugins Summary: Extend the interfaces in plugins.py to take additional plugin paths, and plumb this all the way up. Make sure plugins are resolved relative to the project root (in runtime.py) and not just blindly assumed to be valid paths. This field shouldn't be able to change the behavior of existing modules, so if the same plugin is ever defined twice, raise an error. The complexity of this diff raises a couple of design issues that we need to think about: - The plugins.py interface is getting very complicated. Should we encapsulate it in some kind of config object? Should it take a `Runtime`? (This would make it difficult to unit test.) - Allowing users to run peru from directories other than the project root is creating complexity throughout the code. The kind of path massaging that we have to do here in runtime.py is very easy to get wrong, and very easy to miss in testing. We really don't want to burden every new feature with a test to make sure it handles funny dirs. Is there a way we could make this kind of testing automatic? Should we use chdir magic to make paths Just Work? (That would make resolving paths in peru.yaml easier, at the expense of resolving paths given on the command line.) Test Plan: Lots of new unit tests and an integration test. Reviewers: sean Differential Revision: https://phabricator.buildinspace.com/D27 --- peru/parser.py | 19 +++++++++- peru/plugin.py | 74 +++++++++++++++++++++++++++------------ peru/remote_module.py | 5 +-- peru/runtime.py | 6 +++- tests/test_integration.py | 25 ++++++++++++- tests/test_parser.py | 71 +++++++++++++++++++++++++------------ tests/test_plugins.py | 51 ++++++++++++++++++++------- 7 files changed, 190 insertions(+), 61 deletions(-) diff --git a/peru/parser.py b/peru/parser.py index 592f23c0..8b082a91 100644 --- a/peru/parser.py +++ b/peru/parser.py @@ -1,3 +1,4 @@ +import collections import os import re import yaml @@ -12,6 +13,10 @@ class ParserError(PrintableError): pass +ParseResult = collections.namedtuple( + "ParseResult", ["scope", "local_module", "plugin_paths"]) + + def parse_file(file_path, **local_module_kwargs): project_root = os.path.dirname(file_path) with open(file_path) as f: @@ -30,10 +35,22 @@ def parse_string(yaml_str, project_root='.', **local_module_kwargs): def _parse_toplevel(blob, **local_module_kwargs): scope = {} + plugin_paths = _extract_plugin_paths(blob) _extract_named_rules(blob, scope) _extract_remote_modules(blob, scope) local_module = _build_local_module(blob, **local_module_kwargs) - return (scope, local_module) + return ParseResult(scope, local_module, plugin_paths) + + +def _extract_plugin_paths(blob): + raw_value = blob.pop("plugins", []) + if isinstance(raw_value, str): + plugin_paths = (raw_value,) + elif isinstance(raw_value, list): + plugin_paths = tuple(raw_value) + else: + raise ParserError("'plugins' field must be a string or a list.") + return plugin_paths def _build_local_module(blob, **local_module_kwargs): diff --git a/peru/plugin.py b/peru/plugin.py index efbc59e5..8fce4c98 100644 --- a/peru/plugin.py +++ b/peru/plugin.py @@ -16,9 +16,11 @@ def plugin_fetch(cwd, plugins_cache_root, type, dest, plugin_fields, *, - capture_output=False, stderr_to_stdout=False): + capture_output=False, stderr_to_stdout=False, + plugin_roots=()): cache_path = _plugin_cache_path(plugins_cache_root, type) - command = _plugin_command(type, 'fetch', plugin_fields, dest, cache_path) + command = _plugin_command(type, 'fetch', plugin_roots, plugin_fields, + dest, cache_path) kwargs = {"stderr": subprocess.STDOUT} if stderr_to_stdout else {} if capture_output: @@ -28,9 +30,11 @@ def plugin_fetch(cwd, plugins_cache_root, type, dest, plugin_fields, *, subprocess.check_call(command, cwd=cwd, **kwargs) -def plugin_get_reup_fields(cwd, plugins_cache_root, type, plugin_fields): +def plugin_get_reup_fields(cwd, plugins_cache_root, type, plugin_fields, *, + plugin_roots=()): cache_path = _plugin_cache_path(plugins_cache_root, type) - command = _plugin_command(type, 'reup', plugin_fields, cache_path) + command = _plugin_command(type, 'reup', plugin_roots, plugin_fields, + cache_path) output = subprocess.check_output(command, cwd=cwd).decode('utf8') new_fields = yaml.safe_load(output) or {} for key, val in new_fields.items(): @@ -39,8 +43,8 @@ def plugin_get_reup_fields(cwd, plugins_cache_root, type, plugin_fields): return new_fields -def _plugin_command(type, subcommand, plugin_fields, *args): - path = _plugin_exe_path(type, subcommand) +def _plugin_command(type, subcommand, plugin_roots, plugin_fields, *args): + path = _plugin_exe_path(type, subcommand, plugin_roots) assert os.access(path, os.X_OK), type + " plugin isn't executable." assert "--" not in plugin_fields, "-- is not a valid field name" @@ -61,31 +65,57 @@ def _plugin_cache_path(plugins_cache_root, type): return plugin_cache -def _plugin_exe_path(type, subcommand): - # Scan for a corresponding script dir. - root = os.path.join(PLUGINS_DIR, type) - if not os.path.isdir(root): - raise PrintableError( - 'no root directory found for plugin `{}`'.format(type)) +def _plugin_exe_path(type, subcommand, plugin_roots): + root = _find_plugin(type, plugin_roots) - # Scan for files in the script dir. - # The subcommand is used as a prefix, not an exact match, to support - # extensions. - # To support certain platforms, an extension is necessary for execution as - # a subprocess. - # Most notably, this is required to support Windows. + # Scan for files in the script dir. The subcommand is used as a prefix, not + # an exact match, to support extensions. To support certain platforms, an + # extension is necessary for execution as a subprocess. Most notably, this + # is required to support Windows. matches = [match for match in os.listdir(root) if match.startswith(subcommand) and os.path.isfile(os.path.join(root, match))] - # Ensure there is only one match. - # It is possible for multiple files to share the subcommand prefix. + # Ensure there is only one match. It is possible for multiple files to + # share the subcommand prefix. if not(matches): - raise PrintableError( + raise PluginCommandMissingError( 'no candidate for command `{}`'.format(subcommand)) if len(matches) > 1: # Barf if there is more than one candidate. - raise PrintableError( + raise MultiplePluginCommandsError( 'more than one candidate for command `{}`'.format(subcommand)) return os.path.join(root, matches[0]) + + +def _find_plugin(type, plugin_roots): + roots = [PLUGINS_DIR] + list(plugin_roots) + options = [os.path.join(root, type) for root in roots] + matches = [option for option in options if os.path.isdir(option)] + if not matches: + raise PluginMissingError( + 'No plugin "{}" found at any of the following roots:\n' + .format(type) + + '\n'.join(roots)) + if len(matches) > 1: + raise MultiplePluginsError( + 'Multiple plugins found of type "{}":\n'.format(type) + + '\n'.join(matches)) + return matches[0] + + +class PluginMissingError(PrintableError): + pass + + +class MultiplePluginsError(PrintableError): + pass + + +class PluginCommandMissingError(PrintableError): + pass + + +class MultiplePluginCommandsError(PrintableError): + pass diff --git a/peru/remote_module.py b/peru/remote_module.py index 330963f3..e22d0d09 100644 --- a/peru/remote_module.py +++ b/peru/remote_module.py @@ -34,7 +34,8 @@ def get_tree(self, runtime): return runtime.cache.keyval[key] with runtime.tmp_dir() as tmp_dir: plugin_fetch(runtime.root, runtime.cache.plugins_root, - self.type, tmp_dir, self.plugin_fields) + self.type, tmp_dir, self.plugin_fields, + plugin_roots=runtime.plugin_roots) base_tree = runtime.cache.import_tree(tmp_dir) tree = resolver.merge_import_trees( runtime, self.imports, base_tree) @@ -46,7 +47,7 @@ def reup(self, runtime): print("reup", self.name) reup_fields = plugin_get_reup_fields( runtime.root, runtime.cache.plugins_root, self.type, - self.plugin_fields) + self.plugin_fields, plugin_roots=runtime.plugin_roots) for field, val in reup_fields.items(): if (field not in self.plugin_fields or val != self.plugin_fields[field]): diff --git a/peru/runtime.py b/peru/runtime.py index 21c3ebff..5f7bc602 100644 --- a/peru/runtime.py +++ b/peru/runtime.py @@ -19,8 +19,12 @@ def __init__(self, args, env): 'PERU_DIR', os.path.join(self.root, '.peru')) compat.makedirs(self.peru_dir) - self.scope, self.local_module = parser.parse_file( + parse_result = parser.parse_file( self.peru_file, peru_dir=self.peru_dir) + self.scope = parse_result.scope + self.local_module = parse_result.local_module + self.plugin_roots = tuple(os.path.join(self.root, path) + for path in parse_result.plugin_paths) cache_dir = env.get('PERU_CACHE', os.path.join(self.peru_dir, 'cache')) self.cache = cache.Cache(cache_dir) diff --git a/tests/test_integration.py b/tests/test_integration.py index b9b7dfde..618a82a1 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -1,5 +1,6 @@ import io import os +import shutil import sys from textwrap import dedent import unittest @@ -11,7 +12,8 @@ import shared -peru_bin = os.path.join(os.path.dirname(__file__), "..", "..", "peru.sh") +PERU_MODULE_ROOT = os.path.abspath( + os.path.join(os.path.dirname(peru.__file__))) def run_peru_command(args, test_dir, peru_dir, *, env_vars=None, @@ -208,6 +210,27 @@ def test_local_build(self): "fi": "feefee", }) + def test_local_plugins(self): + cp_plugin_path = os.path.join( + PERU_MODULE_ROOT, 'resources', 'plugins', 'cp') + shutil.copytree(cp_plugin_path, + os.path.join(self.test_dir, 'myplugins', 'newfangled')) + # Grab the contents now so that we can match it later. + # TODO: Rethink how these tests are structured. + expected_content = shared.read_dir(self.test_dir) + expected_content['foo'] = 'bar' + + self.write_peru_yaml('''\ + imports: + foo: ./ + + plugins: myplugins/ + + newfangled module foo: + path: {} + ''') + self.do_integration_test(['sync'], expected_content) + def test_alternate_cache(self): self.write_peru_yaml("""\ cp module foo: diff --git a/tests/test_parser.py b/tests/test_parser.py index 4d18e023..4c6d65eb 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -9,15 +9,42 @@ class ParserTest(unittest.TestCase): def test_parse_empty_file(self): - scope, local_module = parse_string('') - self.assertDictEqual(scope, {}) - self.assertDictEqual(local_module.imports, {}) - self.assertEqual(local_module.default_rule, None) - self.assertEqual(local_module.root, '.') + result = parse_string('') + self.assertDictEqual(result.scope, {}) + self.assertDictEqual(result.local_module.imports, {}) + self.assertEqual(result.local_module.default_rule, None) + self.assertEqual(result.local_module.root, '.') + self.assertTupleEqual(result.plugin_paths, ()) def test_parse_with_project_root(self): - scope, local_module = parse_string('', project_root='foo/bar') - self.assertEqual(local_module.root, 'foo/bar') + result = parse_string('', project_root='foo/bar') + self.assertEqual(result.local_module.root, 'foo/bar') + + def test_parse_with_plugin_paths(self): + result = parse_string(dedent('''\ + plugins: foo + ''')) + self.assertTupleEqual(('foo',), result.plugin_paths) + + def test_parse_with_list_of_plugin_paths(self): + result = parse_string(dedent('''\ + plugins: + - foo + - bar + ''')) + self.assertTupleEqual(('foo', 'bar'), result.plugin_paths) + + def test_parse_with_bad_plugin_paths(self): + wrong_type = dedent('''\ + plugins: 5 + ''') + with self.assertRaises(ParserError): + parse_string(wrong_type) + empty_val = dedent('''\ + plugins: + ''') + with self.assertRaises(ParserError): + parse_string(empty_val) def test_parse_rule(self): input = dedent("""\ @@ -25,9 +52,9 @@ def test_parse_rule(self): build: echo hi export: out/ """) - scope, local_module = parse_string(input) - self.assertIn("foo", scope) - rule = scope["foo"] + result = parse_string(input) + self.assertIn("foo", result.scope) + rule = result.scope["foo"] self.assertIsInstance(rule, Rule) self.assertEqual(rule.name, "foo") self.assertEqual(rule.build_command, "echo hi") @@ -42,9 +69,9 @@ def test_parse_module(self): wham: bam/ thank: you/maam """) - scope, local_module = parse_string(input) - self.assertIn("foo", scope) - module = scope["foo"] + result = parse_string(input) + self.assertIn("foo", result.scope) + module = result.scope["foo"] self.assertIsInstance(module, RemoteModule) self.assertEqual(module.name, "foo") self.assertEqual(module.type, "sometype") @@ -61,9 +88,9 @@ def test_parse_module_default_rule(self): build: foo export: bar """) - scope, local_module = parse_string(input) - self.assertIn("bar", scope) - module = scope["bar"] + result = parse_string(input) + self.assertIn("bar", result.scope) + module = result.scope["bar"] self.assertIsInstance(module, RemoteModule) self.assertIsInstance(module.default_rule, Rule) self.assertEqual(module.default_rule.build_command, "foo") @@ -74,17 +101,17 @@ def test_parse_toplevel_imports(self): imports: foo: bar/ """) - scope, local_module = parse_string(input) - self.assertDictEqual(scope, {}) - self.assertDictEqual(local_module.imports, {"foo": "bar/"}) + result = parse_string(input) + self.assertDictEqual(result.scope, {}) + self.assertDictEqual(result.local_module.imports, {"foo": "bar/"}) def test_parse_empty_imports(self): input = dedent('''\ imports: ''') - scope, local_module = parse_string(input) - self.assertDictEqual(scope, {}) - self.assertDictEqual(local_module.imports, {}) + result = parse_string(input) + self.assertDictEqual(result.scope, {}) + self.assertDictEqual(result.local_module.imports, {}) def test_bad_toplevel_field_throw(self): with self.assertRaises(ParserError): diff --git a/tests/test_plugins.py b/tests/test_plugins.py index e3eb6ba2..924e54bd 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -3,7 +3,7 @@ import subprocess import unittest -from peru.plugin import plugin_fetch, plugin_get_reup_fields +import peru.plugin as plugin import shared from shared import GitRepo, HgRepo @@ -18,9 +18,9 @@ def setUp(self): def do_plugin_test(self, type, plugin_fields, expected_content, *, hide_stderr=False): fetch_dir = shared.create_dir() - output = plugin_fetch('.', self.cache_root, type, fetch_dir, - plugin_fields, capture_output=True, - stderr_to_stdout=hide_stderr) + output = plugin.plugin_fetch('.', self.cache_root, type, fetch_dir, + plugin_fields, capture_output=True, + stderr_to_stdout=hide_stderr) self.assertDictEqual(shared.read_dir(fetch_dir), expected_content, msg="Fetched content did not match expected.") return output @@ -95,7 +95,7 @@ def test_git_plugin_reup(self): plugin_fields = {"url": self.content_dir} # By default, the git plugin should reup from master. expected_output = {"rev": master_head} - output = plugin_get_reup_fields( + output = plugin.plugin_get_reup_fields( '.', self.cache_root, "git", plugin_fields) self.assertDictEqual(expected_output, output) # Add some new commits and make sure master gets fetched properly. @@ -104,14 +104,14 @@ def test_git_plugin_reup(self): repo.run("git commit --allow-empty -m 'more junk'") new_master_head = repo.run("git rev-parse master") expected_output["rev"] = new_master_head - output = plugin_get_reup_fields( + output = plugin.plugin_get_reup_fields( '.', self.cache_root, "git", plugin_fields) self.assertDictEqual(expected_output, output) # Now specify the reup target explicitly. newbranch_head = repo.run("git rev-parse newbranch") plugin_fields["reup"] = "newbranch" expected_output["rev"] = newbranch_head - output = plugin_get_reup_fields( + output = plugin.plugin_get_reup_fields( '.', self.cache_root, "git", plugin_fields) self.assertDictEqual(expected_output, output) @@ -121,7 +121,7 @@ def test_hg_plugin_reup(self): plugin_fields = {"url": self.content_dir} # By default, the hg plugin should reup from default. expected_output = {"rev": default_tip} - output = plugin_get_reup_fields( + output = plugin.plugin_get_reup_fields( '.', self.cache_root, "hg", plugin_fields) self.assertDictEqual(expected_output, output) # Add some new commits and make sure master gets fetched properly. @@ -134,14 +134,14 @@ def test_hg_plugin_reup(self): repo.run("hg commit -A -m 'more junk'") new_default_tip = repo.run("hg identify --debug -r default").split()[0] expected_output["rev"] = new_default_tip - output = plugin_get_reup_fields( + output = plugin.plugin_get_reup_fields( '.', self.cache_root, "hg", plugin_fields) self.assertDictEqual(expected_output, output) # Now specify the reup target explicitly. newbranch_tip = repo.run("hg identify --debug -r tip").split()[0] plugin_fields["reup"] = "newbranch" expected_output["rev"] = newbranch_tip - output = plugin_get_reup_fields( + output = plugin.plugin_get_reup_fields( '.', self.cache_root, "hg", plugin_fields) self.assertDictEqual(expected_output, output) @@ -171,11 +171,11 @@ def test_curl_plugin_reup(self): digest.update(b'content') real_hash = digest.hexdigest() fields = {'url': test_url} - output = plugin_get_reup_fields('.', '.', 'curl', fields) + output = plugin.plugin_get_reup_fields('.', '.', 'curl', fields) self.assertDictEqual({'sha1': real_hash}, output) # Confirm that we get the same thing with a preexisting hash. fields['sha1'] = 'preexisting junk' - output = plugin_get_reup_fields('.', '.', 'curl', fields) + output = plugin.plugin_get_reup_fields('.', '.', 'curl', fields) self.assertDictEqual({'sha1': real_hash}, output) def test_cp_plugin(self): @@ -206,3 +206,30 @@ def test_rsync_plugin_bad_fields(self): def test_empty_plugin(self): self.do_plugin_test("empty", {}, {}) + + def test_plugin_roots(self): + plugins_dir = shared.create_dir({ + 'footype/fetch.py': + '#! /usr/bin/env python3\nprint("hey there!")\n', + 'footype/reup.py': + '#! /usr/bin/env python3\nprint("name: val")\n'}) + os.chmod(os.path.join(plugins_dir, 'footype', 'fetch.py'), 0o755) + os.chmod(os.path.join(plugins_dir, 'footype', 'reup.py'), 0o755) + output = plugin.plugin_fetch( + '.', '.', 'footype', '.', {}, plugin_roots=(plugins_dir,), + capture_output=True) + self.assertEqual('hey there!\n', output) + output = plugin.plugin_get_reup_fields('.', '.', 'footype', {}, + plugin_roots=(plugins_dir,)) + self.assertDictEqual({'name': 'val'}, output) + + def test_no_such_plugin(self): + with self.assertRaises(plugin.PluginMissingError): + plugin.plugin_fetch('.', '.', 'nosuchtype!', '.', {}) + + def test_multiple_plugin_definitions(self): + path1 = shared.create_dir({'footype/junk': 'junk'}) + path2 = shared.create_dir({'footype/junk': 'junk'}) + with self.assertRaises(plugin.MultiplePluginsError): + plugin.plugin_fetch('.', '.', 'footype', '.', {}, + plugin_roots=(path1, path2))