Python Part II · Module 36 · Python for VLSI Design Review
Reviewing Tool Control: Reproducibility, Environment, Cleanup
The layer that launches the tool is the one a reviewer can test most directly and usually does not, because reading it tells you almost nothing: the code that builds a command line looks correct whether or not the command line it builds can be run again. So the review is a single action — take the cmd.sh the tool left behind, paste it into a shell, and see whether the failure comes back. Of five specimens here, one does.
Advanced25 min readPythonPart IIAutomationCode ReviewReproducibility
Part II · Module 36 · Chapter 36.3 · Reviewing Tool Control: Reproducibility, Environment, Cleanup
1. Five Properties, One of Them Testable in Seconds
PROPERTIES = [
("the command is recorded", "paste cmd.sh and run it", "seconds"),
("the environment is explicit", "read cmd.sh for exports", "seconds"),
("every call is bounded", "walk the AST for timeout=", "a minute"),
("the paths are portable", "grep for absolute and ~ paths", "a minute"),
("the run dir survives failure", "make it fail, then look", "a minute"),
]
print(f"{'property':<32} {'how a reviewer checks it':<28} cost")
for what, how, cost in PROPERTIES:
print(f"{what:<32} {how:<28} {cost}")
print()
print("properties :", len(PROPERTIES))
print("checkable without reading code :", 3)
print("the first one subsumes ")
print(" two of the others :", "the cd and the exports")
print()
print("so the review's first action is :", "run cmd.sh")property how a reviewer checks it cost
the command is recorded paste cmd.sh and run it seconds
the environment is explicit read cmd.sh for exports seconds
every call is bounded walk the AST for timeout= a minute
the paths are portable grep for absolute and ~ paths a minute
the run dir survives failure make it fail, then look a minute
properties : 5
checkable without reading code : 3
the first one subsumes
two of the others : the cd and the exports
so the review's first action is : run cmd.shRunning cmd.sh tests three of the five properties at once, because a
command that reproduces has necessarily recorded its directory and its
environment. That is why the annotation names it as the single action.
2. The Annotation
# can a reviewer paste cmd.sh and reproduce the failure?WHO = [
("the author, on their machine", "almost always", "their cwd, their env"),
("the author, a week later", "often", "the env may have moved"),
("a reviewer, on their machine", "rarely", "different cwd and env"),
("CI, from a clean checkout", "the real test", "nothing is set"),
]
print(f"{'who tries to reproduce':<32} {'succeeds':<16} what they have")
for who, succeeds, have in WHO:
print(f"{who:<32} {succeeds:<16} {have}")
print()
print("the author is the worst tester of this:",
"their environment is already correct")
print("the reviewer is a good proxy for :", "CI and for the next owner")
print()
print("so 'it works for me' means :", "nothing about cmd.sh")
print("and the review is :", "the first honest test")who tries to reproduce succeeds what they have
the author, on their machine almost always their cwd, their env
the author, a week later often the env may have moved
a reviewer, on their machine rarely different cwd and env
CI, from a clean checkout the real test nothing is set
the author is the worst tester of this: their environment is already correct
the reviewer is a good proxy for : CI and for the next owner
so 'it works for me' means : nothing about cmd.sh
and the review is : the first honest testThe author is the worst possible tester of reproducibility because their
environment is already correct — every variable set, every path relative to a
directory they happen to be in. A reviewer on a different machine is the first
person for whom cmd.sh has to actually work, which makes the review the
earliest honest test and not a formality.
3. Five Specimens, One Reproduces
import os
import shlex
import subprocess
import sys
from pathlib import Path
# The "tool" the specimens invoke. Its exit codes say WHY it failed, so a
# failed reproduction is diagnosable rather than just unsuccessful.
TOOL = '''import os
import sys
args = sys.argv[1:]
if os.environ.get("FLOW_MODE", "") != "strict":
print("MODE NOT SET")
sys.exit(4)
if not os.path.isfile("rtl/files.f"):
print("NO FILELIST")
sys.exit(3)
if "--label" not in args:
print("NO LABEL")
sys.exit(2)
label = args[args.index("--label") + 1]
if label != "nightly run":
print(f"BAD LABEL {label!r}")
sys.exit(2)
print("REPRODUCED")
sys.exit(1)
'''
run_dir = Path("run")
(run_dir / "rtl").mkdir(parents=True, exist_ok=True)
(run_dir / "rtl" / "files.f").write_text("rtl/a.v\n", encoding="utf-8")
(run_dir / "tool.py").write_text(TOOL, encoding="utf-8")
PY = shlex.quote(sys.executable)
HERE = shlex.quote(str(run_dir.resolve()))
LABEL = shlex.quote("nightly run")
# The TOOL path is absolute in every specimen, so the only thing `cd` affects
# is how the tool resolves its own inputs — which is the defect under review.
TOOL_PATH = shlex.quote(str((run_dir / "tool.py").resolve()))
SPECIMENS = {
"complete": f'#!/bin/sh\ncd {HERE}\nexport FLOW_MODE=strict\n'
f'{PY} {TOOL_PATH} --label {LABEL}\n',
"no cd": f'#!/bin/sh\nexport FLOW_MODE=strict\n'
f'{PY} {TOOL_PATH} --label {LABEL}\n',
"no exports": f'#!/bin/sh\ncd {HERE}\n'
f'{PY} {TOOL_PATH} --label {LABEL}\n',
"unquoted arg": f'#!/bin/sh\ncd {HERE}\nexport FLOW_MODE=strict\n'
f'{PY} {TOOL_PATH} --label nightly run\n',
"bare python": f'#!/bin/sh\ncd {HERE}\nexport FLOW_MODE=strict\n'
f'python {TOOL_PATH} --label {LABEL}\n',
}
print(f"{'cmd.sh specimen':<16} {'rc':>4} {'first line':<24} reproduces?")
reproduced = []
for name, body in SPECIMENS.items():
Path("cmd.sh").write_text(body, encoding="utf-8")
os.chmod("cmd.sh", 0o755)
r = subprocess.run(["/bin/sh", "cmd.sh"], stdout=subprocess.PIPE,
stderr=subprocess.STDOUT, text=True)
first = (r.stdout.strip().splitlines() or ["(nothing)"])[0]
ok = r.returncode == 1 and "REPRODUCED" in r.stdout
if ok:
reproduced.append(name)
print(f"{name:<16} {r.returncode:>4} {first[:24]:<24} {ok}")
print()
print("specimens :", len(SPECIMENS))
print("that reproduce:", len(reproduced), reproduced)
print()
print("every failure has a DIFFERENT exit code:", True)
print("so a failed reproduction is :", "diagnosable")cmd.sh specimen rc first line reproduces?
complete 1 REPRODUCED True
no cd 3 NO FILELIST False
no exports 4 MODE NOT SET False
unquoted arg 2 BAD LABEL 'nightly' False
bare python 127 cmd.sh: line 4: python: False
specimens : 5
that reproduce: 1 ['complete']
every failure has a DIFFERENT exit code: True
so a failed reproduction is : diagnosableOne of five reproduces, and each of the four failures is distinguishable. That property is not accidental: the specimen tool was written to exit a different code for each precondition, which is Chapter 33.8's contract applied to a diagnostic. A tool that exits 1 for everything would make all four failures look like "it did not reproduce" and a reviewer could not say why.
Each failure is worth naming because the review comment differs:
4. Four Ways a cmd.sh Fails
FAILURES = [
("no cd", 3, "NO FILELIST",
"entries resolve against the reviewer's cwd — 35.7 §4",
"record `cd` to the run dir"),
("no exports", 4, "MODE NOT SET",
"the tool read a variable the script never recorded — 34.5",
"export every variable the tool reads"),
("unquoted arg", 2, "BAD LABEL 'nightly'",
"a space split one argument into two",
"build the line with shlex.quote — 23.1 §6"),
("bare python", 127, "command not found",
"`python` does not exist here — 32.3 §3",
"record sys.executable, not 'python'"),
]
print(f"{'specimen':<14} {'rc':>4} {'symptom':<22} review comment")
for name, rc, symptom, _, fix in FAILURES:
print(f"{name:<14} {rc:>4} {symptom:<22} {fix}")
print()
print("the four causes:")
for name, _, _, cause, _ in FAILURES:
print(f" {name:<14} {cause}")
print()
print("the subtlest one :", "unquoted arg")
print("because the command ")
print(" LOOKS right :", "--label nightly run")
print("and the tool reports :", "BAD LABEL 'nightly'")
print()
print("rc 127 is produced by :", "the shell, not the tool")
print("which distinguishes :",
"a missing interpreter from a tool that ran and refused")specimen rc symptom review comment
no cd 3 NO FILELIST record `cd` to the run dir
no exports 4 MODE NOT SET export every variable the tool reads
unquoted arg 2 BAD LABEL 'nightly' build the line with shlex.quote — 23.1 §6
bare python 127 command not found record sys.executable, not 'python'
the four causes:
no cd entries resolve against the reviewer's cwd — 35.7 §4
no exports the tool read a variable the script never recorded — 34.5
unquoted arg a space split one argument into two
bare python `python` does not exist here — 32.3 §3
the subtlest one : unquoted arg
because the command
LOOKS right : --label nightly run
and the tool reports : BAD LABEL 'nightly'
rc 127 is produced by : the shell, not the tool
which distinguishes : a missing interpreter from a tool that ran and refusedThe unquoted argument is the subtlest because the recorded line reads
correctly to a human — --label nightly run is what the author typed — and the
shell splits it into two words, so the tool receives nightly and an
unexpected run. Chapter 23.1 Section 6's shlex.quote exists for exactly
this, and a reviewer can spot it by looking for any recorded argument containing
a space without quotes around it.
rc 127 comes from the shell, not from the tool, which is a useful
distinction when triaging a failed reproduction: 127 means the command was never
started, so the problem is the recorded interpreter or PATH, and any other
code means the tool ran and declined.
5. The Environment the Tool Actually Reads
import ast
SPECIMEN = '''import os
import subprocess
from pathlib import Path
def run(test, seed):
sim = os.environ["FLOW_SIM"]
root = Path(os.environ.get("FLOW_RUN_ROOT", "runs"))
extra = os.environ.get("FLOW_SIM_ARGS", "").split()
os.environ["UVM_HOME"] = "/opt/uvm-1.2"
return subprocess.run([sim, *extra], cwd=root,
env={**os.environ, "SEED": str(seed)})
'''
RECORDED = {"FLOW_SIM", "FLOW_RUN_ROOT"} # what cmd.sh exports
def env_reads(tree):
reads, writes = set(), set()
for n in ast.walk(tree):
if isinstance(n, ast.Subscript) and "environ" in ast.dump(n.value) \
and isinstance(n.slice, ast.Constant):
if isinstance(n.ctx, ast.Store):
writes.add(n.slice.value)
else:
reads.add(n.slice.value)
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) \
and n.func.attr == "get" and "environ" in ast.dump(n.func) \
and n.args and isinstance(n.args[0], ast.Constant):
reads.add(n.args[0].value)
return reads, writes
reads, writes = env_reads(ast.parse(SPECIMEN))
print("variables the tool READS :", sorted(reads))
print("variables the tool WRITES:", sorted(writes))
print("variables cmd.sh RECORDS :", sorted(RECORDED))
print()
print("read but not recorded :", sorted(reads - RECORDED))
print("recorded but not read :", sorted(RECORDED - reads))
print()
print("the unrecorded one :", "FLOW_SIM_ARGS")
print("its default is :", '"" — so it is silently empty — 34.5 §3')
print("what a reviewer sees in ")
print(" cmd.sh :", "nothing at all about it")
print()
print("and the variable the tool WRITES:", sorted(writes))
print("which cmd.sh need not record :", "the tool sets it itself")variables the tool READS : ['FLOW_RUN_ROOT', 'FLOW_SIM', 'FLOW_SIM_ARGS']
variables the tool WRITES: ['UVM_HOME']
variables cmd.sh RECORDS : ['FLOW_RUN_ROOT', 'FLOW_SIM']
read but not recorded : ['FLOW_SIM_ARGS']
recorded but not read : []
the unrecorded one : FLOW_SIM_ARGS
its default is : "" — so it is silently empty — 34.5 §3
what a reviewer sees in
cmd.sh : nothing at all about it
and the variable the tool WRITES: ['UVM_HOME']
which cmd.sh need not record : the tool sets it itselfOne variable is read and not recorded, and it is the dangerous kind.
FLOW_SIM_ARGS defaults to the empty string, so a run with it set and a
reproduction without it both work — and produce different simulations. Chapter
34.5 Section 3 measured that os.environ.get(name, "") treats set-but-empty as
a value; here the same shape makes a reproduction silently incomplete.
The read/write distinction is worth making in a review. UVM_HOME is set by
the tool, so cmd.sh does not need to record it — recording it would be
harmless but would also hide the fact that the tool controls it, and a reviewer
who sees it exported might reasonably assume the site sets it.
6. Is Every Call Bounded?
import ast
import subprocess
print("=== what boundedness buys")
try:
subprocess.run(["sleep", "5"], timeout=0.2)
print(" timeout=0.2 on `sleep 5`: returned normally")
except subprocess.TimeoutExpired:
print(" timeout=0.2 on `sleep 5`: TimeoutExpired raised")
print(" with no timeout at all :",
"the call returns when the tool decides, or never")
SPECIMEN = '''import subprocess
def build(sources):
return subprocess.run(["iverilog", "-o", "a.vvp"] + sources, timeout=600)
def simulate(vvp):
return subprocess.run(["vvp", vvp])
def lint(path):
return subprocess.check_output(["lint", path])
def legacy(cmd):
return subprocess.call(cmd, shell=True)
'''
RUNNERS = {"run", "call", "check_output", "check_call", "Popen"}
def audit_calls(src):
rows = []
for n in ast.walk(ast.parse(src)):
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) \
and n.func.attr in RUNNERS:
kw = {k.arg for k in n.keywords}
rows.append({"call": f"subprocess.{n.func.attr}",
"line": n.lineno,
"bounded": "timeout" in kw,
"shell": "shell" in kw})
return rows
rows = audit_calls(SPECIMEN)
print()
print(f"{'call':<24} {'line':>5} {'bounded':>9} {'shell=True':>11}")
for r in rows:
print(f"{r['call']:<24} {r['line']:>5} {str(r['bounded']):>9} "
f"{str(r['shell']):>11}")
print()
print("tool invocations :", len(rows))
print("bounded :", sum(1 for r in rows if r["bounded"]))
print("UNBOUNDED :", sum(1 for r in rows if not r["bounded"]))
print("with shell=True :", sum(1 for r in rows if r["shell"]))
print()
print("the unbounded simulate() means:", "a hung simulation hangs the flow")
print("and 33.12's pre-flight cannot ")
print(" help because :", "the hang happens during the run")=== what boundedness buys
timeout=0.2 on `sleep 5`: TimeoutExpired raised
with no timeout at all : the call returns when the tool decides, or never
call line bounded shell=True
subprocess.run 5 True False
subprocess.run 9 False False
subprocess.check_output 13 False False
subprocess.call 17 False True
tool invocations : 4
bounded : 1
UNBOUNDED : 3
with shell=True : 1
the unbounded simulate() means: a hung simulation hangs the flow
and 33.12's pre-flight cannot
help because : the hang happens during the runOne of four calls is bounded. The audit is six lines of ast and produces a
finding per call site with a line number, which is the form a review comment
wants — and the unbounded simulate() is the one that matters, because a
simulation is exactly the call that can hang.
The shell=True on the last row is a second finding from the same walk. It
means the command is a string the shell will re-parse, so Chapter 23.1's
quoting problem returns and a filename containing a space or a ; changes what
runs. A reviewer finding both on one line — unbounded and shell=True —
has found the highest-risk invocation in the file without reading what it does.
7. Does the Run Directory Survive a Failure?
import shutil
from pathlib import Path
SCRATCH = {"obj", "work", "csrc"}
EVIDENCE_SUFFIX = {".log", ".sh", ".json"}
def build_run_dir(name):
d = Path(name)
for sub in SCRATCH:
(d / sub).mkdir(parents=True, exist_ok=True)
(d / sub / "big.tmp").write_text("x", encoding="utf-8")
d.mkdir(parents=True, exist_ok=True)
for f in ("sim.log", "compile.log", "cmd.sh", "results.json",
"scratch.tmp"):
(d / f).write_text("x", encoding="utf-8")
return d
def cleanup_everything(d):
"""Specimen A: tidy, and useless after a failure."""
shutil.rmtree(d)
def cleanup_nothing(d):
"""Specimen B: keeps everything, including gigabytes of scratch."""
return
def cleanup_selective(d):
"""Specimen C: 33.13's rule."""
for child in sorted(Path(d).iterdir()):
if child.is_dir() and child.name in SCRATCH:
shutil.rmtree(child)
elif child.is_file() and child.suffix not in EVIDENCE_SUFFIX:
child.unlink()
SPECIMENS = [("remove the run dir", cleanup_everything),
("keep everything", cleanup_nothing),
("remove scratch only", cleanup_selective)]
NEEDED = {"cmd.sh", "sim.log", "compile.log"}
print(f"{'cleanup specimen':<22} {'dir exists':<12} {'kept':<34} usable?")
for label, fn in SPECIMENS:
d = build_run_dir(f"run_{label.split()[0]}")
fn(d)
exists = d.exists()
kept = sorted(p.name for p in d.iterdir() if p.is_file()) if exists else []
usable = NEEDED <= set(kept)
print(f"{label:<22} {str(exists):<12} {str(kept)[:34]:<34} {usable}")
print()
print("what a reviewer needs after a failure:", sorted(NEEDED))
print("specimens that provide it :", 1)
print()
print("specimen A is :", "tidy and unusable")
print("specimen B is :", "usable and fills the volume")
print("specimen C is :", "33.13 §7's classification")cleanup specimen dir exists kept usable?
remove the run dir False [] False
keep everything True ['cmd.sh', 'compile.log', 'results True
remove scratch only True ['cmd.sh', 'compile.log', 'results True
what a reviewer needs after a failure: ['cmd.sh', 'compile.log', 'sim.log']
specimens that provide it : 1
specimen A is : tidy and unusable
specimen B is : usable and fills the volume
specimen C is : 33.13 §7's classificationTwo of three cleanups leave a usable run directory and one removes it
entirely. Specimen A is the one to flag: shutil.rmtree(run_dir) in a
finally is the tidiest-looking line in a flow and it deletes the only evidence
a failure produced.
The difference between B and C is not usability but volume — both keep what a reviewer needs, and only C removes the scratch. So the review has two separate questions here, and conflating them is how a team ends up with A: is the evidence kept is about correctness, and is the scratch removed is about disk, and the answer to the first must never be traded for the second.
8. The Tool-Control Review, Assembled
import ast
from dataclasses import dataclass
from pathlib import Path
RUNNERS = {"run", "call", "check_output", "check_call", "Popen"}
@dataclass(frozen=True)
class Finding:
area: str
severity: str
detail: str
def review_tool_control(src, cmd_sh):
findings = []
tree = ast.parse(src)
# --- environment: read against recorded
reads = set()
for n in ast.walk(tree):
if isinstance(n, ast.Subscript) and "environ" in ast.dump(n.value) \
and isinstance(n.slice, ast.Constant) \
and not isinstance(n.ctx, ast.Store):
reads.add(n.slice.value)
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) \
and n.func.attr == "get" and "environ" in ast.dump(n.func) \
and n.args and isinstance(n.args[0], ast.Constant):
reads.add(n.args[0].value)
recorded = {line.split("=", 1)[0].removeprefix("export ").strip()
for line in cmd_sh.splitlines()
if line.startswith("export ")}
for name in sorted(reads - recorded):
findings.append(Finding("environment", "major",
f"${name} is read and not exported by cmd.sh"))
# --- boundedness and shell
for n in ast.walk(tree):
if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute) \
and n.func.attr in RUNNERS:
kw = {k.arg for k in n.keywords}
if "timeout" not in kw:
findings.append(Finding(
"boundedness", "major",
f"line {n.lineno}: subprocess.{n.func.attr} has no "
f"timeout"))
if "shell" in kw:
findings.append(Finding(
"quoting", "major",
f"line {n.lineno}: shell=True re-parses the command"))
# --- cmd.sh contents
if "cd " not in cmd_sh:
findings.append(Finding("reproducibility", "blocker",
"cmd.sh has no `cd`: it depends on the cwd"))
for line in cmd_sh.splitlines():
if line.startswith("python ") or " python " in line:
findings.append(Finding("reproducibility", "blocker",
"cmd.sh invokes bare `python` — 32.3 §3"))
# an argument with a space and no quoting
for line in cmd_sh.splitlines():
if line.startswith(("#", "cd ", "export ")):
continue
if "--label" in line and "'" not in line and '"' not in line:
findings.append(Finding("quoting", "blocker",
"an argument with a space is unquoted"))
# --- portability
for n in ast.walk(tree):
if isinstance(n, ast.Constant) and isinstance(n.value, str):
if n.value.startswith("/") and n.value.count("/") > 1:
findings.append(Finding("portability", "minor",
f"absolute path literal "
f"{n.value!r}"))
if n.value.startswith("~"):
findings.append(Finding("portability", "major",
f"per-user path {n.value!r}"))
return findings
SPECIMEN_SRC = '''import os
import subprocess
from pathlib import Path
ROOT = "/proj/chip_a/rtl"
def run(test, seed):
sim = os.environ["FLOW_SIM"]
extra = os.environ.get("FLOW_SIM_ARGS", "").split()
subprocess.run([sim, *extra, test])
subprocess.call(f"grep -c UVM_ERROR {test}.log", shell=True)
'''
SPECIMEN_CMD = ("#!/bin/sh\n"
"export FLOW_SIM=iverilog\n"
"python tool.py --label nightly run\n")
findings = review_tool_control(SPECIMEN_SRC, SPECIMEN_CMD)
ORDER = {"blocker": 0, "major": 1, "minor": 2}
print("tool-control review")
for f in sorted(findings, key=lambda f: (ORDER[f.severity], f.area, f.detail)):
print(f" [{f.severity:<7}] {f.area:<16} {f.detail}")
print()
print("findings :", len(findings))
for sev in ("blocker", "major", "minor"):
print(f" {sev:<8} {sum(1 for f in findings if f.severity == sev)}")
print()
print("areas covered:", sorted({f.area for f in findings}))
print("lines of specimen:", len(SPECIMEN_SRC.strip().splitlines()))tool-control review
[blocker] quoting an argument with a space is unquoted
[blocker] reproducibility cmd.sh has no `cd`: it depends on the cwd
[blocker] reproducibility cmd.sh invokes bare `python` — 32.3 §3
[major ] boundedness line 11: subprocess.run has no timeout
[major ] boundedness line 12: subprocess.call has no timeout
[major ] environment $FLOW_SIM_ARGS is read and not exported by cmd.sh
[major ] quoting line 12: shell=True re-parses the command
[minor ] portability absolute path literal '/proj/chip_a/rtl'
findings : 8
blocker 3
major 4
minor 1
areas covered: ['boundedness', 'environment', 'portability', 'quoting', 'reproducibility']
lines of specimen: 12Eight findings in eleven lines, three of them blockers. The three blockers
are all in cmd.sh rather than in the code, which is the chapter's point: the
most serious defects in tool control are in the artefact that records what
happened, and no amount of reading the invocation code finds them.
Note that line 11 produces two findings — unbounded and shell=True — and
the report lists them separately rather than merging. A reviewer fixes them
differently, so a combined comment would get half-addressed.
9. Are the Paths Portable?
The fourth property in Section 1 got one minor finding in Section 8, and it deserves a pass of its own, because path literals are not one defect class but four, with different severities:
import ast
from collections import Counter
SPECIMEN = "\n".join([
"import os",
"from pathlib import Path",
"",
'SIM = "/tools/cadence/xcelium/19.09/bin/xrun"',
'SCRATCH = "/proj/chip_a/users/azhar/runs"',
'BASELINE = "~/.flow/baseline.json"',
'FILELIST = "rtl/files.f"',
'LOG = os.environ.get("FLOW_RUN_ROOT", "runs") + "/sim.log"',
])
def classify_path(value):
"""Four classes, three severities. Most specific test first."""
if value.startswith("~"):
return "per-user", "blocker", "depends on who ran it"
if value.startswith("/") and any(c.isdigit() for c in value) \
and value.count("/") > 3:
return "versioned tool", "blocker", "breaks on upgrade day"
if value.startswith("/"):
return "absolute", "major", "one machine's layout"
if "/" in value:
return "relative", "minor", "needs a declared root"
return None, None, None
rows = []
for node in ast.walk(ast.parse(SPECIMEN)):
if isinstance(node, ast.Constant) and isinstance(node.value, str):
cls, sev, why = classify_path(node.value)
if cls:
rows.append((node.value, cls, sev, why))
print(f"{'literal':<38} {'class':<15} {'severity':<8} why")
for value, cls, sev, why in rows:
print(f"{value:<38} {cls:<15} {sev:<8} {why}")
print()
print("path literals found :", len(rows))
for cls, n in sorted(Counter(r[1] for r in rows).items()):
print(f" {cls:<15} {n}")
print("portable as written :", 0)literal class severity why
/tools/cadence/xcelium/19.09/bin/xrun versioned tool blocker breaks on upgrade day
/proj/chip_a/users/azhar/runs absolute major one machine's layout
~/.flow/baseline.json per-user blocker depends on who ran it
rtl/files.f relative minor needs a declared root
/sim.log absolute major one machine's layout
path literals found : 5
absolute 2
per-user 1
relative 1
versioned tool 1
portable as written : 0Five literals, four classes, none portable as written — and the last row is a false positive worth keeping rather than quietly tidying away.
/sim.log is not a path. It is the right-hand operand of
os.environ.get("FLOW_RUN_ROOT", "runs") + "/sim.log" — a concatenation
fragment that merely begins with a slash, which is exactly the over-count
Chapter 34.1 Section 3 measured when a naive scan reported eleven distinct paths
where seven were real. This audit inherits that flaw honestly: it looks at
string constants without looking at what they are joined to.
That is acceptable in a review instrument as long as the reviewer knows it. Four
of the five findings are real, and the fifth is a prompt to look at the last
line — which is worth looking at anyway, because that line does path arithmetic
with + instead of /:
import os
from pathlib import Path
os.environ.pop("FLOW_RUN_ROOT", None)
root = os.environ.get("FLOW_RUN_ROOT", "runs")
print("without a trailing slash")
print(" string + :", repr(root + "/sim.log"))
print(" Path / :", repr(str(Path(root) / "sim.log")))
os.environ["FLOW_RUN_ROOT"] = "runs/"
root = os.environ["FLOW_RUN_ROOT"]
joined = root + "/sim.log"
paved = str(Path(root) / "sim.log")
print()
print("with a trailing slash in the variable")
print(" string + :", repr(joined))
print(" Path / :", repr(paved))
del os.environ["FLOW_RUN_ROOT"]
print()
print("the string form doubles the slash:", "//" in joined)
print("the Path form does not :", "//" not in paved)
print("the two spellings are equal :", joined == paved)
print("and they name one file :", True)without a trailing slash
string + : 'runs/sim.log'
Path / : 'runs/sim.log'
with a trailing slash in the variable
string + : 'runs//sim.log'
Path / : 'runs/sim.log'
the string form doubles the slash: True
the Path form does not : True
the two spellings are equal : False
and they name one file : TrueA trailing slash in the variable turns + into runs//sim.log where /
gives runs/sim.log. Nothing fails: the doubled slash opens the same file on
POSIX. The two strings are simply unequal while naming one file, which is
precisely the duplicate Chapter 35.7 Section 7 measured — resolve() collapses
., .. and //, and a checker comparing raw strings does not.
So the review comment on that line is not about style. Joining with + produces
values that compare unequal while being equal, and the flow's own duplicate
detection goes blind to them.
10. Predict, Run, Observe
import os
import shlex
import subprocess
import sys
from pathlib import Path
Path("d").mkdir(exist_ok=True)
(Path("d") / "marker").write_text("here\n", encoding="utf-8")
# A
Path("a.sh").write_text("#!/bin/sh\ncat marker\n", encoding="utf-8")
r = subprocess.run(["/bin/sh", "a.sh"], stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL, text=True)
print("A:", r.returncode, repr(r.stdout.strip()))
# B
Path("b.sh").write_text(f"#!/bin/sh\ncd {shlex.quote('d')}\ncat marker\n",
encoding="utf-8")
r = subprocess.run(["/bin/sh", "b.sh"], stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL, text=True)
print("B:", r.returncode, repr(r.stdout.strip()))
# C
Path("c.sh").write_text("#!/bin/sh\npython -c 'print(1)'\n", encoding="utf-8")
r = subprocess.run(["/bin/sh", "c.sh"], stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL)
print("C:", r.returncode)
# D
try:
subprocess.run(["sleep", "5"], timeout=0.2)
print("D:", "returned")
except subprocess.TimeoutExpired:
print("D:", "TimeoutExpired")The verified output:
A: 1 ''
B: 0 'here'
C: 127
D: TimeoutExpiredA — no cd, so cat marker fails: the file is in d/ and the script runs
where it was invoked. That is the "no cd" specimen in two values.
B — with the cd, the same command works. One line is the whole difference
between a cmd.sh that reproduces and one that does not.
C — bare python gives 127, the shell's command-not-found. Chapter 32.3
Section 3 established that python does not exist on this machine, and a
cmd.sh recording it is unrunnable here no matter how correct the rest is.
D — timeout=0.2 raises TimeoutExpired rather than waiting. The exception
is the point: a bounded call fails in a way the caller can handle, and an
unbounded one has no failure mode at all, which is why "it has never hung" is
not evidence.
11. Exercises
Exercise 1 — five properties. Check each one for a script your team owns.
Exercise 2 — three at once. Explain why running cmd.sh tests more than
one property.
Exercise 3 — the author is the worst tester. Say why, in one sentence.
Exercise 4 — be the reviewer. Take a colleague's cmd.sh and run it on your
machine.
Exercise 5 — diagnosable failures. Make your tool exit a different code per precondition.
Exercise 6 — no cd. Delete the cd from a working cmd.sh and record what
breaks.
Exercise 7 — no exports. Do the same for one export and record the
symptom.
Exercise 8 — the unquoted argument. Put a space in a label and see how the tool receives it.
Exercise 9 — bare python. Confirm what python does on your machine and
what rc you get.
Exercise 10 — rc 127. Explain what it tells you that any other code does not.
Exercise 11 — check for absences. Write the four-item presence list for
cmd.sh.
Exercise 12 — compose the two reviews. Use Chapter 36.1's input audit to
produce the variable list, then check cmd.sh against it.
Exercise 13 — read against recorded. Run the comparison on your own tool.
Exercise 14 — the silently empty variable. Find one with an empty default and say what a reproduction without it produces.
Exercise 15 — read or written. Separate the variables your tool sets from the ones it consumes.
Exercise 16 — audit boundedness. Walk your own source for subprocess
calls and count the bounded ones.
Exercise 17 — the call that can hang. Identify it and say what a timeout would do.
Exercise 18 — shell=True. Find one and describe what a filename with a space does.
Exercise 19 — the highest-risk line. Find a call that is both unbounded and
shell=True.
Exercise 20 — three cleanups. Implement all three and check which leave a usable directory.
Exercise 21 — tidy and unusable. Search your flow for an rmtree of a run
directory.
Exercise 22 — two separate questions. State them, and say why the answers must not be traded.
Exercise 23 — eight findings. Run the assembled review on one of your scripts.
Exercise 24 — where the blockers were. Note which artefact they were in.
Exercise 25 — two findings on one line. Explain why they are reported separately.
Exercise 26 — it has never hung. Explain why that is not evidence of boundedness.
Exercise 27 — four classes. Classify every path literal in one of your own tools.
Exercise 28 — the versioned one. Find a tool path with a version in it and describe the upgrade-day failure.
Exercise 29 — per-user before absolute. Explain why the Callout ranks them that way.
Exercise 30 — the relative one. Say what it needs before it means anything.
Exercise 31 — the false positive. Find a concatenation fragment your audit reports as a path.
Exercise 32 — follow it anyway. Explain why that finding is still worth acting on.
Exercise 33 — + against /. Join a path both ways with a trailing slash in the variable.
Exercise 34 — the invisible duplicate. Say what a doubled slash does to a string-comparing duplicate check, and name the chapter that measured it.
12. Summary
- Running
cmd.shtests three of the five properties at once, because a command that reproduces has necessarily recorded its directory and environment. - The author is the worst tester of reproducibility — their environment is already correct — so the review is the first honest test and a proxy for CI.
- One of five
cmd.shspecimens reproduced, and each of the four failures had a different exit code because the specimen tool exits a distinct code per precondition. - No
cdmakes entries resolve against the reviewer's working directory — Chapter 35.7 Section 4's finding arriving in the recorded command. - No
exportleaves the tool reading a variable nobody recorded. - An unquoted argument is the subtlest defect:
--label nightly runreads correctly and the shell splits it, so the tool receivesnightly. - Bare
pythongives rc 127 from the shell, which distinguishes a command that never started from a tool that ran and declined. - Review
cmd.shfor what it does not contain. All four defects are absences, and absences are what reading misses. - One variable read and not recorded is the dangerous kind when its default
is
"": the run and the reproduction both work and simulate differently. - Separate the variables the tool sets from the ones it reads — recording the first hides the fact that the tool controls it.
- One of four
subprocesscalls was bounded. The audit is six lines ofastand names a line number per finding. - Unbounded plus
shell=Trueon one line is the highest-risk invocation in a file, findable without reading what it does. - Two of three cleanups left a usable run directory and one removed it.
shutil.rmtree(run_dir)is the tidiest-looking line in a flow and deletes the only evidence a failure produced. - "Is the evidence kept" and "is the scratch removed" are separate questions, and the first must never be traded for the second.
- Eight findings in eleven lines, and all three blockers were in
cmd.shrather than in the code. - "It has never hung" is not evidence of boundedness — an unbounded call has
- Path literals are four classes with three severities: per-user and versioned-tool are blockers, absolute is major, relative is minor and needs a declared root.
- None of the five literals in the specimen was portable as written.
- Severity is about who is blocked: an absolute path is wrong for everyone identically, a per-user path is wrong differently for each person — so it works for its author and reads the wrong baseline for everyone else.
- One finding was a false positive —
/sim.logis a concatenation fragment, the same over-count Chapter 34.1 Section 3 measured — and it is still worth following, because the line it points at joins paths with+. +givesruns//sim.logwhere/givesruns/sim.log. Both open one file and the strings are unequal, which is exactly the duplicate Chapter 35.7 Section 7's string comparison cannot see. no failure mode to observe.
The next chapter reviews the system that calls all of this many times at once, where the question stops being about one run and becomes whether every job is accounted for.
Where this fits
Part of the Python curriculum.
