automate_observation: warn about ignored flag combinations

uraniborg/docs/automate_observation.md

@@ -42,6 +42,24 @@ A requested serial that is not connected is not silently skipped: it is logged
 as an error, reported as `FAILED` in the final summary, and makes the script
 exit with code `1`. The remaining requested devices are still observed.
 
+### Ignored Flag Combinations
+
+Some flags only matter together with others. When a flag would be ignored, the
+script logs a warning at startup and then runs as it otherwise would; the exit
+code is not affected:
+
+- `--check_preinstalled_only`, `--verifier_path`, `--no_prefetch`,
+  `--cache_dir`, `--cache_prefetch_concurrency` and `--cache_prefetch_timeout`
+  have no effect without `--perform_inclusion_proof_check`. The two integer
+  flags only count as given when their value differs from the default.
+- `--cache_prefetch_concurrency` and `--cache_prefetch_timeout` also have no
+  effect with `--no_prefetch`, since they only tune pre-fetching.
+  (`--cache_dir` is still used: verification reads the cache.)
+- `--pull-all-apks` together with `--pull-preinstalled-apks-only` pulls only
+  pre-installed APKs.
+
+`--perform_inclusion_proof_check` without `--verifier_path` remains an error.
+
 ### Machine-Readable Progress (`--events`)
 
 Log messages are meant for people and may change between versions. Wrappers

uraniborg/scripts/python/automate_observation.py

@@ -145,6 +145,54 @@ def parse_arguments() -> argparse.Namespace:
   return args
 
 
+def validate_argument_combinations(args: argparse.Namespace) -> list[str]:
+  """Finds flags that are ignored because of how they are combined.
+
+  These combinations are not errors, so that existing invocations keep
+  working; run() logs each returned message as a warning. Combinations that
+  cannot work at all are rejected by parse_arguments() instead.
+
+  Args:
+    args: Parsed arguments from parse_arguments().
+
+  Returns:
+    One human-readable warning per ignored flag or conflict, in a stable
+    order. Empty if nothing is ignored.
+  """
+  warnings = []
+  # Integer flags always have a value, so only a non-default one counts as
+  # given. Only prefetch_log_entries() reads them.
+  prefetch_tuning = [
+      ("--cache_prefetch_concurrency",
+       args.cache_prefetch_concurrency !=
+       inclusion_proof_check.DEFAULT_PREFETCH_CONCURRENCY),
+      ("--cache_prefetch_timeout",
+       args.cache_prefetch_timeout !=
+       inclusion_proof_check.DEFAULT_PREFETCH_TIMEOUT),
+  ]
+  if not args.perform_inclusion_proof_check:
+    ignored = [
+        ("--check_preinstalled_only", args.check_preinstalled_only),
+        ("--verifier_path", args.verifier_path is not None),
+        ("--no_prefetch", args.no_prefetch),
+        ("--cache_dir", args.cache_dir is not None),
+    ] + prefetch_tuning
+    for flag, given in ignored:
+      if given:
+        warnings.append("{} has no effect without "
+                        "--perform_inclusion_proof_check.".format(flag))
+  elif args.no_prefetch:
+    # Pre-fetching is skipped, so its tuning flags are unused. --cache_dir is
+    # not: verification itself still uses the cache.
+    for flag, given in prefetch_tuning:
+      if given:
+        warnings.append("{} has no effect with --no_prefetch.".format(flag))
+  if args.pull_all_apks is not None and args.pull_preinstalled_apks_only:
+    warnings.append("--pull-all-apks and --pull-preinstalled-apks-only were "
+                    "both given; only pre-installed APKs will be pulled.")
+  return warnings
+
+
 def set_up_logging(args: argparse.Namespace) -> logging.Logger:
   """Sets up various logging parameters.
 
@@ -1321,6 +1369,8 @@ def run(args: argparse.Namespace, logger: logging.Logger,
     return 0, as they always have; run_finished.error describes them.
   """
   events.emit("run_started", argv=sys.argv[1:], pid=os.getpid())
+  for warning in validate_argument_combinations(args):
+    logger.warning(warning)
 
   def early_exit(reason: str, message: str) -> int:
     events.finish_run(0, error=_error(reason, message))

uraniborg/scripts/python/tests/test_automate_observation.py

@@ -1876,5 +1876,129 @@ def test_main_without_serial_no_devices_keeps_early_return(
   m["logger"].error.assert_any_call("No devices connected!")
 
 
+_NO_PROOF = "has no effect without --perform_inclusion_proof_check."
+_BOTH_PULL = ("--pull-all-apks and --pull-preinstalled-apks-only were both "
+              "given; only pre-installed APKs will be pulled.")
+_PROOF = ("--perform_inclusion_proof_check", "--verifier_path=/v")
+
+
+def _warnings_for(monkeypatch: pytest.MonkeyPatch, *extra: str) -> list[str]:
+  _set_argv(monkeypatch, *extra)
+  return automate_observation.validate_argument_combinations(
+      automate_observation.parse_arguments())
+
+
+@pytest.mark.parametrize(
+    "extra",
+    [
+        (),
+        _PROOF,
+        (*_PROOF, "--check_preinstalled_only", "--cache_dir=/c",
+         "--cache_prefetch_concurrency=4", "--cache_prefetch_timeout=30"),
+        # Verification still uses the cache when pre-fetching is off.
+        (*_PROOF, "--no_prefetch", "--cache_dir=/c"),
+        ("--pull-all-apks",),
+        ("--pull-preinstalled-apks-only",),
+        (*_PROOF, "--pull-all-apks"),
+        # Integer flags set to their defaults count as not given.
+        ("--cache_prefetch_concurrency={}".format(
+            inclusion_proof_check.DEFAULT_PREFETCH_CONCURRENCY),
+         "--cache_prefetch_timeout={}".format(
+             inclusion_proof_check.DEFAULT_PREFETCH_TIMEOUT)),
+    ],
+    ids=["defaults", "proof_check", "proof_check_all_options",
+         "no_prefetch_with_cache_dir", "pull_all",
+         "pull_preinstalled", "proof_check_and_pull_all",
+         "explicit_int_defaults"],
+)
+def test_validate_argument_combinations_no_warnings(
+    monkeypatch: pytest.MonkeyPatch, extra,
+):
+  assert _warnings_for(monkeypatch, *extra) == []
+
+
+@pytest.mark.parametrize(
+    "extra, expected",
+    [
+        (("--check_preinstalled_only",),
+         "--check_preinstalled_only " + _NO_PROOF),
+        (("--verifier_path=/v",), "--verifier_path " + _NO_PROOF),
+        (("--no_prefetch",), "--no_prefetch " + _NO_PROOF),
+        (("--cache_dir=/c",), "--cache_dir " + _NO_PROOF),
+        (("--cache_prefetch_concurrency=4",),
+         "--cache_prefetch_concurrency " + _NO_PROOF),
+        (("--cache_prefetch_timeout=30",),
+         "--cache_prefetch_timeout " + _NO_PROOF),
+        ((*_PROOF, "--no_prefetch", "--cache_prefetch_concurrency=4"),
+         "--cache_prefetch_concurrency has no effect with --no_prefetch."),
+        ((*_PROOF, "--no_prefetch", "--cache_prefetch_timeout=30"),
+         "--cache_prefetch_timeout has no effect with --no_prefetch."),
+        (("--pull-all-apks", "--pull-preinstalled-apks-only"), _BOTH_PULL),
+    ],
+    ids=["check_preinstalled_only", "verifier_path", "no_prefetch",
+         "cache_dir", "cache_prefetch_concurrency", "cache_prefetch_timeout",
+         "no_prefetch_concurrency", "no_prefetch_timeout", "both_pull_flags"],
+)
+def test_validate_argument_combinations_one_warning_each(
+    monkeypatch: pytest.MonkeyPatch, extra, expected,
+):
+  assert _warnings_for(monkeypatch, *extra) == [expected]
+
+
+def test_validate_argument_combinations_reports_all_in_stable_order(
+    monkeypatch: pytest.MonkeyPatch,
+):
+  assert _warnings_for(
+      monkeypatch, "--pull-preinstalled-apks-only", "--pull-all-apks",
+      "--cache_dir=/c", "--check_preinstalled_only") == [
+          "--check_preinstalled_only " + _NO_PROOF,
+          "--cache_dir " + _NO_PROOF,
+          _BOTH_PULL,
+      ]
+
+
+def test_validate_argument_combinations_no_prefetch_with_both_tuning_flags(
+    monkeypatch: pytest.MonkeyPatch,
+):
+  assert _warnings_for(
+      monkeypatch, *_PROOF, "--no_prefetch", "--cache_prefetch_concurrency=4",
+      "--cache_prefetch_timeout=30") == [
+          "--cache_prefetch_concurrency has no effect with --no_prefetch.",
+          "--cache_prefetch_timeout has no effect with --no_prefetch.",
+      ]
+  # Without the proof check, each flag gets only the "without" warning.
+  assert _warnings_for(
+      monkeypatch, "--no_prefetch", "--cache_prefetch_timeout=30") == [
+          "--no_prefetch " + _NO_PROOF,
+          "--cache_prefetch_timeout " + _NO_PROOF,
+      ]
+
+
+def test_main_logs_argument_warnings_before_doing_any_work(
+    serial_main_mocks, monkeypatch: pytest.MonkeyPatch,
+):
+  m = serial_main_mocks
+  m["supported_platform"].return_value = False  # stop right after the checks
+  _set_argv(monkeypatch, "--no_prefetch", "--pull-all-apks",
+            "--pull-preinstalled-apks-only")
+
+  automate_observation.main()
+
+  assert m["logger"].warning.call_args_list == [
+      mock.call("--no_prefetch " + _NO_PROOF), mock.call(_BOTH_PULL)]
+
+
+def test_main_logs_no_argument_warnings_by_default(
+    serial_main_mocks, monkeypatch: pytest.MonkeyPatch,
+):
+  m = serial_main_mocks
+  m["AdbWrapper"].devices.return_value = [_make_mock_device("DEV1")]
+  _set_argv(monkeypatch)
+
+  automate_observation.main()
+
+  m["logger"].warning.assert_not_called()
+
+
 if __name__ == "__main__":
   sys.exit(pytest.main([__file__]))