Repository: reframe-hpc/reframe

Issue:
using an action option via mode configuration does not work
A colleague noticed that some of our execution mode configurations are no longer working as expected after upgrading to reframe 4.7 and pinpointed the specific commit where the behavior seemed to change:

> After some manual bisection on git tag -l I found that this seems to have started breaking between 4.6.4 and 4.7.0. It looks like commit aa75c377 was the one where it started failing.

It seems like maybe an initial parsing of the cli arguments is failing before the options for the specified mode are added.

Here's a minimal reproducer:

```console
> cat test-config.py
site_configuration = {
    'modes': [
        {
            'name': 'run',
            'options': ['--run']
        }
    ]
}
> reframe --version
4.7.1
> reframe -C test-config.py --mode=run
usage: reframe [-h] [--compress-report] [--dont-restage] [--keep-stage-files]
               [-o DIR] [--perflogdir DIR] [--prefix DIR] [--report-file FILE]
               [--report-junit FILE] [-s DIR] [--save-log-files]
               [--timestamp [TIMEFMT]] [-c PATH] [-R] [--cpu-only] [--failed]
               [--gpu-only] [--maintainer PATTERN] [-n PATTERN] [-p PATTERN]
               [-T PATTERN] [-t PATTERN] [-x PATTERN] [-E EXPR]
               (--ci-generate FILE | --delete-stored-sessions QUERY | --describe | --describe-stored-sessions QUERY | --describe-stored-testcases QUERY | --detect-host-topology [FILE] | --dry-run | -L [{C,T}] | --list-stored-sessions [QUERY] | --list-stored-testcases QUERY | -l [{C,T}] | --list-tags | --performance-compare CMPSPEC | -r | --show-config [PARAM] | -V)
               [--disable-hook NAME] [--duration TIMEOUT] [--exec-order ORDER]
               [--exec-policy POLICY] [--flex-alloc-nodes {all|STATE|NUM}]
               [--flex-alloc-strict] [-J OPT] [--max-retries NUM]
               [--maxfail NUM] [--mode MODE] [--reruns N]
               [--restore-session [REPORT]] [--retries-threshold VALUE[%]]
               [-S [TEST.]VAR=VAL] [--skip-performance-check]
               [--skip-prgenv-check] [--skip-sanity-check]
               [--skip-system-check] [-M MAPPING] [-m MOD]
               [--module-mappings FILE] [--module-path PATH]
               [--non-default-craype] [--purge-env] [-u MOD]
               [--distribute [{all|avail|STATE}]] [-P VAR:VAL0,VAL1,...]
               [--repeat N] [--performance-report [PERFORMANCE_REPORT]]
               [--session-extras KV_DATA] [-C FILE] [--failure-stats]
               [--nocolor] [--system SYSTEM]
               [--table-format {csv,pretty,plain}] [-v] [-q]
reframe: error: one of the arguments --ci-generate --delete-stored-sessions --describe --describe-stored-sessions --describe-stored-testcases --detect-host-topology --dry-run -L/--list-detailed --list-stored-sessions --list-stored-testcases -l/--list --list-tags --performance-compare -r/--run --show-config -V/--version is required
```

Hints from issue comments:
@vkarak: Indeed, this will not work with ReFrame >= 4.7. You will have to pass the action option outside the mode. This change of behaviour for modes has gone undocumented and we should update the docs.

Making the `--mode` option an "action" mode would automatically require the mode's `options` to include a real action (`-r`, `-l` etc.), which is against the original intent of execution modes.@dmargala: ah okay, thanks for the explanation!@vkarak: It was a bit more complicated after all. Since 4.7 you have to specify an action option in the command line and if you have one in your execution mode, it is implementation defined which one will take effect. To avoid this, #3432 disallows completely action options from the execution modes. This will take effect from 4.8, so it's better if you remove any action option from your execution modes.@dmargala: Alright, we can update our configuration and usage patterns. Thanks again for the clarification.

Solution patch:
diff --git a/docs/config_reference.rst b/docs/config_reference.rst
index a3e9cb470..b4caf1b43 100644
--- a/docs/config_reference.rst
+++ b/docs/config_reference.rst
@@ -1698,6 +1698,14 @@ The options of an execution mode will be passed to ReFrame as if they were speci
 
    The command-line options associated with this execution mode.
 
+   .. warning::
+      :ref:`Command options <commands>` are *not* allowed in execution modes.
+      These should always be passed explicitly in command line.
+
+   .. versionchanged:: 4.8
+
+      Command options are explicitly disallowed in execution modes.
+
 
 .. py:attribute:: modes.target_systems
 
diff --git a/docs/manpage.rst b/docs/manpage.rst
index f92305bb7..3801b8bc7 100644
--- a/docs/manpage.rst
+++ b/docs/manpage.rst
@@ -666,6 +666,14 @@ Options controlling ReFrame execution
    .. versionchanged:: 4.1
       Options that can be specified multiple times are now combined between execution modes and the command line.
 
+   .. versionchanged:: 4.7
+      The :option:`--mode` must always be combined with a :ref:`command option <commands>`.
+      If the mode contains a command option already, the command option that will finally take effect is implementation defined.
+
+   .. versionchanged:: 4.8
+      Command options are disallowed from execution modes.
+
+
 .. option:: --reruns=N
 
    Rerun the whole test session ``N`` times.
diff --git a/reframe/frontend/argparse.py b/reframe/frontend/argparse.py
index ceec757b1..b1a1dc6d2 100644
--- a/reframe/frontend/argparse.py
+++ b/reframe/frontend/argparse.py
@@ -160,6 +160,10 @@ def __init__(self, holder, shared_options=None):
         # options. Values are tuples of the form (envvar, configvar)
         self._option_map = shared_options if shared_options is not None else {}
 
+        # We store the options (without actions) to quickly decide if an option
+        # is part of specific option group
+        self._options = set()
+
     def __getattr__(self, name):
         # Delegate all unknown public attribute requests to the underlying
         # holder
@@ -176,7 +180,15 @@ def __setattr__(self, name, value):
         else:
             setattr(self._holder, name, value)
 
+    def has_known_options(self, args):
+        flags = {arg for arg in args if arg.startswith('-')}
+        return flags & self._options
+
     def add_argument(self, *flags, **kwargs):
+        # Store all flags separately
+        for opt in flags:
+            self._options.add(opt)
+
         try:
             opt_name = kwargs['dest']
         except KeyError:
diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py
index c1a6eef86..9b9b9b2e8 100644
--- a/reframe/frontend/cli.py
+++ b/reframe/frontend/cli.py
@@ -947,6 +947,15 @@ def restrict_logging():
                 printer.debug(f'Expanding execution mode {options.mode!r}: '
                               f'{" ".join(mode_args)}')
 
+                action_opts = action_options.has_known_options(mode_args)
+                if action_opts:
+                    printer.error(
+                        f'Expanding execution mode {options.mode!r} failed: '
+                        'action options are not allowed in modes: '
+                        f'{" ".join(action_opts)}'
+                    )
+                    sys.exit(1)
+
                 # Parse the mode's options and reparse the command-line
                 options = argparser.parse_args(mode_args,
                                                suppress_required=True)


Test patch:
diff --git a/unittests/test_argparser.py b/unittests/test_argparser.py
index 6be36f383..4b59b69d4 100644
--- a/unittests/test_argparser.py
+++ b/unittests/test_argparser.py
@@ -50,6 +50,10 @@ def test_arguments(argparser, foo_options):
 
     foo_options.add_argument('--foo-bar', action='store_true')
     foo_options.add_argument('--alist', action='append', default=[])
+    assert foo_options.has_known_options(['-f', '-l', '-b'])
+    assert foo_options.has_known_options(['--foobar', '-l', '-b'])
+    assert not foo_options.has_known_options(['-l', '-b'])
+
     options = argparser.parse_args(['--foobar', '--foo-bar'])
     assert options.foobar
     assert options.foo_bar