From 56a8892364c45c2babc8e160d88caef092145150 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Thu, 9 Jan 2020 13:14:35 -0700 Subject: [PATCH] Deprecate `--positional-arg-file` in favor of `--spec-file` (#8928) In preparation for non-target specs, we had renamed `--target-spec-file` to `--positional-arg-file`. However, in https://github.com/pantsbuild/pants/pull/8916, we decided to use the name `Spec` to describe a general _specification_ for what Pants should run against, rather than `PositionalArg`. This updates the option and all relevant code to use the newly preferred name `spec`. We also explain in the deprecation warnings why this change is happening so that users don't think we're introducing all this churn for no good reason. --- .../project_info/tasks/idea_plugin_gen.py | 6 +- src/python/pants/bin/local_pants_runner.py | 2 +- .../pants/init/target_roots_calculator.py | 2 +- src/python/pants/option/arg_splitter.py | 20 ++--- src/python/pants/option/arg_splitter_test.py | 80 +++++++++---------- src/python/pants/option/global_options.py | 17 +++- src/python/pants/option/options.py | 46 ++++++++--- src/python/pants/option/options_test.py | 10 +-- .../pants/testutil/console_rule_test_base.py | 2 +- 9 files changed, 106 insertions(+), 79 deletions(-) diff --git a/src/python/pants/backend/project_info/tasks/idea_plugin_gen.py b/src/python/pants/backend/project_info/tasks/idea_plugin_gen.py index 890fad92932..454eef314ed 100644 --- a/src/python/pants/backend/project_info/tasks/idea_plugin_gen.py +++ b/src/python/pants/backend/project_info/tasks/idea_plugin_gen.py @@ -99,7 +99,7 @@ def __init__(self, *args, **kwargs): safe_mkdir(output_dir) with temporary_dir(root_dir=output_dir, cleanup=False) as output_project_dir: - project_name = self.get_project_name(self.context.options.positional_args) + project_name = self.get_project_name(self.context.options.specs) self.gen_project_workdir = output_project_dir self.project_filename = os.path.join(self.gen_project_workdir, @@ -135,7 +135,7 @@ def generate_project(self): debug_port=self.get_options().debug_port, ) - abs_target_specs = [os.path.join(get_buildroot(), spec) for spec in self.context.options.positional_args] + abs_target_specs = [os.path.join(get_buildroot(), spec) for spec in self.context.options.specs] configured_workspace = TemplateData( targets=json.dumps(abs_target_specs), project_path=os.path.join(get_buildroot(), abs_target_specs[0].split(':')[0]), @@ -170,7 +170,7 @@ def _generate_to_tempfile(self, generator): return output.name def console_output(self, _targets): - if not self.context.options.positional_args: + if not self.context.options.specs: raise TaskError("No targets specified.") # Heuristics to guess whether user tries to load a python project, diff --git a/src/python/pants/bin/local_pants_runner.py b/src/python/pants/bin/local_pants_runner.py index 61b3640108b..d00c7f72fa7 100644 --- a/src/python/pants/bin/local_pants_runner.py +++ b/src/python/pants/bin/local_pants_runner.py @@ -237,7 +237,7 @@ def set_start_time(self, start_time): spec_parser = CmdLineSpecParser(get_buildroot()) target_specs = [ spec_parser.parse_address_spec(spec).to_spec_string() - for spec in self._options.positional_args + for spec in self._options.specs ] # Note: This will not include values from `--owner-of` or `--changed-*` flags. self._run_tracker.run_info.add_info("specs_from_command_line", target_specs, stringify=False) diff --git a/src/python/pants/init/target_roots_calculator.py b/src/python/pants/init/target_roots_calculator.py index edf45a420b4..c2cf63d9ecd 100644 --- a/src/python/pants/init/target_roots_calculator.py +++ b/src/python/pants/init/target_roots_calculator.py @@ -63,7 +63,7 @@ def create( ) -> TargetRoots: # Determine the literal target roots. address_spec_roots = cls.parse_address_specs( - target_specs=options.positional_args, + target_specs=options.specs, build_root=build_root, exclude_patterns=exclude_patterns, tags=tags) diff --git a/src/python/pants/option/arg_splitter.py b/src/python/pants/option/arg_splitter.py index 45636479c5d..dee461a6c46 100644 --- a/src/python/pants/option/arg_splitter.py +++ b/src/python/pants/option/arg_splitter.py @@ -21,7 +21,7 @@ class SplitArgs: """The result of splitting args.""" goals: List[str] # Explicitly requested goals. scope_to_flags: Dict[str, List[str]] # Scope name -> list of flags in that scope. - positional_args: List[str] # The positional args for the goals. + specs: List[str] # The specifications for what to run against, e.g. the targets or files passthru: List[str] # Any remaining args specified after a -- separator. passthru_owner: Optional[str] # The scope specified last on the command line, if any. unknown_scopes: List[str] @@ -64,7 +64,7 @@ class NoGoalHelp(HelpRequest): class ArgSplitter: - """Splits a command-line into scoped sets of flags, and a set of positional args. + """Splits a command-line into scoped sets of flags and a set of specs. Recognizes, e.g.: @@ -142,7 +142,7 @@ def add_scope(s: str) -> None: if s not in scope_to_flags: scope_to_flags[s] = [] - positional_args = [] + specs = [] passthru = [] passthru_owner = None @@ -179,8 +179,8 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None: # We assume any args here are in global scope. if not self._check_for_help_request(arg): assign_flag_to_scope(arg, GLOBAL_SCOPE) - elif self.is_positional_arg(arg): - positional_args.append(arg) + elif self.spec(arg): + specs.append(arg) elif arg not in self._known_scopes: self._unknown_scopes.append(arg) @@ -194,15 +194,15 @@ def assign_flag_to_scope(flg: str, default_scope: str) -> None: if not goals and not self._help_request: self._help_request = NoGoalHelp() - return SplitArgs(list(goals), scope_to_flags, positional_args, passthru, + return SplitArgs(list(goals), scope_to_flags, specs, passthru, passthru_owner if passthru else None, self._unknown_scopes) @staticmethod - def is_positional_arg(arg: str) -> bool: - """Does arg 'look like' a positional arg (rather than a goal name). + def spec(arg: str) -> bool: + """Does arg 'look like' a Spec (rather than a goal name). - An arg is a positional arg if it looks like a target spec or a path. - In the future we can expand this heuristic to support other kinds of positional args. + An arg is a spec if it looks like an AddressSpec or a FilesystemSpec. + In the future we can expand this heuristic to support other kinds of specs, such as URLs. """ return os.path.sep in arg or ':' in arg or os.path.isdir(arg) diff --git a/src/python/pants/option/arg_splitter_test.py b/src/python/pants/option/arg_splitter_test.py index e964e5422f9..1404b81457c 100644 --- a/src/python/pants/option/arg_splitter_test.py +++ b/src/python/pants/option/arg_splitter_test.py @@ -41,7 +41,7 @@ def assert_valid_split( *, expected_goals: List[str], expected_scope_to_flags: Dict[str, List[str]], - expected_positional_args: List[str], + expected_specs: List[str], expected_passthru: Optional[List[str]] = None, expected_passthru_owner: Optional[str] = None, expected_is_help: bool = False, @@ -56,7 +56,7 @@ def assert_valid_split( split_args = splitter.split_args(args) self.assertEqual(expected_goals, split_args.goals) self.assertEqual(expected_scope_to_flags, split_args.scope_to_flags) - self.assertEqual(expected_positional_args, split_args.positional_args) + self.assertEqual(expected_specs, split_args.specs) self.assertEqual(expected_passthru, split_args.passthru) self.assertEqual(expected_passthru_owner, split_args.passthru_owner) self.assertEqual(expected_is_help, splitter.help_request is not None) @@ -68,23 +68,23 @@ def assert_valid_split( splitter.help_request.all_scopes)) self.assertEqual(expected_unknown_scopes, split_args.unknown_scopes) - def test_is_positional_arg(self) -> None: - def assert_positional(arg: str) -> None: - self.assertTrue(ArgSplitter.is_positional_arg(arg)) + def test_is_spec(self) -> None: + def assert_spec(arg: str) -> None: + self.assertTrue(ArgSplitter.spec(arg)) - def assert_not_positional(arg: str) -> None: - self.assertFalse(ArgSplitter.is_positional_arg(arg)) + def assert_not_spec(arg: str) -> None: + self.assertFalse(ArgSplitter.spec(arg)) - assert_positional('a/b/c') - assert_positional('a/b:c') - assert_positional(':c') + assert_spec('a/b/c') + assert_spec('a/b:c') + assert_spec(':c') with temporary_dir() as tmpdir: os.mkdir(os.path.join(tmpdir, 'foo')) with pushd(tmpdir): - assert_positional('foo') + assert_spec('foo') - assert_not_positional('foo') - assert_not_positional('a_b_c') + assert_not_spec('foo') + assert_not_spec('a_b_c') def test_basic_arg_splitting(self) -> None: # Various flag combos. @@ -98,7 +98,7 @@ def test_basic_arg_splitting(self) -> None: 'compile': ['-g'], 'test.junit': ['-i'] }, - expected_positional_args=['src/java/org/pantsbuild/foo', 'src/java/org/pantsbuild/bar:baz'], + expected_specs=['src/java/org/pantsbuild/foo', 'src/java/org/pantsbuild/bar:baz'], ) self.assert_valid_split( './pants -farg --fff=arg compile --gg-gg=arg-arg -g test.junit --iii ' @@ -110,45 +110,45 @@ def test_basic_arg_splitting(self) -> None: 'test.junit': ['--iii'], 'compile.java': ['--long-flag'], }, - expected_positional_args=['src/java/org/pantsbuild/foo', 'src/java/org/pantsbuild/bar:baz'], + expected_specs=['src/java/org/pantsbuild/foo', 'src/java/org/pantsbuild/bar:baz'], ) - def test_distinguish_goals_from_positional_args(self) -> None: + def test_distinguish_goals_from_specs(self) -> None: self.assert_valid_split( './pants compile test foo::', expected_goals=['compile', 'test'], expected_scope_to_flags={'': [], 'compile': [], 'test': []}, - expected_positional_args=['foo::'], + expected_specs=['foo::'], ) self.assert_valid_split( './pants compile test foo::', expected_goals=['compile', 'test'], expected_scope_to_flags={'': [], 'compile': [], 'test': []}, - expected_positional_args=['foo::'], + expected_specs=['foo::'], ) self.assert_valid_split( './pants compile test:test', expected_goals=['compile'], expected_scope_to_flags={'': [], 'compile': []}, - expected_positional_args=['test:test'], + expected_specs=['test:test'], ) self.assert_valid_split( './pants test test:test', expected_goals=['test'], expected_scope_to_flags={'': [], 'test': []}, - expected_positional_args=['test:test'], + expected_specs=['test:test'], ) self.assert_valid_split( './pants test ./test', expected_goals=['test'], expected_scope_to_flags={'': [], 'test': []}, - expected_positional_args=['./test'], + expected_specs=['./test'], ) self.assert_valid_split( './pants test //test', expected_goals=['test'], expected_scope_to_flags={'': [], 'test': []}, - expected_positional_args=['//test'], + expected_specs=['//test'], ) def test_descoping_qualified_flags(self) -> None: @@ -158,14 +158,14 @@ def test_descoping_qualified_flags(self) -> None: expected_scope_to_flags={ '': [], 'compile': [], 'compile.java': ['--bar'], 'test': [], 'test.junit': ['--no-baz'] }, - expected_positional_args=['foo/bar'], + expected_specs=['foo/bar'], ) # Qualified flags don't count as explicit goals. self.assert_valid_split( './pants compile --test-junit-bar foo/bar', expected_goals=['compile'], expected_scope_to_flags={'': [], 'compile': [], 'test.junit': ['--bar']}, - expected_positional_args=['foo/bar'], + expected_specs=['foo/bar'], ) def test_passthru_args(self) -> None: @@ -173,7 +173,7 @@ def test_passthru_args(self) -> None: './pants test foo/bar -- -t arg', expected_goals=['test'], expected_scope_to_flags={'': [], 'test': []}, - expected_positional_args=['foo/bar'], + expected_specs=['foo/bar'], expected_passthru=['-t', 'arg'], expected_passthru_owner='test', ) @@ -188,7 +188,7 @@ def test_passthru_args(self) -> None: 'compile.java': ['--long-flag'], 'test.junit': ['--iii'] }, - expected_positional_args=['src/java/org/pantsbuild/foo', 'src/java/org/pantsbuild/bar:baz'], + expected_specs=['src/java/org/pantsbuild/foo', 'src/java/org/pantsbuild/bar:baz'], expected_passthru=['passthru1', 'passthru2'], expected_passthru_owner='test.junit' ) @@ -199,14 +199,14 @@ def test_subsystem_flags(self) -> None: './pants --jvm-options=-Dbar=baz test foo:bar', expected_goals=['test'], expected_scope_to_flags={'': [], 'jvm': ['--options=-Dbar=baz'], 'test': []}, - expected_positional_args=['foo:bar'] + expected_specs=['foo:bar'] ) # Qualified task subsystem flag in global scope. self.assert_valid_split( './pants --jvm-test-junit-options=-Dbar=baz test foo:bar', expected_goals=['test'], expected_scope_to_flags={'': [], 'jvm.test.junit': ['--options=-Dbar=baz'], 'test': []}, - expected_positional_args=['foo:bar'] + expected_specs=['foo:bar'] ) # Unqualified task subsystem flag in task scope. # Note that this exposes a small problem: You can't set an option on the cmd-line if that @@ -217,13 +217,13 @@ def test_subsystem_flags(self) -> None: './pants test.junit --jvm-options=-Dbar=baz foo:bar', expected_goals=['test'], expected_scope_to_flags={'': [], 'jvm.test.junit': ['--options=-Dbar=baz'], 'test.junit': []}, - expected_positional_args=['foo:bar']) + expected_specs=['foo:bar']) # Global-only flag in task scope. self.assert_valid_split( './pants test.junit --reporting-template-dir=path foo:bar', expected_goals=['test'], expected_scope_to_flags={'': [], 'reporting': ['--template-dir=path'], 'test.junit': []}, - expected_positional_args=['foo:bar'] + expected_specs=['foo:bar'] ) def test_help_detection(self) -> None: @@ -231,7 +231,7 @@ def test_help_detection(self) -> None: self.assert_valid_split, expected_passthru=None, expected_passthru_owner=None, expected_is_help=True, ) assert_help_no_arguments = partial( - assert_help, expected_goals=[], expected_scope_to_flags={'': []}, expected_positional_args=[], + assert_help, expected_goals=[], expected_scope_to_flags={'': []}, expected_specs=[], ) assert_help_no_arguments('./pants') assert_help_no_arguments('./pants help') @@ -258,58 +258,58 @@ def test_help_detection(self) -> None: './pants -f', expected_goals=[], expected_scope_to_flags={'': ['-f']}, - expected_positional_args=[], + expected_specs=[], ) assert_help( './pants help compile -x', expected_goals=['compile'], expected_scope_to_flags={'': [], 'compile': ['-x']}, - expected_positional_args=[], + expected_specs=[], ) assert_help( './pants help compile -x', expected_goals=['compile'], expected_scope_to_flags={'': [], 'compile': ['-x']}, - expected_positional_args=[], + expected_specs=[], ) assert_help( './pants compile -h', expected_goals=['compile'], expected_scope_to_flags={'': [], 'compile': []}, - expected_positional_args=[], + expected_specs=[], ) assert_help( './pants compile --help test', expected_goals=['compile', 'test'], expected_scope_to_flags={'': [], 'compile': [], 'test': []}, - expected_positional_args=[], + expected_specs=[], ) assert_help( './pants test src/foo/bar:baz -h', expected_goals=['test'], expected_scope_to_flags={'': [], 'test': []}, - expected_positional_args=['src/foo/bar:baz'], + expected_specs=['src/foo/bar:baz'], ) assert_help( './pants compile --help-advanced test', expected_goals=['compile', 'test'], expected_scope_to_flags={'': [], 'compile': [], 'test': []}, - expected_positional_args=[], + expected_specs=[], expected_help_advanced=True, ) assert_help( './pants help-advanced compile', expected_goals=['compile'], expected_scope_to_flags={'': [], 'compile': []}, - expected_positional_args=[], + expected_specs=[], expected_help_advanced=True, ) assert_help( './pants compile help-all test --help', expected_goals=['compile', 'test'], expected_scope_to_flags={'': [], 'compile': [], 'test': []}, - expected_positional_args=[], + expected_specs=[], expected_help_all=True, ) diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 197758728a5..bbe71e3c5a4 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -253,12 +253,21 @@ def register_bootstrap_options(cls, register): 'Later files override earlier ones.') register('--pythonpath', advanced=True, type=list, help='Add these directories to PYTHONPATH to search for plugins.') - register('--target-spec-file', type=list, dest='positional_arg_files', daemon=False, - removal_version='1.25.0.dev2', - removal_hint='Use --positional-arg-file instead', + register('--target-spec-file', type=list, dest='spec_files', daemon=False, + removal_version='1.27.0.dev0', + removal_hint='Use `--spec-file` instead. This change is in preparation for Pants ' + 'eventually allowing you to pass file names as arguments, e.g. ' + '`./pants cloc foo.py`.', help='Read additional specs from this file, one per line') - register('--positional-arg-file', type=list, dest='positional_arg_files', daemon=False, + register('--positional-arg-file', type=list, dest='spec_files', daemon=False, + removal_version='1.27.0.dev0', + removal_hint='Use `--spec-file` instead. This change is in preparation for Pants ' + 'eventually allowing you to pass file names as arguments, e.g. ' + '`./pants cloc foo.py`.', help='Read additional positional args from this file, one per line') + register('--spec-file', type=list, dest='spec_files', daemon=False, + help='Read additional specs from this file (e.g. target addresses or file names). ' + 'Each spec should be one per line.') register('--verify-config', type=bool, default=True, daemon=False, advanced=True, help='Verify that all config file values correspond to known options.') diff --git a/src/python/pants/option/options.py b/src/python/pants/option/options.py index 8d9da1d0e4e..dc853fe2db9 100644 --- a/src/python/pants/option/options.py +++ b/src/python/pants/option/options.py @@ -5,6 +5,7 @@ import re import sys from dataclasses import dataclass +from typing import List from pants.base.deprecated import warn_or_error from pants.option.arg_splitter import ArgSplitter @@ -132,32 +133,32 @@ def create(cls, env, config, known_scope_infos, args=None, bootstrap_option_valu option_tracker = OptionTracker() if bootstrap_option_values: - positional_arg_files = bootstrap_option_values.positional_arg_files - if positional_arg_files: - for positional_arg_file in positional_arg_files: - with open(positional_arg_file, 'r') as f: - split_args.positional_args.extend([line for line in [line.strip() for line in f] if line]) + spec_files = bootstrap_option_values.spec_files + if spec_files: + for spec_file in spec_files: + with open(spec_file, 'r') as f: + split_args.specs.extend([line for line in [line.strip() for line in f] if line]) help_request = splitter.help_request parser_hierarchy = ParserHierarchy(env, config, complete_known_scope_infos, option_tracker) bootstrap_option_values = bootstrap_option_values known_scope_to_info = {s.scope: s for s in complete_known_scope_infos} - return cls(split_args.goals, split_args.scope_to_flags, split_args.positional_args, + return cls(split_args.goals, split_args.scope_to_flags, split_args.specs, split_args.passthru, split_args.passthru_owner, help_request, parser_hierarchy, bootstrap_option_values, known_scope_to_info, option_tracker, split_args.unknown_scopes) - def __init__(self, goals, scope_to_flags, positional_args, passthru, passthru_owner, help_request, + def __init__(self, goals, scope_to_flags, specs: List[str], passthru, passthru_owner, help_request, parser_hierarchy, bootstrap_option_values, known_scope_to_info, - option_tracker, unknown_scopes): + option_tracker, unknown_scopes) -> None: """The low-level constructor for an Options instance. Dependees should use `Options.create` instead. """ self._goals = goals self._scope_to_flags = scope_to_flags - self._positional_args = positional_args + self._specs = specs self._passthru = passthru self._passthru_owner = passthru_owner self._help_request = help_request @@ -185,14 +186,27 @@ def help_request(self): """ return self._help_request + @property + def specs(self) -> List[str]: + """The specifications to operate on, e.g. the target addresses and the file names. + + :API: public + """ + return self._specs + @property def target_specs(self): """The targets to operate on. :API: public """ - warn_or_error('1.25.0.dev2', '.target_specs', 'Use .positional_args instead') - return self._positional_args + warn_or_error( + removal_version='1.27.0.dev0', + deprecated_entity_description='.target_specs', + hint='Use .specs instead. This change is in preparation for Pants eventually allowing you to ' + 'pass file names as arguments, e.g. `./pants cloc foo.py`.', + ) + return self._specs @property def positional_args(self): @@ -200,7 +214,13 @@ def positional_args(self): :API: public """ - return self._positional_args + warn_or_error( + removal_version='1.27.0.dev0', + deprecated_entity_description='.positional_args', + hint='Use .specs instead. This change is in preparation for Pants eventually allowing you to ' + 'pass file names as arguments, e.g. `./pants cloc foo.py`.', + ) + return self._specs @property def goals(self): @@ -256,7 +276,7 @@ def drop_flag_values(self): no_flags = {} return Options(self._goals, no_flags, - self._positional_args, + self._specs, self._passthru, self._passthru_owner, self._help_request, diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index 3b092202f40..8403404ea95 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -257,7 +257,7 @@ def test_arg_scoping(self) -> None: options = self._parse(flags='--verbose') self.assertEqual(True, options.for_global_scope().verbose) options = self._parse(flags='-z compile path/to/tgt') - self.assertEqual(['path/to/tgt'], options.positional_args) + self.assertEqual(['path/to/tgt'], options.specs) self.assertEqual(True, options.for_global_scope().verbose) with self.assertRaises(ParseError): @@ -594,9 +594,7 @@ def check( ) -> None: env = {"PANTS_GLOBAL_SHELL_STR_LISTY": env_val} if env_val else None config = {"GLOBAL": {"shell_str_listy": config_val}} if config_val else None - global_options = self._parse( - args_str=f"./pants {flags}", env=env, config=config - ).for_global_scope() + global_options = self._parse(flags=flags, env=env, config=config).for_global_scope() assert global_options.shell_str_listy == expected default = ['--default1', '--default2=test'] @@ -840,7 +838,7 @@ def test_file_spec_args(self) -> None: bootstrapper = OptionsBootstrapper.create(args=shlex.split(f"./pants {flags}")) bootstrap_options = bootstrapper.bootstrap_options.for_global_scope() options = self._parse(flags=flags, bootstrap_option_values=bootstrap_options) - sorted_specs = sorted(options.positional_args) + sorted_specs = sorted(options.specs) self.assertEqual(['bar', 'fleem:tgt', 'foo', 'morx:tgt'], sorted_specs) def test_passthru_args(self): @@ -1382,7 +1380,7 @@ def parse_joined_command_line(*args): # Set the Levenshtein edit distance to search for misspelled options. 'option_name_check_distance': 2, # If bootstrap option values are provided, this option is accessed and must be provided. - 'positional_arg_files': [], + 'spec_files': [], }, }) return self._parse(flags=safe_shlex_join(list(args)), diff --git a/src/python/pants/testutil/console_rule_test_base.py b/src/python/pants/testutil/console_rule_test_base.py index 34d3c22a154..6799748c0f3 100644 --- a/src/python/pants/testutil/console_rule_test_base.py +++ b/src/python/pants/testutil/console_rule_test_base.py @@ -56,7 +56,7 @@ def execute_rule(self, args=tuple(), env=tuple(), exit_code=0, additional_params workspace = Workspace(scheduler) # Run for the target specs parsed from the args. - address_specs = TargetRootsCalculator.parse_address_specs(full_options.positional_args, self.build_root) + address_specs = TargetRootsCalculator.parse_address_specs(full_options.specs, self.build_root) params = Params(address_specs, console, options_bootstrapper, workspace, *additional_params) actual_exit_code = self.scheduler.run_console_rule(self.goal_cls, params)