Skip to content

Commit

Permalink
Deprecate --positional-arg-file in favor of --spec-file (#8928)
Browse files Browse the repository at this point in the history
In preparation for non-target specs, we had renamed `--target-spec-file` to `--positional-arg-file`. 

However, in #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.
  • Loading branch information
Eric-Arellano authored Jan 9, 2020
1 parent bb2fb34 commit 56a8892
Show file tree
Hide file tree
Showing 9 changed files with 106 additions and 79 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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]),
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/bin/local_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/init/target_roots_calculator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
20 changes: 10 additions & 10 deletions src/python/pants/option/arg_splitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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.:
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand Down
80 changes: 40 additions & 40 deletions src/python/pants/option/arg_splitter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand All @@ -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.
Expand All @@ -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 '
Expand All @@ -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:
Expand All @@ -158,22 +158,22 @@ 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:
self.assert_valid_split(
'./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',
)
Expand All @@ -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'
)
Expand All @@ -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
Expand All @@ -217,21 +217,21 @@ 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:
assert_help = partial(
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')
Expand All @@ -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,
)

Expand Down
17 changes: 13 additions & 4 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
Expand Down
Loading

0 comments on commit 56a8892

Please sign in to comment.