From 77368dfff24699bd5bc4bdf30347b6afe611dc89 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Sun, 26 Jul 2020 21:54:18 -0700 Subject: [PATCH] Fix fromfile expansion to skip passthrough args. (#10457) We should perform no processing of args in passthrough position at all. --- src/python/pants/option/options_test.py | 42 ++++++++++++++++++------- src/python/pants/option/parser.py | 13 ++++---- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/python/pants/option/options_test.py b/src/python/pants/option/options_test.py index 0e1cfbe285d..1806d6ddeae 100644 --- a/src/python/pants/option/options_test.py +++ b/src/python/pants/option/options_test.py @@ -265,6 +265,7 @@ def register_global(*args, **kwargs): options.register("fromfile", "--intvalue", type=int) options.register("fromfile", "--dictvalue", type=dict) options.register("fromfile", "--listvalue", type=list) + options.register("fromfile", "--passthru-listvalue", type=list, passthrough=True) options.register("fromfile", "--appendvalue", type=list, member_type=int) # For fingerprint tests @@ -886,18 +887,20 @@ def test_file_spec_args(self) -> None: def test_passthru_args_subsystems_and_goals(self): # Test that passthrough args are applied. - options = self._parse(flags="") options = Options.create( env={}, config=self._create_config(), known_scope_infos=[global_scope(), task("test"), subsystem("passconsumer")], - args=["./pants", "test", "target", "--", "bar", "--baz"], + args=["./pants", "test", "target", "--", "bar", "--baz", "@dont_fromfile_expand_me"], ) options.register( "passconsumer", "--passthing", passthrough=True, type=list, member_type=str ) - self.assertEqual(["bar", "--baz"], options.for_scope("passconsumer").passthing) + self.assertEqual( + ["bar", "--baz", "@dont_fromfile_expand_me"], + options.for_scope("passconsumer").passthing, + ) def test_at_most_one_goal_with_passthru_args(self): with self.assertRaisesWithMessageContaining( @@ -1289,11 +1292,11 @@ def test_fingerprintable_inverted(self) -> None: self.assertNotIn((str, "shant_be_fingerprinted"), pairs) def assert_fromfile(self, parse_func, expected_append=None, append_contents=None): - def _do_assert_fromfile(dest, expected, contents): + def _do_assert_fromfile(dest, expected, contents, passthru_flags=""): with temporary_file(binary_mode=False) as fp: fp.write(contents) fp.close() - options = parse_func(dest, fp.name) + options = parse_func(dest, fp.name, passthru_flags) self.assertEqual(expected, options.for_scope("fromfile")[dest]) _do_assert_fromfile(dest="string", expected="jake", contents="jake") @@ -1324,6 +1327,18 @@ def _do_assert_fromfile(dest, expected, contents): """ ), ) + _do_assert_fromfile( + dest="passthru_listvalue", + expected=["a", "1", "2", "bob", "@jake"], + contents=dedent( + """ + ['a', + 1, + 2] + """ + ), + passthru_flags="bob @jake", + ) expected_append = expected_append or [1, 2, 42] append_contents = append_contents or dedent( @@ -1338,8 +1353,10 @@ def _do_assert_fromfile(dest, expected, contents): _do_assert_fromfile(dest="appendvalue", expected=expected_append, contents=append_contents) def test_fromfile_flags(self) -> None: - def parse_func(dest, fromfile): - return self._parse(flags=f"fromfile --{dest.replace('_', '-')}=@{fromfile}") + def parse_func(dest, fromfile, passthru_flags): + return self._parse( + flags=f"fromfile --{dest.replace('_', '-')}=@{fromfile} -- {passthru_flags}" + ) # You can only append a single item at a time with append flags, ie: we don't override the # default list like we do with env of config. As such, send in a single append value here @@ -1347,15 +1364,18 @@ def parse_func(dest, fromfile): self.assert_fromfile(parse_func, expected_append=[42], append_contents="42") def test_fromfile_config(self) -> None: - def parse_func(dest, fromfile): - return self._parse(flags="fromfile", config={"fromfile": {dest: f"@{fromfile}"}}) + def parse_func(dest, fromfile, passthru_flags): + return self._parse( + flags=f"fromfile -- {passthru_flags}", config={"fromfile": {dest: f"@{fromfile}"}} + ) self.assert_fromfile(parse_func) def test_fromfile_env(self) -> None: - def parse_func(dest, fromfile): + def parse_func(dest, fromfile, passthru_flags): return self._parse( - flags="fromfile", env={f"PANTS_FROMFILE_{dest.upper()}": f"@{fromfile}"} + flags=f"fromfile -- {passthru_flags}", + env={f"PANTS_FROMFILE_{dest.upper()}": f"@{fromfile}"}, ) self.assert_fromfile(parse_func) diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py index e01512f75a1..e5d35b7bfec 100644 --- a/src/python/pants/option/parser.py +++ b/src/python/pants/option/parser.py @@ -303,13 +303,11 @@ def add_flag_val(v: Optional[Union[int, float, bool, str]]) -> None: add_flag_val(v) del flag_value_map[arg] - # If the arg is marked passthrough, additionally apply the collected passthrough_args. - if kwargs.get("passthrough"): - flag_vals.extend(parse_args_request.passthrough_args) - # Get the value for this option, falling back to defaults as needed. try: - value_history = self._compute_value(dest, kwargs, flag_vals) + value_history = self._compute_value( + dest, kwargs, flag_vals, parse_args_request.passthrough_args + ) self._history[dest] = value_history val = value_history.final_value except ParseError as e: @@ -731,7 +729,7 @@ def get_env_var_names(cls, scope: str, dest: str): env_vars = [f"PANTS_{sanitized_env_var_scope}_{udest}"] return env_vars - def _compute_value(self, dest, kwargs, flag_val_strs): + def _compute_value(self, dest, kwargs, flag_val_strs, passthru_arg_strs): """Compute the value to use for an option. The source of the value is chosen according to the ranking in Rank. @@ -797,6 +795,9 @@ def expand(val_or_str): # Get value from cmd-line flags. flag_vals = [to_value_type(expand(x)) for x in flag_val_strs] + if kwargs.get("passthrough"): + flag_vals.extend(to_value_type(x) for x in passthru_arg_strs) + if is_list_option(kwargs): # Note: It's important to set flag_val to None if no flags were specified, so we can # distinguish between no flags set vs. explicit setting of the value to [].