From 27c89a4fc83c76c85131a4b2ec3849022e6b218e Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 8 Jan 2024 13:35:37 +0000 Subject: [PATCH 1/2] Ensure that sys argv cleaner cleans up --workflow-name=foo as well as --workflow-name foo --- changes.d/5909.fix.md | 2 + cylc/flow/option_parsers.py | 64 ++++++++++++++++++++++--------- tests/unit/test_option_parsers.py | 16 ++++++++ 3 files changed, 63 insertions(+), 19 deletions(-) create mode 100644 changes.d/5909.fix.md diff --git a/changes.d/5909.fix.md b/changes.d/5909.fix.md new file mode 100644 index 00000000000..b6a229498a0 --- /dev/null +++ b/changes.d/5909.fix.md @@ -0,0 +1,2 @@ +Fix a bug where Cylc VIP did not remove --workflow-name= from +Cylc play arguments. \ No newline at end of file diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index 3bf59720d21..1b4945f2bc2 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -33,7 +33,7 @@ import sys from textwrap import dedent -from typing import Any, Dict, Optional, List, Tuple, Union +from typing import Any, Dict, Iterable, Optional, List, Tuple, Union from cylc.flow import LOG from cylc.flow.terminal import supports_color, DIM @@ -820,17 +820,33 @@ def combine_options(*args, modify=None): def cleanup_sysargv( - script_name, - workflow_id, - options, - compound_script_opts, - script_opts, - source, -): + script_name: str, + workflow_id: str, + options: 'Values', + compound_script_opts: Iterable['OptionSettings'], + script_opts: Iterable['OptionSettings'], + source: str, +) -> None: """Remove unwanted options from sys.argv Some cylc scripts (notably Cylc Play when it is re-invoked on a scheduler - server) require the correct content in sys.argv. + server) require the correct content in sys.argv: This function + subtracts the unwanted options from sys.argv. + + Args: + script_name: + Name of the target script. For example if we are + using this for the play step of cylc vip then this + will be "play". + workflow_id: + options: + Actual options provided to the compound script. + compound_script_options: + Options available in compound script. + script_options: + Options available in target script. + source: + Source directory. """ # Organize Options by dest. script_opts_by_dest = { @@ -841,21 +857,31 @@ def cleanup_sysargv( x.kwargs.get('dest', x.args[0].strip(DOUBLEDASH)): x for x in compound_script_opts } - # Filter out non-cylc-play options. - args = [i.split('=')[0] for i in sys.argv] - for unwanted_opt in (set(options.__dict__)) - set(script_opts_by_dest): - for arg in compound_opts_by_dest[unwanted_opt].args: - if arg in sys.argv: - index = sys.argv.index(arg) + + # Filter out non-cylc-play options: + # The set of options which we want to weed out: + for unwanted_dest in (set(options.__dict__)) - set(script_opts_by_dest): + + # The possible ways this could be written - if the above + # were "workflow_name" this could be '-n' or '--workflow-name': + for unwanted_arg in compound_opts_by_dest[unwanted_dest].args: + + # Check for args which are standalone or space separated + # `--workflow-name foo`: + if unwanted_arg in sys.argv: + index = sys.argv.index(unwanted_arg) sys.argv.pop(index) if ( - compound_opts_by_dest[unwanted_opt].kwargs['action'] + compound_opts_by_dest[unwanted_dest].kwargs['action'] not in ['store_true', 'store_false'] ): sys.argv.pop(index) - elif arg in args: - index = args.index(arg) - sys.argv.pop(index) + + # Check for `--workflow-name=foo`: + elif unwanted_arg in [a.split('=')[0] for a in sys.argv]: + for cli_arg in sys.argv: + if cli_arg.startswith(unwanted_arg): + sys.argv.remove(cli_arg) # replace compound script name: sys.argv[1] = script_name diff --git a/tests/unit/test_option_parsers.py b/tests/unit/test_option_parsers.py index e3831a1daea..b183b3ef8a5 100644 --- a/tests/unit/test_option_parsers.py +++ b/tests/unit/test_option_parsers.py @@ -410,6 +410,22 @@ def test_combine_options(inputs, expect): 'play myworkflow'.split(), id='removes --key=value' ), + param( + # Test for https://github.com/cylc/cylc-flow/issues/5905 + 'vip ./myworkflow --no-run-name --workflow-name=hi'.split(), + { + 'script_name': 'play', + 'workflow_id': 'myworkflow', + 'compound_script_opts': [ + OptionSettings(['--no-run-name'], action='store_true'), + OptionSettings(['--workflow-name'], action='store') + ], + 'script_opts': [], + 'source': './myworkflow', + }, + 'play myworkflow'.split(), + id='equals-bug' + ), ] ) def test_cleanup_sysargv(monkeypatch, argv_before, kwargs, expect): From 88e821978b4dd89ce354e39bb049dbef9c21d931 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 9 Jan 2024 13:56:28 +0000 Subject: [PATCH 2/2] Improved the sysargv filter. Wrote some extra tests. --- cylc/flow/option_parsers.py | 75 +++++++++++++++++-------- tests/unit/test_option_parsers.py | 92 ++++++++++++++++--------------- 2 files changed, 99 insertions(+), 68 deletions(-) diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index 1b4945f2bc2..ab5f1fcbf91 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -858,39 +858,66 @@ def cleanup_sysargv( for x in compound_script_opts } - # Filter out non-cylc-play options: - # The set of options which we want to weed out: + # Get a list of unwanted args: + unwanted_compound: List[str] = [] + unwanted_simple: List[str] = [] for unwanted_dest in (set(options.__dict__)) - set(script_opts_by_dest): - - # The possible ways this could be written - if the above - # were "workflow_name" this could be '-n' or '--workflow-name': for unwanted_arg in compound_opts_by_dest[unwanted_dest].args: + if ( + compound_opts_by_dest[unwanted_dest].kwargs.get('action', None) + in ['store_true', 'store_false'] + ): + unwanted_simple.append(unwanted_arg) + else: + unwanted_compound.append(unwanted_arg) - # Check for args which are standalone or space separated - # `--workflow-name foo`: - if unwanted_arg in sys.argv: - index = sys.argv.index(unwanted_arg) - sys.argv.pop(index) - if ( - compound_opts_by_dest[unwanted_dest].kwargs['action'] - not in ['store_true', 'store_false'] - ): - sys.argv.pop(index) - - # Check for `--workflow-name=foo`: - elif unwanted_arg in [a.split('=')[0] for a in sys.argv]: - for cli_arg in sys.argv: - if cli_arg.startswith(unwanted_arg): - sys.argv.remove(cli_arg) + new_args = filter_sysargv(sys.argv, unwanted_simple, unwanted_compound) # replace compound script name: - sys.argv[1] = script_name + new_args[1] = script_name # replace source path with workflow ID. if str(source) in sys.argv: - sys.argv.remove(str(source)) + new_args.remove(str(source)) if workflow_id not in sys.argv: - sys.argv.append(workflow_id) + new_args.append(workflow_id) + + sys.argv = new_args + + +def filter_sysargv( + sysargs, unwanted_simple: List, unwanted_compound: List +) -> List: + """Create a copy of sys.argv without unwanted arguments: + + Cases: + >>> this = filter_sysargv + >>> this(['--foo', 'expects-a-value', '--bar'], [], ['--foo']) + ['--bar'] + >>> this(['--foo=expects-a-value', '--bar'], [], ['--foo']) + ['--bar'] + >>> this(['--foo', '--bar'], ['--foo'], []) + ['--bar'] + """ + pop_next: bool = False + new_args: List = [] + for this_arg in sysargs: + parts = this_arg.split('=', 1) + if pop_next: + pop_next = False + continue + elif parts[0] in unwanted_compound: + # Case --foo=value or --foo value + if len(parts) == 1: + # --foo value + pop_next = True + continue + elif parts[0] in unwanted_simple: + # Case --foo does not expect a value: + continue + else: + new_args.append(this_arg) + return new_args def log_subcommand(*args): diff --git a/tests/unit/test_option_parsers.py b/tests/unit/test_option_parsers.py index b183b3ef8a5..05b52badfbe 100644 --- a/tests/unit/test_option_parsers.py +++ b/tests/unit/test_option_parsers.py @@ -26,7 +26,7 @@ import cylc.flow.flags from cylc.flow.option_parsers import ( CylcOptionParser as COP, Options, combine_options, combine_options_pair, - OptionSettings, cleanup_sysargv + OptionSettings, cleanup_sysargv, filter_sysargv ) @@ -321,20 +321,6 @@ def test_combine_options(inputs, expect): @pytest.mark.parametrize( 'argv_before, kwargs, expect', [ - param( - 'vip myworkflow --foo something'.split(), - { - 'script_name': 'play', - 'workflow_id': 'myworkflow', - 'compound_script_opts': [ - OptionSettings(['--foo', '-f']), - ], - 'script_opts': [ - OptionSettings(['--foo', '-f'])] - }, - 'play myworkflow --foo something'.split(), - id='no opts to remove' - ), param( 'vip myworkflow -f something -b something_else --baz'.split(), { @@ -397,35 +383,6 @@ def test_combine_options(inputs, expect): 'play --foo something myworkflow'.split(), id='no path given' ), - param( - 'vip --bar=something'.split(), - { - 'script_name': 'play', - 'workflow_id': 'myworkflow', - 'compound_script_opts': [ - OptionSettings(['--bar', '-b'])], - 'script_opts': [], - 'source': './myworkflow', - }, - 'play myworkflow'.split(), - id='removes --key=value' - ), - param( - # Test for https://github.com/cylc/cylc-flow/issues/5905 - 'vip ./myworkflow --no-run-name --workflow-name=hi'.split(), - { - 'script_name': 'play', - 'workflow_id': 'myworkflow', - 'compound_script_opts': [ - OptionSettings(['--no-run-name'], action='store_true'), - OptionSettings(['--workflow-name'], action='store') - ], - 'script_opts': [], - 'source': './myworkflow', - }, - 'play myworkflow'.split(), - id='equals-bug' - ), ] ) def test_cleanup_sysargv(monkeypatch, argv_before, kwargs, expect): @@ -448,6 +405,53 @@ def test_cleanup_sysargv(monkeypatch, argv_before, kwargs, expect): assert sys.argv == dummy_cylc_path + expect +@pytest.mark.parametrize( + 'sysargs, simple, compound, expect', ( + param( + # Test for https://github.com/cylc/cylc-flow/issues/5905 + '--no-run-name --workflow-name=name'.split(), + ['--no-run-name'], + ['--workflow-name'], + [], + id='--workflow-name=name' + ), + param( + '--foo something'.split(), + [], [], '--foo something'.split(), + id='no-opts-removed' + ), + param( + [], ['--foo'], ['--bar'], [], + id='Null-check' + ), + param( + '''--keep1 --keep2 42 --keep3=Hi + --throw1 --throw2 84 --throw3=There + '''.split(), + ['--throw1'], + '--throw2 --throw3'.split(), + '--keep1 --keep2 42 --keep3=Hi'.split(), + id='complex' + ), + param( + "--foo 'foo=42' --bar='foo=94'".split(), + [], ['--foo'], + ['--bar=\'foo=94\''], + id='--bar=\'foo=94\'' + ) + ) +) +def test_filter_sysargv( + sysargs, simple, compound, expect +): + """It returns the subset of sys.argv that we ask for. + + n.b. The three most basic cases for this function are stored in + its own docstring. + """ + assert filter_sysargv(sysargs, simple, compound) == expect + + class TestOptionSettings(): @staticmethod def test_init():