ci: check the setting paths shipped scripts hand to cli (#146)
* ci: check the setting paths shipped scripts hand to cli
`cli -s` cannot fail. yaml-cli stores whatever dotted path it is given,
creating intermediate mappings as it goes, and exits 0; majestic then silently
ignores a key it does not recognise. So a typo in a setting path applies
nothing and reports nothing, from the moment it is written to the end of the
device's life.
t40_lite_movols-mo-805p has been shipping six of them:
cli -s .video0.bitrate: 4000
cli -s .video0.rcMode: avbr
The path is `.video0.bitrate:`, not `.video0.bitrate`. Bitrate, rate-control
mode, profile, GOP size, GOP mode and OSD size have never applied on that
camera. The trailing colons are dropped here, including the one on the
commented-out `.onvif.enabled:`, which was a trap for whoever uncommented it.
The linter checks shape and nothing else. Whether a well-formed key is one a
given majestic build actually declares can only be answered by the running
binary -- it varies by vendor and by flavour -- while shape is decidable from
the tree, costs nothing, and is the whole of the class that shipped. A path
built at runtime is skipped rather than guessed at: uvc-gadget-setup reads
`cli -g ".$1"`, which is fine and unknowable here.
It carries a floor check for the same reason lint-workflow-shell.py does. A
matcher that quietly stops matching finds no bad paths, which looks exactly
like a clean tree -- so finding no cli invocation at all is itself a failure.
80 scripts scanned, 610 invocations, 6 bad paths, all in the one device.
* ci: recheck the merged result when the cli linter itself changes
lint.yml's push trigger names the checkers it guards one by one, and the new
one was not on the list -- so a master push touching only
.github/scripts/lint-cli-paths.py would have skipped the very job that exists
because #121 shipped a broken merged result nothing re-checked.
Same reasoning as the lint-workflow-shell.py entry beside it, and the same
cost: seconds, no runners.
This commit is contained in:
Dmitry Ilyin
Executable
+170
@@ -0,0 +1,170 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Check the setting paths every shipped script hands to `cli`.
|
||||
|
||||
WHY THIS EXISTS. `cli -s` writes a value into /etc/majestic.yaml and cannot
|
||||
fail: yaml-cli stores whatever dotted path it is given, creating the
|
||||
intermediate mappings as it goes, and exits 0. Nothing downstream complains
|
||||
either, because majestic simply never reads a key it does not recognise. A typo
|
||||
in a setting path is therefore completely silent from the moment it is written
|
||||
to the end of the device's life.
|
||||
|
||||
devices/t40_lite_movols-mo-805p/general/overlay/usr/share/openipc/customizer.sh
|
||||
is what this file is made of. Six of its lines carry a trailing colon:
|
||||
|
||||
cli -s .video0.bitrate: 4000
|
||||
cli -s .video0.rcMode: avbr
|
||||
|
||||
so the path is `.video0.bitrate:`, which is not `.video0.bitrate`. Those six
|
||||
settings -- bitrate, rate-control mode, profile, GOP size, GOP mode and OSD
|
||||
size -- have never applied on that camera, and no build, no review and no boot
|
||||
ever said so. The device has shipped that way.
|
||||
|
||||
SCOPE is the shape of the path, deliberately, and nothing else. Whether
|
||||
`.video0.bitrate` is a key this majestic build actually declares is a question
|
||||
only the running binary can answer (`curl localhost/api/v1/config.json` lists
|
||||
what a given build has), and it varies by vendor and by flavour. Shape is what
|
||||
can be decided from the tree alone, it is free, and it is the whole of the
|
||||
class that shipped.
|
||||
|
||||
A path built at runtime is skipped rather than guessed at --
|
||||
devices/gk7205v200_otg_generic/.../uvc-gadget-setup reads `cli -g ".$1"` out of
|
||||
a helper, which is fine and unknowable here.
|
||||
|
||||
Usage:
|
||||
lint-cli-paths.py # check the tree
|
||||
lint-cli-paths.py --self-test # check this checker still catches things
|
||||
"""
|
||||
|
||||
import re
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
# `cli`, `sensor_cli` and the `yaml-cli` binary underneath them all take the
|
||||
# same grammar: a mode flag, then the dotted path. `wifibroadcast cli` is the
|
||||
# same applet against /etc/wfb.yaml and is caught by the same rule.
|
||||
INVOCATION = re.compile(
|
||||
r"\b(?:yaml-cli|sensor_cli|cli)\s+"
|
||||
r"(?:-[a-zA-Z]+\s+\S+\s+)*" # any -i/-o pair that came first
|
||||
r"(-s|--set|-g|--get|-d|--del)\s+"
|
||||
r"(\"[^\"]*\"|'[^']*'|\S+)"
|
||||
)
|
||||
|
||||
# A leading dot is convention rather than syntax (yaml-cli splits on "." and
|
||||
# drops empty components), so it is optional here. What is not optional is that
|
||||
# every component is a plausible YAML key.
|
||||
WELL_FORMED = re.compile(r"^\.?[A-Za-z0-9_][A-Za-z0-9_.-]*$")
|
||||
|
||||
# Interpolation of any kind means the path is not decidable from the tree.
|
||||
DYNAMIC = re.compile(r"[$`]")
|
||||
|
||||
|
||||
def path_is_malformed(path):
|
||||
"""True when this is a path yaml-cli will store and majestic never read."""
|
||||
if DYNAMIC.search(path):
|
||||
return False
|
||||
if not WELL_FORMED.match(path):
|
||||
return True
|
||||
# Empty components and a trailing separator address nothing. yaml-cli drops
|
||||
# them silently rather than refusing, which is what makes them worth
|
||||
# catching here.
|
||||
return ".." in path or path.endswith(".")
|
||||
|
||||
|
||||
def findings_in(text):
|
||||
"""(line number, offending path, source line) for each bad path in `text`."""
|
||||
out = []
|
||||
for lineno, line in enumerate(text.splitlines(), 1):
|
||||
if line.lstrip().startswith("#"):
|
||||
continue
|
||||
for match in INVOCATION.finditer(line):
|
||||
path = match.group(2).strip("\"'")
|
||||
if path_is_malformed(path):
|
||||
out.append((lineno, path, line.strip()))
|
||||
return out
|
||||
|
||||
|
||||
def is_shell(p):
|
||||
"""Shipped shell, by extension or by shebang. Markdown is not scanned: the
|
||||
documentation spells paths as `.<path>` on purpose."""
|
||||
if p.suffix == ".sh":
|
||||
return True
|
||||
try:
|
||||
with p.open("rb") as f:
|
||||
return f.read(2) == b"#!" and b"sh" in f.readline()
|
||||
except OSError:
|
||||
return False
|
||||
|
||||
|
||||
def check_tree(root):
|
||||
scanned = 0
|
||||
bad = []
|
||||
for p in sorted(Path(root).rglob("*")):
|
||||
if not p.is_file() or ".git/" in str(p) or not is_shell(p):
|
||||
continue
|
||||
try:
|
||||
text = p.read_text(errors="ignore")
|
||||
except OSError:
|
||||
continue
|
||||
if not INVOCATION.search(text):
|
||||
continue
|
||||
scanned += 1
|
||||
for lineno, path, line in findings_in(text):
|
||||
bad.append((p, lineno, path, line))
|
||||
|
||||
# A scan that finds nothing to check looks exactly like a clean run, which
|
||||
# is the failure mode every silent-typo bug in this file is made of.
|
||||
if scanned == 0:
|
||||
print("FAIL: no script in the tree invokes cli -- is the matcher broken?")
|
||||
return 1
|
||||
|
||||
for p, lineno, path, line in bad:
|
||||
print(f"{p}:{lineno}: malformed setting path {path!r}")
|
||||
print(f" {line}")
|
||||
if bad:
|
||||
print(f"\nFAIL: {len(bad)} malformed cli setting path(s) in {scanned} script(s)")
|
||||
print("A path yaml-cli stores but majestic never reads applies nothing,")
|
||||
print("and says nothing. Fix the path, or use a variable if it is dynamic.")
|
||||
return 1
|
||||
print(f"ok cli setting paths well formed in {scanned} script(s)")
|
||||
return 0
|
||||
|
||||
|
||||
def self_test():
|
||||
cases = [
|
||||
("cli -s .video0.bitrate 4096", False, "plain set"),
|
||||
("cli -s .video0.bitrate: 4000", True, "the movols trailing colon"),
|
||||
("cli -s .osd.size: 0.8", True, "trailing colon, float value"),
|
||||
("cli -g .video0.size", False, "get"),
|
||||
("cli -d .outgoing.server", False, "delete"),
|
||||
("cli -s .isp.sensorConfig /etc/sensors/imx335.ini", False, "path-like value"),
|
||||
('cli -g ".$1"', False, "runtime path is not decidable"),
|
||||
("cli -s .nightMode.irCutPin1 11", False, "digits in a component"),
|
||||
("cli -s .outgoing.0 rtmp://a/b", False, "sequence index"),
|
||||
("cli -s .video0..fps 20", True, "empty component"),
|
||||
("cli -s .video0. 20", True, "trailing separator"),
|
||||
("cli -s .video0.fps=20 x", True, "= is not a separator"),
|
||||
("yaml-cli -i /etc/wfb.yaml -s .wireless.txpower 20", False, "explicit -i first"),
|
||||
("# cli -s .video0.bitrate: 4000", False, "commented out"),
|
||||
("sensor_cli -s .sensor.width 1920", False, "sensor_cli applet"),
|
||||
]
|
||||
rc = 0
|
||||
for line, want_bad, name in cases:
|
||||
got_bad = bool(findings_in(line))
|
||||
if got_bad != want_bad:
|
||||
verb = "flag" if want_bad else "accept"
|
||||
print(f"FAIL self-test: expected to {verb} {name}: {line!r}")
|
||||
rc = 1
|
||||
else:
|
||||
print(f"ok self-test: {name}")
|
||||
return rc
|
||||
|
||||
|
||||
def main():
|
||||
if "--self-test" in sys.argv[1:]:
|
||||
return self_test()
|
||||
root = sys.argv[1] if len(sys.argv) > 1 else "."
|
||||
return check_tree(root)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -16,6 +16,7 @@ on:
|
||||
paths:
|
||||
- '.github/workflows/**'
|
||||
- '.github/scripts/lint-workflow-shell.py'
|
||||
- '.github/scripts/lint-cli-paths.py'
|
||||
workflow_dispatch:
|
||||
|
||||
# Reads the tree and reports; writes nothing. Declared rather than inherited so
|
||||
@@ -72,3 +73,12 @@ jobs:
|
||||
# by commits in that repository and not by anything in a PR here.
|
||||
- name: Check the drift checker still describes the tree
|
||||
run: python3 .github/scripts/check-firmware-drift.py --self-test
|
||||
# `cli -s` cannot fail: yaml-cli stores whatever dotted path it is given,
|
||||
# and majestic silently ignores a key it does not recognise. So a typo in
|
||||
# a setting path applies nothing and says nothing, for the life of the
|
||||
# device. t40_lite_movols-mo-805p shipped six of them behind a trailing
|
||||
# colon. Same shape as the checks above -- no runners, no matrix, seconds.
|
||||
- name: Check the cli path linter still catches what it should
|
||||
run: python3 .github/scripts/lint-cli-paths.py --self-test
|
||||
- name: Check every shipped cli setting path
|
||||
run: python3 .github/scripts/lint-cli-paths.py
|
||||
|
||||
@@ -12,24 +12,24 @@ fw_setenv upgrade 'https://github.com/OpenIPC/builder/releases/download/latest/t
|
||||
#
|
||||
cli -s .isp.blkCnt 4
|
||||
cli -s .video0.codec h265
|
||||
cli -s .video0.bitrate: 4000
|
||||
cli -s .video0.rcMode: avbr
|
||||
cli -s .video0.profile: high
|
||||
cli -s .video0.gopSize: 5
|
||||
cli -s .video0.gopMode: smart
|
||||
cli -s .video0.bitrate 4000
|
||||
cli -s .video0.rcMode avbr
|
||||
cli -s .video0.profile high
|
||||
cli -s .video0.gopSize 5
|
||||
cli -s .video0.gopMode smart
|
||||
cli -s .nightMode.irCutPin1 122
|
||||
cli -s .nightMode.irCutPin2 123
|
||||
cli -s .nightMode.backlightPin 119
|
||||
cli -s .osd.enabled true
|
||||
cli -s .osd.template "%d.%m.%Y %H:%M:%S"
|
||||
cli -s .osd.size: 0.8
|
||||
cli -s .osd.size 0.8
|
||||
#cli -s .audio.enabled true
|
||||
#cli -s .audio.codec aac
|
||||
#cli -s .audio.srate 48000
|
||||
#cli -s .audio.volume 95
|
||||
#cli -s .audio.gain 25
|
||||
#cli -s .audio.speakerPin 7
|
||||
#cli -s .onvif.enabled: true
|
||||
#cli -s .onvif.enabled true
|
||||
#
|
||||
# Set osmem and rmem
|
||||
#
|
||||
|
||||
Reference in New Issue
Block a user