Skip to content

Commit 6c5350a

Browse files
redsun82Copilot
andcommitted
Just: refine the CodeQL test runner's argument handling
`--extra-check` had grown to also select which checks to run at all, so a language wanting a single non-default check enabled had no way to say so without also asking for the others; splits it into `--extra-check=` for adding one and `--all-checks` for running everything, leaving each option meaning one thing. The three options private to the runner (target, checks, extra checks) were parsed apart from the CodeQL CLI arguments they are mixed in with, then reassembled by hand into a dataclass alongside the parsed remainder; argparse now owns all three itself, removing that reassembly along with a class that existed only to hold it. Along the way: an argument list containing only blanks is treated as empty, matching how the tooling handles that elsewhere; passing an empty value later in a merged list now overrides an earlier non-empty one, since a later assignment doing nothing was surprising and made per-language justfiles no longer resettable field by field; and the test runner's own file is brought under `black`, having been the one file here that was not. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 4e31df4 commit 6c5350a

19 files changed

Lines changed: 281 additions & 175 deletions

File tree

‎actions/ql/test/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ base_flags := []
88
all_checks := default_db_checks
99

1010
[no-cd]
11-
test *ARGS=".": (_codeql_test "actions" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
11+
test *ARGS=".": (_codeql_test "actions" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎cpp/ql/test/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ base_flags := ['--include-location-in-star']
88
all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-unused-labels', '--consistency-queries=' + consistency_queries]
99

1010
[no-cd]
11-
test *ARGS=".": (_codeql_test "cpp" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
11+
test *ARGS=".": (_codeql_test "cpp" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎csharp/ql/test/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ base_flags := []
88
all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--additional-packs=ql', '--consistency-queries=' + consistency_queries]
99

1010
[no-cd]
11-
test *ARGS=".": (_codeql_test "csharp" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
11+
test *ARGS=".": (_codeql_test "csharp" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎go/ql/test/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ base_flags := []
88
all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-unused-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries]
99

1010
[no-cd]
11-
test *ARGS=".": (_codeql_test "go" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
11+
test *ARGS=".": (_codeql_test "go" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎java/ql/test-kotlin1/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,4 @@ base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT=']
1010
all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries]
1111

1212
[no-cd]
13-
test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
13+
test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎java/ql/test-kotlin2/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,4 @@ base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT=', 'CODEQL_KOTLIN_LEGAC
1010
all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries]
1111

1212
[no-cd]
13-
test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
13+
test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎java/ql/test/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,4 @@ base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT=']
99
all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries]
1010

1111
[no-cd]
12-
test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
12+
test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎javascript/ql/test/justfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,4 @@ base_flags := []
88
all_checks := default_db_checks
99

1010
[no-cd]
11-
test *ARGS=".": (_codeql_test "javascript" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS))
11+
test *ARGS=".": (_codeql_test "javascript" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS))

‎misc/just/README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,8 @@ Another point is how launching QL tests can be tweaked:
158158
- you can add the additional checks that CI does with `--all-checks` or the `+`
159159
abbreviation. These additional checks are configured in justfiles per language, and
160160
correspond to all the additional checks that CI adds (but that a dev might not want to
161-
run by default).
161+
run by default). Checks a root passes unconditionally are not offers: what it offers
162+
is what `--all-checks` can enable
162163

163164
Test arguments are passed around as `just` lists (`set lists`), so they reach the
164165
underlying runner already split and arguments containing spaces survive intact.

‎misc/just/codeql_test_run.py‎

Lines changed: 99 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,17 @@
55
python3 codeql_test_run.py LANGUAGE [ARG...]
66
77
Arguments are already split by `just` (see `set lists`), so each one is taken verbatim.
8-
`--all-checks=FLAG` contributes FLAG to the set of extra checks that `--all-checks` (or
9-
its `+` abbreviation) turns on.
8+
`--extra-check=FLAG` offers FLAG as a check to run, and `--all-checks` (or its `+`
9+
abbreviation) turns the offered ones on. Per-language justfiles supply the offers and
10+
the caller supplies the switch, so the two are separate options rather than one.
1011
"""
1112

13+
import argparse
1214
import os
1315
import re
1416
import subprocess
1517
import sys
18+
import shutil
1619
from pathlib import Path
1720

1821
JUST = os.environ.get("JUST_EXECUTABLE", "just")
@@ -21,13 +24,12 @@
2124
CMD_END = os.environ.get("CMD_END", "")
2225
SEMMLE_CODE = os.environ.get("SEMMLE_CODE")
2326

24-
ALL_CHECKS_PREFIX = "--all-checks="
25-
ENV_RE = re.compile(r"^[A-Z_][A-Z_0-9]*=.*$")
27+
ENV_RE = re.compile(r"(^[A-Z_][A-Z_0-9]*)=(.*)$")
2628

2729

2830
def invoke(invocation, *, cwd=None, log_prefix=""):
2931
prefix = f"{log_prefix} " if log_prefix else ""
30-
print(f"{CMD_BEGIN}{prefix}{' '.join(invocation)}{CMD_END}")
32+
print(f"{CMD_BEGIN}{prefix}{' '.join(map(str, invocation))}{CMD_END}")
3133
try:
3234
subprocess.run(invocation, check=True, cwd=cwd)
3335
except subprocess.CalledProcessError as e:
@@ -37,114 +39,121 @@ def invoke(invocation, *, cwd=None, log_prefix=""):
3739

3840
def error(message):
3941
print(f"{ERROR}{message}", file=sys.stderr)
42+
raise SystemExit(1)
4043

4144

42-
def parse_args(args, argv):
43-
"""Sort arguments into tests, flags and environment assignments."""
44-
for arg in argv:
45-
if not arg:
46-
# an empty argument can come from a caller interpolating an unset variable
47-
continue
48-
if arg.startswith(ALL_CHECKS_PREFIX):
49-
args["all_checks"].append(arg[len(ALL_CHECKS_PREFIX) :])
50-
elif arg.startswith("--codeql="):
51-
args["codeql"] = arg.split("=", 1)[1]
52-
elif arg in ("+", "--all-checks"):
53-
args["all"] = True
54-
elif arg.startswith("-"):
55-
args["flags"].append(arg)
56-
elif ENV_RE.match(arg):
57-
args["env"].append(arg)
58-
else:
59-
args["tests"].append(arg)
45+
class _Parser(argparse.ArgumentParser):
46+
"""An `argparse` parser that fails the way the rest of this script does.
6047
48+
The default reports to `stderr` in its own format and exits 2, which would arrive
49+
in a `just` banner unprefixed and alongside a usage line naming this script rather
50+
than the recipe the caller actually typed.
51+
"""
6152

62-
def env_value(args, name, default):
63-
"""Resolve a setting from test arguments, then the environment, then a default."""
64-
for assignment in reversed(args["env"]):
65-
key, _, value = assignment.partition("=")
66-
if key == name and value:
67-
return value
68-
return os.environ.get(name) or default
53+
def error(self, message):
54+
error(message)
6955

7056

71-
def main():
72-
argv = sys.argv[1:]
73-
if not argv:
74-
error("Usage: codeql_test_run.py LANGUAGE [ARG...]")
75-
return 1
76-
77-
language, *rest = argv
78-
79-
args = {
80-
"tests": [],
81-
"flags": [],
82-
"env": [],
83-
"all_checks": [],
84-
"codeql": "build" if SEMMLE_CODE else "host",
85-
"all": False,
86-
}
87-
parse_args(args, rest)
88-
if args["all"]:
89-
parse_args(args, args["all_checks"])
90-
91-
if not SEMMLE_CODE and args["codeql"] in ("build", "built"):
92-
error(
57+
def build_parser():
58+
# `+` can be an option string only because it is also a prefix character. `-h` and
59+
# `--help` are left unclaimed so that they reach `codeql test run`.
60+
parser = _Parser(add_help=False, allow_abbrev=False, prefix_chars="-+")
61+
parser.add_argument("language")
62+
parser.add_argument("--codeql", default="build" if SEMMLE_CODE else "host")
63+
parser.add_argument("--extra-check", action="append", dest="extra_checks")
64+
parser.add_argument("--all-checks", "+", action="store_true", dest="all")
65+
parser.set_defaults(extra_checks=[], tests=[], flags=[], env={})
66+
return parser
67+
68+
69+
def parse_arguments():
70+
"""Sort a command line into the kinds that are handled differently.
71+
72+
`argparse` owns the options this script acts on itself. Everything else belongs to
73+
`codeql test run` and has to survive untouched, which is what `parse_known_args`
74+
hands back, and what is sorted by shape here: a test path and a `CPUS=4` are both
75+
positionals, told apart only by how they look.
76+
"""
77+
p = build_parser()
78+
args, rest = p.parse_known_args()
79+
if args.codeql in ("build", "built") and not SEMMLE_CODE:
80+
p.error(
9381
"Using `--codeql=build` or `--codeql=built` requires working "
9482
"with the internal repository"
9583
)
96-
return 1
9784

98-
if not args["tests"]:
99-
args["tests"].append(".")
85+
for arg in rest:
86+
if arg.startswith("-"):
87+
args.flags.append(arg)
88+
elif m := ENV_RE.match(arg):
89+
k, v = m.groups()
90+
args.env[k] = v
91+
else:
92+
args.tests.append(arg)
93+
return args
94+
95+
96+
def resolve_codeql(args: argparse.Namespace) -> Path:
97+
suffix = ".exe" if sys.platform == "win32" else ""
98+
match args.codeql:
99+
case "built" | "build":
100+
return Path(
101+
SEMMLE_CODE,
102+
"target",
103+
"intree",
104+
f"codeql-{args.language}",
105+
"codeql" + suffix,
106+
)
107+
case "host":
108+
codeql = shutil.which("codeql" + suffix)
109+
if not codeql:
110+
error("CodeQL executable not found in PATH")
111+
return Path(codeql)
112+
case _:
113+
codeql = Path(args.codeql)
114+
if codeql.is_dir():
115+
codeql /= "codeql" + suffix
116+
return codeql
117+
118+
119+
def main():
120+
args = parse_arguments()
121+
122+
if args.all:
123+
# Apply what the language offered by parsing it alongside everything else, so an
124+
# offered check lands exactly where the same flag typed by hand would.
125+
sys.argv[1:1] = args.extra_checks
126+
args = parse_arguments()
127+
128+
if not args.tests:
129+
args.tests.append(".")
130+
131+
os.environ.update(args.env)
100132

101133
# Resolve these only once all arguments are known, so that a `RAM_PER_THREAD=` test
102134
# argument can lower the default on memory-heavy suites.
103135
default_ram = 3000 if sys.platform == "linux" else 2048
104-
ram_per_thread = int(env_value(args, "RAM_PER_THREAD", default_ram))
105-
cpus = int(env_value(args, "CPUS", os.cpu_count() or 1))
106-
args["flags"][:0] = [f"--ram={ram_per_thread * cpus}", f"-j{cpus}"]
136+
ram_per_thread = int(os.environ.get("RAM_PER_THREAD") or default_ram)
137+
cpus = int(os.environ.get("CPUS") or os.cpu_count() or 1)
138+
args.flags[:0] = [f"--ram={ram_per_thread * cpus}", f"-j{cpus}"]
107139

108-
if args["codeql"] == "build":
109-
if invoke([JUST, language, "build"], cwd=SEMMLE_CODE) != 0:
110-
return 1
140+
if args.codeql == "build":
141+
if ret := invoke([JUST, args.language, "build"], cwd=SEMMLE_CODE):
142+
return ret
111143

112-
if args["codeql"] != "host":
144+
if args.codeql != "host":
113145
# Disable the default implicit config file, but keep an explicit one.
114146
# Same behavior wrt --codeql as the integration test runner.
115147
os.environ.setdefault("CODEQL_CONFIG_FILE", ".")
116148

117-
for env_var in args["env"]:
118-
key, _, value = env_var.partition("=")
119-
if not key:
120-
error(f"Invalid environment variable assignment: {env_var}")
121-
return 1
122-
os.environ[key] = value
123-
124-
# Resolve codeql executable
125-
if args["codeql"] in ("built", "build"):
126-
codeql = Path(SEMMLE_CODE, "target", "intree", f"codeql-{language}", "codeql")
127-
elif args["codeql"] == "host":
128-
codeql = Path("codeql")
129-
else:
130-
codeql = Path(args["codeql"])
131-
132-
if codeql.is_dir():
133-
codeql = codeql / "codeql"
134-
135-
# On Windows, prefer codeql.exe over the Unix shell wrapper
136-
if sys.platform == "win32" and codeql.suffix != ".exe":
137-
exe = codeql.with_suffix(".exe")
138-
if exe.exists():
139-
codeql = exe
140-
141-
if args["codeql"] != "host" and not codeql.exists():
149+
codeql = resolve_codeql(args)
150+
151+
if not codeql.exists():
142152
error(f"CodeQL executable not found: {codeql}")
143-
return 1
144153

145154
return invoke(
146-
[str(codeql), "test", "run", *args["flags"], "--", *args["tests"]],
147-
log_prefix=" ".join(args["env"]),
155+
[codeql, "test", "run", *args.flags, "--", *args.tests],
156+
log_prefix=" ".join(f"{k}={v}" for k, v in args.env.items()),
148157
)
149158

150159

0 commit comments

Comments
 (0)