Merge pull request #6102 from MetRonnie/compound

`cylc vip/vr`: do not pass rose options to `cylc play`

changes.d/6102.fix.md

@@ -0,0 +1 @@
+Fixed bug introduced in 8.2.6 in `cylc vip` & `cylc vr` when using cylc-rose options (`-S`, `-D`, `-O`).

cylc/flow/option_parsers.py

@@ -33,7 +33,7 @@
 
 import sys
 from textwrap import dedent
-from typing import Any, Dict, Iterable, Optional, List, Tuple, Union
+from typing import Any, Dict, Iterable, Optional, List, Set, Tuple, Union
 
 from cylc.flow import LOG
 from cylc.flow.terminal import supports_color, DIM
@@ -62,7 +62,13 @@ class OptionSettings():
     cylc.flow.option_parsers(thismodule).combine_options_pair.
     """
 
-    def __init__(self, argslist, sources=None, useif=None, **kwargs):
+    def __init__(
+        self,
+        argslist: List[str],
+        sources: Optional[Set[str]] = None,
+        useif: str = '',
+        **kwargs
+    ):
         """Init function:
 
         Args:
@@ -71,13 +77,10 @@ def __init__(self, argslist, sources=None, useif=None, **kwargs):
             useif: badge for use by Cylc optionparser.
             **kwargs: kwargs for optparse.option.
         """
-        self.kwargs: Dict[str, str] = {}
-        self.sources: set = sources if sources is not None else set()
-        self.useif: str = useif if useif is not None else ''
-
-        self.args: list[str] = argslist
-        for kwarg, value in kwargs.items():
-            self.kwargs.update({kwarg: value})
+        self.args: List[str] = argslist
+        self.kwargs: Dict[str, Any] = kwargs
+        self.sources: Set[str] = sources if sources is not None else set()
+        self.useif: str = useif
 
     def __eq__(self, other):
         """Args and Kwargs, but not other props equal.
@@ -862,7 +865,7 @@ def cleanup_sysargv(
     # 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):
+    for unwanted_dest in set(options.__dict__) - set(script_opts_by_dest):
         for unwanted_arg in compound_opts_by_dest[unwanted_dest].args:
             if (
                 compound_opts_by_dest[unwanted_dest].kwargs.get('action', None)

cylc/flow/scripts/validate_install_play.py

@@ -105,10 +105,7 @@ def main(parser: COP, options: 'Values', workflow_id: Optional[str] = None):
         workflow_id,
         options,
         compound_script_opts=VIP_OPTIONS,
-        script_opts=(
-            PLAY_OPTIONS + CYLC_ROSE_OPTIONS
-            + parser.get_std_options()
-        ),
+        script_opts=(*PLAY_OPTIONS, *parser.get_std_options()),
         source=orig_source,
     )
 

cylc/flow/scripts/validate_reinstall.py

@@ -190,10 +190,7 @@ def vro_cli(parser: COP, options: 'Values', workflow_id: str):
             unparsed_wid,
             options,
             compound_script_opts=VR_OPTIONS,
-            script_opts=(
-                PLAY_OPTIONS + CYLC_ROSE_OPTIONS
-                + parser.get_std_options()
-            ),
+            script_opts=(*PLAY_OPTIONS, *parser.get_std_options()),
             source='',  # Intentionally blank
         )
         log_subcommand(*sys.argv[1:])

tests/unit/test_option_parsers.py

@@ -322,7 +322,7 @@ def test_combine_options(inputs, expect):
     'argv_before, kwargs, expect',
     [
         param(
-            'vip myworkflow -f something -b something_else --baz'.split(),
+            'vip myworkflow -f something -b something_else --baz',
             {
                 'script_name': 'play',
                 'workflow_id': 'myworkflow',
@@ -335,11 +335,11 @@ def test_combine_options(inputs, expect):
                     OptionSettings(['--foo', '-f']),
                 ]
             },
-            'play myworkflow -f something'.split(),
+            'play myworkflow -f something',
             id='remove some opts'
         ),
         param(
-            'vip myworkflow'.split(),
+            'vip myworkflow',
             {
                 'script_name': 'play',
                 'workflow_id': 'myworkflow',
@@ -350,11 +350,11 @@ def test_combine_options(inputs, expect):
                 ],
                 'script_opts': []
             },
-            'play myworkflow'.split(),
+            'play myworkflow',
             id='no opts to keep'
         ),
         param(
-            'vip ./myworkflow --foo something'.split(),
+            'vip ./myworkflow --foo something',
             {
                 'script_name': 'play',
                 'workflow_id': 'myworkflow',
@@ -365,11 +365,11 @@ def test_combine_options(inputs, expect):
                 ],
                 'source': './myworkflow',
             },
-            'play --foo something myworkflow'.split(),
+            'play --foo something myworkflow',
             id='replace path'
         ),
         param(
-            'vip --foo something'.split(),
+            'vip --foo something',
             {
                 'script_name': 'play',
                 'workflow_id': 'myworkflow',
@@ -380,11 +380,11 @@ def test_combine_options(inputs, expect):
                 ],
                 'source': './myworkflow',
             },
-            'play --foo something myworkflow'.split(),
+            'play --foo something myworkflow',
             id='no path given'
         ),
         param(
-            'vip -n myworkflow --no-run-name'.split(),
+            'vip -n myworkflow --no-run-name',
             {
                 'script_name': 'play',
                 'workflow_id': 'myworkflow',
@@ -396,17 +396,22 @@ def test_combine_options(inputs, expect):
                     OptionSettings(['--not-used']),
                 ]
             },
-            'play myworkflow'.split(),
+            'play myworkflow',
             id='workflow-id-added'
         ),
     ]
 )
-def test_cleanup_sysargv(monkeypatch, argv_before, kwargs, expect):
+def test_cleanup_sysargv(
+    monkeypatch: pytest.MonkeyPatch,
+    argv_before: str,
+    kwargs: dict,
+    expect: str
+):
     """It replaces the contents of sysargv with Cylc Play argv items.
     """
     # Fake up sys.argv: for this test.
     dummy_cylc_path = ['/pathto/my/cylc/bin/cylc']
-    monkeypatch.setattr(sys, 'argv', dummy_cylc_path + argv_before)
+    monkeypatch.setattr(sys, 'argv', dummy_cylc_path + argv_before.split())
     # Fake options too:
     opts = SimpleNamespace(**{
         i.args[0].replace('--', ''): i for i in kwargs['compound_script_opts']
@@ -418,7 +423,7 @@ def test_cleanup_sysargv(monkeypatch, argv_before, kwargs, expect):
 
     # Test the script:
     cleanup_sysargv(**kwargs)
-    assert sys.argv == dummy_cylc_path + expect
+    assert sys.argv == dummy_cylc_path + expect.split()
 
 
 @pytest.mark.parametrize(