From 2e8f929c2f75924c8c399a43dd93930d53725a23 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 15 Sep 2026 11:34:38 +0200 Subject: [PATCH 01/15] C++: move consistency queries into the open-source repo These queries live next to the C++ QL tests they check, so that `codeql test run --consistency-queries` can find them without an internal checkout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- cpp/ql/consistency-queries/badLocations.ql | 9 +++++++++ cpp/ql/consistency-queries/nullInToString.ql | 5 +++++ cpp/ql/consistency-queries/qlpack.yml | 5 +++++ cpp/ql/consistency-queries/unusedLocations.ql | 10 ++++++++++ .../variableDeclarationsWithoutTypes.ql | 5 +++++ cpp/ql/consistency-queries/variablesWithoutTypes.ql | 5 +++++ 6 files changed, 39 insertions(+) create mode 100644 cpp/ql/consistency-queries/badLocations.ql create mode 100644 cpp/ql/consistency-queries/nullInToString.ql create mode 100644 cpp/ql/consistency-queries/qlpack.yml create mode 100644 cpp/ql/consistency-queries/unusedLocations.ql create mode 100644 cpp/ql/consistency-queries/variableDeclarationsWithoutTypes.ql create mode 100644 cpp/ql/consistency-queries/variablesWithoutTypes.ql diff --git a/cpp/ql/consistency-queries/badLocations.ql b/cpp/ql/consistency-queries/badLocations.ql new file mode 100644 index 000000000000..385d3d92fe6a --- /dev/null +++ b/cpp/ql/consistency-queries/badLocations.ql @@ -0,0 +1,9 @@ +import cpp + +// Locations should either be :0:0:0:0 locations (UnknownLocation, or +// a whole file), or all 4 fields should be positive. +from Location l +where + [l.getStartLine(), l.getEndLine(), l.getStartColumn(), l.getEndColumn()] != 0 and + [l.getStartLine(), l.getEndLine(), l.getStartColumn(), l.getEndColumn()] < 1 +select l diff --git a/cpp/ql/consistency-queries/nullInToString.ql b/cpp/ql/consistency-queries/nullInToString.ql new file mode 100644 index 000000000000..4a6385b519ab --- /dev/null +++ b/cpp/ql/consistency-queries/nullInToString.ql @@ -0,0 +1,5 @@ +import cpp + +from Element e +where e.toString().matches("%(null)%") +select e diff --git a/cpp/ql/consistency-queries/qlpack.yml b/cpp/ql/consistency-queries/qlpack.yml new file mode 100644 index 000000000000..fed0e22e17ba --- /dev/null +++ b/cpp/ql/consistency-queries/qlpack.yml @@ -0,0 +1,5 @@ +name: codeql/cpp-consistency-queries +groups: [cpp, test, consistency-queries] +dependencies: + codeql/cpp-all: ${workspace} +extractor: cpp diff --git a/cpp/ql/consistency-queries/unusedLocations.ql b/cpp/ql/consistency-queries/unusedLocations.ql new file mode 100644 index 000000000000..875c60ba3251 --- /dev/null +++ b/cpp/ql/consistency-queries/unusedLocations.ql @@ -0,0 +1,10 @@ +import cpp + +from Location l +where + not any(Element e).getLocation() = l and + not any(LambdaCapture lc).getLocation() = l and + not any(MacroAccess ma).getActualLocation() = l and + not any(NamespaceDeclarationEntry nde).getBodyLocation() = l and + not any(XmlLocatable xml).getLocation() = l +select l diff --git a/cpp/ql/consistency-queries/variableDeclarationsWithoutTypes.ql b/cpp/ql/consistency-queries/variableDeclarationsWithoutTypes.ql new file mode 100644 index 000000000000..2573d660defd --- /dev/null +++ b/cpp/ql/consistency-queries/variableDeclarationsWithoutTypes.ql @@ -0,0 +1,5 @@ +import cpp + +from VariableDeclarationEntry i +where not exists(i.getType()) +select i diff --git a/cpp/ql/consistency-queries/variablesWithoutTypes.ql b/cpp/ql/consistency-queries/variablesWithoutTypes.ql new file mode 100644 index 000000000000..d004c175abd0 --- /dev/null +++ b/cpp/ql/consistency-queries/variablesWithoutTypes.ql @@ -0,0 +1,5 @@ +import cpp + +from Variable i +where not exists(i.getType()) +select i From 909e23eb78d460dcec3f59604430f7f081cee979 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 15 Sep 2026 11:34:39 +0200 Subject: [PATCH 02/15] Rust: allow formatting without linting, fix codegen runfiles path `lint.py --format-only` gives the upcoming `just format` verb a way to reformat without failing on pre-existing lint findings. codegen.sh looked up its runfiles via `external/ql+`, which only resolves in a main-repository layout. `../ql+` works from both, so codegen keeps working when the repository is consumed as a bazel dependency. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- rust/codegen/codegen.sh | 2 +- rust/lint.py | 47 +++++++++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/rust/codegen/codegen.sh b/rust/codegen/codegen.sh index 2d415009aed8..726ff138db78 100755 --- a/rust/codegen/codegen.sh +++ b/rust/codegen/codegen.sh @@ -2,7 +2,7 @@ set -eu -source misc/bazel/runfiles.sh 2>/dev/null || source external/ql+/misc/bazel/runfiles.sh +source misc/bazel/runfiles.sh 2>/dev/null || source ../ql+/misc/bazel/runfiles.sh ast_generator="$(rlocation "$1")" grammar_file="$(rlocation "$2")" diff --git a/rust/lint.py b/rust/lint.py index 600a888649e9..3ace1667a464 100755 --- a/rust/lint.py +++ b/rust/lint.py @@ -4,6 +4,15 @@ import pathlib import shutil import sys +import argparse + + +def options(): + parser = argparse.ArgumentParser(description="lint rust language pack code") + parser.add_argument( + "--format-only", action="store_true", help="Only apply formatting" + ) + return parser.parse_args() def tool(name): @@ -12,27 +21,33 @@ def tool(name): return ret -this_dir = pathlib.Path(__file__).resolve().parent +def main(): + args = options() + this_dir = pathlib.Path(__file__).resolve().parent + + cargo = tool("cargo") + bazel = tool("bazel") -cargo = tool("cargo") -bazel = tool("bazel") + runs = [] -runs = [] + def run(tool, args, *, cwd=this_dir): + print("+", tool, args) + runs.append(subprocess.run([tool] + args.split(), cwd=cwd)) -def run(tool, args, *, cwd=this_dir): - print("+", tool, args) - runs.append(subprocess.run([tool] + args.split(), cwd=cwd)) + # make sure bazel-provided sources are put in tree for `cargo` to work with them + run(bazel, "run ast-generator:inject-sources") + run(cargo, "fmt --all --quiet") + if not args.format_only: + for manifest in this_dir.rglob("Cargo.toml"): + if not manifest.is_relative_to(this_dir / "ql") and not manifest.is_relative_to(this_dir / "integration-tests"): + run(cargo, + "clippy --fix --allow-dirty --allow-staged --quiet -- -D warnings", + cwd=manifest.parent) -# make sure bazel-provided sources are put in tree for `cargo` to work with them -run(bazel, "run ast-generator:inject-sources") -run(cargo, "fmt --all --quiet") + return max(r.returncode for r in runs) -for manifest in this_dir.rglob("Cargo.toml"): - if not manifest.is_relative_to(this_dir / "ql") and not manifest.is_relative_to(this_dir / "integration-tests"): - run(cargo, - "clippy --fix --allow-dirty --allow-staged --quiet -- -D warnings", - cwd=manifest.parent) -sys.exit(max(r.returncode for r in runs)) +if __name__ == "__main__": + sys.exit(main()) From 95d64a3b6cbc1b37c11d15105081a73ad2c68e7e Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:50:46 +0200 Subject: [PATCH 03/15] Just: introduce the common verbs across all languages Adds a shared `just` infrastructure for `build`, `test`, `format`, `lint` and `generate`, and wires every language into it. A verb is discovered by walking up from the current directory to find the nearest justfile that implements it (with a forwarder retrying `just ` from there), and by walking down to find every recipe nested under the argument, since a verb higher up usually does a different job rather than a broader version of one further down. Both directions run, and every distinct recipe found runs; one reached only through `import` is the same job and runs once. QL test suites opt out of the downward search, since running one implicitly would be slow and not something a broad `just test` should decide unasked. The initial rollout immediately turned up gaps once every language actually used it: `format` broke on paths containing spaces, since the file list came from splitting a shell command's output rather than a proper argument list; a verb run from a nested directory did not look above it, even though an absolute argument already did; a directory that only makes sense named explicitly had no way to opt out of implicit discovery; a passed-over directory marked an otherwise successful run as failed instead of just being reported; the bazel file walker needed to select by name and skip generated trees; a forwarding justfile had no way to answer a verb itself rather than only ever delegating to what it imports; and output needed to be flushed before a forwarded child writes, so the account of what is about to run cannot land after that command's own output when both share a redirected stream. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- actions/justfile | 9 + actions/ql/integration-tests/justfile | 8 + actions/ql/justfile | 6 + actions/ql/test/justfile | 12 + cpp/justfile | 10 + cpp/ql/integration-tests/justfile | 8 + cpp/ql/justfile | 6 + cpp/ql/test/justfile | 12 + csharp/justfile | 9 + csharp/ql/integration-tests/justfile | 8 + csharp/ql/justfile | 6 + csharp/ql/test/justfile | 12 + go/justfile | 9 + go/ql/integration-tests/justfile | 8 + go/ql/justfile | 6 + go/ql/test/justfile | 12 + java/justfile | 4 + java/ql/integration-tests/justfile | 8 + java/ql/justfile | 6 + java/ql/test-kotlin1/justfile | 13 + java/ql/test-kotlin2/justfile | 13 + java/ql/test/justfile | 14 + javascript/justfile | 9 + javascript/ql/integration-tests/justfile | 8 + javascript/ql/justfile | 6 + javascript/ql/test/justfile | 12 + justfile | 4 + lib.just | 1 + misc/codegen/justfile | 5 + misc/just/README.md | 92 ++++++ misc/just/build.just | 21 ++ misc/just/codeql_test_run.py | 155 +++++++++ misc/just/defs.just | 61 ++++ misc/just/format.just | 29 ++ misc/just/forward.just | 30 ++ misc/just/forward_command.py | 383 +++++++++++++++++++++++ misc/just/justfile | 2 + misc/just/language_tests.py | 66 ++++ misc/just/lib.just | 31 ++ misc/just/run_on_files.py | 120 +++++++ misc/just/semmle-code-stub.just | 1 + python/justfile | 26 ++ python/ql/integration-tests/justfile | 8 + python/ql/justfile | 12 + python/ql/test/justfile | 12 + ruby/justfile | 9 + ruby/ql/integration-tests/justfile | 8 + ruby/ql/justfile | 6 + ruby/ql/test/justfile | 12 + rust/justfile | 17 + rust/ql/integration-tests/justfile | 8 + rust/ql/justfile | 6 + rust/ql/test/justfile | 12 + swift/justfile | 18 ++ swift/ql/integration-tests/justfile | 8 + swift/ql/justfile | 6 + swift/ql/test/justfile | 12 + unified/extractor/justfile | 4 + unified/justfile | 12 + unified/ql/justfile | 6 + unified/ql/test/justfile | 12 + unified/swift-syntax-rs/justfile | 4 + 62 files changed, 1453 insertions(+) create mode 100644 actions/justfile create mode 100644 actions/ql/integration-tests/justfile create mode 100644 actions/ql/justfile create mode 100644 actions/ql/test/justfile create mode 100644 cpp/justfile create mode 100644 cpp/ql/integration-tests/justfile create mode 100644 cpp/ql/justfile create mode 100644 cpp/ql/test/justfile create mode 100644 csharp/justfile create mode 100644 csharp/ql/integration-tests/justfile create mode 100644 csharp/ql/justfile create mode 100644 csharp/ql/test/justfile create mode 100644 go/justfile create mode 100644 go/ql/integration-tests/justfile create mode 100644 go/ql/justfile create mode 100644 go/ql/test/justfile create mode 100644 java/justfile create mode 100644 java/ql/integration-tests/justfile create mode 100644 java/ql/justfile create mode 100644 java/ql/test-kotlin1/justfile create mode 100644 java/ql/test-kotlin2/justfile create mode 100644 java/ql/test/justfile create mode 100644 javascript/justfile create mode 100644 javascript/ql/integration-tests/justfile create mode 100644 javascript/ql/justfile create mode 100644 javascript/ql/test/justfile create mode 100644 justfile create mode 100644 lib.just create mode 100644 misc/codegen/justfile create mode 100644 misc/just/README.md create mode 100644 misc/just/build.just create mode 100755 misc/just/codeql_test_run.py create mode 100644 misc/just/defs.just create mode 100644 misc/just/format.just create mode 100644 misc/just/forward.just create mode 100644 misc/just/forward_command.py create mode 100644 misc/just/justfile create mode 100755 misc/just/language_tests.py create mode 100644 misc/just/lib.just create mode 100644 misc/just/run_on_files.py create mode 100644 misc/just/semmle-code-stub.just create mode 100644 python/justfile create mode 100644 python/ql/integration-tests/justfile create mode 100644 python/ql/justfile create mode 100644 python/ql/test/justfile create mode 100644 ruby/justfile create mode 100644 ruby/ql/integration-tests/justfile create mode 100644 ruby/ql/justfile create mode 100644 ruby/ql/test/justfile create mode 100644 rust/justfile create mode 100644 rust/ql/integration-tests/justfile create mode 100644 rust/ql/justfile create mode 100644 rust/ql/test/justfile create mode 100644 swift/justfile create mode 100644 swift/ql/integration-tests/justfile create mode 100644 swift/ql/justfile create mode 100644 swift/ql/test/justfile create mode 100644 unified/extractor/justfile create mode 100644 unified/justfile create mode 100644 unified/ql/justfile create mode 100644 unified/ql/test/justfile create mode 100644 unified/swift-syntax-rs/justfile diff --git a/actions/justfile b/actions/justfile new file mode 100644 index 000000000000..b96eb20dfe26 --- /dev/null +++ b/actions/justfile @@ -0,0 +1,9 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "actions") + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/actions/ql/integration-tests/justfile b/actions/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/actions/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/actions/ql/justfile b/actions/ql/justfile new file mode 100644 index 000000000000..e613c25b52c1 --- /dev/null +++ b/actions/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := "" diff --git a/actions/ql/test/justfile b/actions/ql/test/justfile new file mode 100644 index 000000000000..8c06f3e5c155 --- /dev/null +++ b/actions/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +all_checks := default_db_checks + +[no-cd] +test *ARGS=".": (_codeql_test "actions" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/cpp/justfile b/cpp/justfile new file mode 100644 index 000000000000..0fc87260ebec --- /dev/null +++ b/cpp/justfile @@ -0,0 +1,10 @@ +import '../lib.just' +import? '../../cpp-coding-standards.just' + +[group('build')] +build: (_build_dist "cpp") + +roots := [source_dir() / 'ql/test', SEMMLE_CODE / 'semmlecode-cpp-tests'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/cpp/ql/integration-tests/justfile b/cpp/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/cpp/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/cpp/ql/justfile b/cpp/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/cpp/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/cpp/ql/test/justfile b/cpp/ql/test/justfile new file mode 100644 index 000000000000..4ab3ef69856a --- /dev/null +++ b/cpp/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := ['--include-location-in-star'] + +all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-unused-labels', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "cpp" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/csharp/justfile b/csharp/justfile new file mode 100644 index 000000000000..6f99dd8703e0 --- /dev/null +++ b/csharp/justfile @@ -0,0 +1,9 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "csharp") + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/csharp/ql/integration-tests/justfile b/csharp/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/csharp/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/csharp/ql/justfile b/csharp/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/csharp/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/csharp/ql/test/justfile b/csharp/ql/test/justfile new file mode 100644 index 000000000000..3efa95d340ca --- /dev/null +++ b/csharp/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--additional-packs=ql', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "csharp" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/go/justfile b/go/justfile new file mode 100644 index 000000000000..e1ea5203166e --- /dev/null +++ b/go/justfile @@ -0,0 +1,9 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "go") + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/go/ql/integration-tests/justfile b/go/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/go/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/go/ql/justfile b/go/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/go/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/go/ql/test/justfile b/go/ql/test/justfile new file mode 100644 index 000000000000..60b5d78b053a --- /dev/null +++ b/go/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +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] + +[no-cd] +test *ARGS=".": (_codeql_test "go" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/java/justfile b/java/justfile new file mode 100644 index 000000000000..aba4ba7b21dd --- /dev/null +++ b/java/justfile @@ -0,0 +1,4 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "java") diff --git a/java/ql/integration-tests/justfile b/java/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/java/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/java/ql/justfile b/java/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/java/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/java/ql/test-kotlin1/justfile b/java/ql/test-kotlin1/justfile new file mode 100644 index 000000000000..a9815627d15e --- /dev/null +++ b/java/ql/test-kotlin1/justfile @@ -0,0 +1,13 @@ +import "../justfile" + +# These are CI shards of the Kotlin language tests, run as `just java +# kotlin-language-tests-1`, so they only run when asked for by name. +explicit_verbs := ['test'] + +# Kotlin tests may fail the diags.ql consistency test if the diagnostic limit is set. +base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT='] + +all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/java/ql/test-kotlin2/justfile b/java/ql/test-kotlin2/justfile new file mode 100644 index 000000000000..bda00ff0ca75 --- /dev/null +++ b/java/ql/test-kotlin2/justfile @@ -0,0 +1,13 @@ +import "../justfile" + +# These are CI shards of the Kotlin language tests, run as `just java +# kotlin-language-tests-2`, so they only run when asked for by name. +explicit_verbs := ['test'] + +# Kotlin tests may fail the diags.ql consistency test if the diagnostic limit is set. +base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT=', 'CODEQL_KOTLIN_LEGACY_TEST_EXTRACTION_KOTLIN2=true'] + +all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/java/ql/test/justfile b/java/ql/test/justfile new file mode 100644 index 000000000000..53a5d2d9dee3 --- /dev/null +++ b/java/ql/test/justfile @@ -0,0 +1,14 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +# The Kotlin extractor must see the diagnostic limit set, but blank: hence the single +# trailing space. +base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT= '] + +all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/javascript/justfile b/javascript/justfile new file mode 100644 index 000000000000..769847a380d1 --- /dev/null +++ b/javascript/justfile @@ -0,0 +1,9 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "javascript") + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/javascript/ql/integration-tests/justfile b/javascript/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/javascript/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/javascript/ql/justfile b/javascript/ql/justfile new file mode 100644 index 000000000000..e613c25b52c1 --- /dev/null +++ b/javascript/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := "" diff --git a/javascript/ql/test/justfile b/javascript/ql/test/justfile new file mode 100644 index 000000000000..18daff51c273 --- /dev/null +++ b/javascript/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +all_checks := default_db_checks + +[no-cd] +test *ARGS=".": (_codeql_test "javascript" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/justfile b/justfile new file mode 100644 index 000000000000..94cf7d2f4bb3 --- /dev/null +++ b/justfile @@ -0,0 +1,4 @@ +# see misc/just/README.md for an overview + +import 'lib.just' +import 'misc/just/forward.just' diff --git a/lib.just b/lib.just new file mode 100644 index 000000000000..0ddd926bcda5 --- /dev/null +++ b/lib.just @@ -0,0 +1 @@ +import "misc/just/lib.just" diff --git a/misc/codegen/justfile b/misc/codegen/justfile new file mode 100644 index 000000000000..a65fa16e5679 --- /dev/null +++ b/misc/codegen/justfile @@ -0,0 +1,5 @@ +import "../just/lib.just" + +test *ARGS="": (_bazel ['test', '@codeql//misc/codegen/...']) + +format *ARGS=".": (_format_py ARGS) diff --git a/misc/just/README.md b/misc/just/README.md new file mode 100644 index 000000000000..74b64283b7a6 --- /dev/null +++ b/misc/just/README.md @@ -0,0 +1,92 @@ +This directory contains an infrastructure for [`just`](https://github.com/casey/just) +recipes that can be used throughout this and the internal repository. In particular we +have common verbs (`build`, `test`, `format`, `lint`, `generate`) that individual parts +of the project can implement, and some common functionality that can be used to that +effect. + +# Forwarding + +The core of the functionality is given by forwarding. The idea is that: + +- if you are in the directory where a verb is implemented, you will get that as per + standard `just` behaviour (possibly using fallback). +- if on the other hand you are above it, and you run something like + `just test ql/rust/ql/test/{a,b}`, then a forwarder script finds a common justfile + implementing the verb for all the positional arguments passed there, and then retries + calling `just test` from there. So if `test` is implemented beneath that (in that case, + it is in `rust/ql/test`), it uses that recipe. +- even if there isn't a recipe that is common to all the positional arguments, the + forwarder will still group the arguments in batches using the same recipe. So + `just build ql/rust ql/java`, or + `just test ql/rust/ql/test/some/language/test ql/rust/ql/integration-test/some/integration/test` + will also work, with corresponding recipes run sequentially. +- finally, the forwarder also looks _below_ each argument, so that `just test ql/cpp` + runs the tests defined underneath it. The argument only says where to look in this + case, so each recipe found is run on its own directory rather than being passed the + argument. Several may be found, in which case they run sequentially: `just format + ql/cpp` formats everything under `ql/cpp` that knows how to format itself. + +Both directions are searched, and every distinct recipe found runs. This matters because +a verb higher up is usually doing a different job from one further down rather than a +broader version of it: `rust` formats Rust sources while `rust/ql` formats QL, so +`just format rust` has to do both. A recipe that only arrived through `import` is the +same job, though, and runs once. + +A directory that only makes sense when named explicitly can opt out of being found from +above: + +```just +explicit_verbs := ['test'] +``` + +This only affects the downward search. Running the verb from inside that directory, or +naming the directory on the command line, keeps working. A verb that passed over such a +directory says so and names it, so that a command covering a tree does not look like it +covered more than it did. That listing is part of the account of what ran and leaves the +exit status alone; only a verb that matched nothing at all fails. + +The QL test suites use this: `test` on a language runs the whole suite, which takes a +long time and needs a CodeQL CLI, so that has to be asked for by name. Integration tests +and the sharded Kotlin suites that CI runs opt out for the same reason. What is left +discoverable from above is what is cheap enough to run without meaning to. + +Being an ordinary variable, `explicit_verbs` is inherited by justfiles importing one +that sets it. That is normally what is wanted, as importing a suite's justfile means +being the same kind of suite, down to the reason for naming it explicitly. An importer +that disagrees can reassign it, and its own value wins: + +```just +import '../some/suite/justfile' + +explicit_verbs := [] +``` + +Duplicate variables are allowed throughout (see `defs.just`), so this is silent in both +directions: assigning `explicit_verbs` without realising one was inherited overrides it +without complaint, which can put a heavy suite back within reach of a verb aimed at a +parent directory. + +Justfiles are found through `git`, so a newly written one needs to be either tracked or +untracked-but-not-ignored to be picked up. + +Another point is how launching QL tests can be tweaked: + +- by default, the corresponding CLI is built from the internal repo (nothing is done if + working in `codeql` standalone), and no additional database or consistency checks are + made +- `--codeql=built` can be passed to skip the build step (if no changes were made to the + CLI/extractors). This is consistent with the same pytest option +- you can add the additional checks that CI does with `--all-checks` or the `+` + abbreviation. These additional checks are configured in justfiles per language, and + correspond to all the additional checks that CI adds (but that a dev might not want to + run by default). + +Test arguments are passed around as `just` lists (`set lists`), so they reach the +underlying runner already split and arguments containing spaces survive intact. + +One caveat: when a verb ends up running several recipes, non-positional arguments need +to be understood by all of them. That is fine when they speak the same language, as +`--learn` or `--codeql` do across QL and integration tests. It is not when they do not: +a broad `just test .` reaches bazel and pytest suites alike, and a flag meant for one of +them will fail on the other. It fails rather than being quietly ignored, so the answer +is to aim the verb at something narrower. diff --git a/misc/just/build.just b/misc/just/build.just new file mode 100644 index 000000000000..f564c4aa9041 --- /dev/null +++ b/misc/just/build.just @@ -0,0 +1,21 @@ +# Helper build recipes + +import "defs.just" + +# Build the given language-specific CLI distribution +_build_dist LANGUAGE: _require_semmle_code (_maybe_build_dist LANGUAGE) + +# Build the language-specific distribution if we are in an internal repository checkout +# Otherwise, do nothing +[no-exit-message] +_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=all') '# using codeql from PATH, if any') + +# Call bazel. Uses our official bazel wrapper if we are in an internal repository checkout +[no-cd] +[no-exit-message] +_bazel *ARGS: (_if_in_semmle_code 'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel' 'bazel' ARGS) + +# Call sembuild (requires an internal repository checkout) +[no-cd] +[no-exit-message] +_sembuild *ARGS: (_run_in_semmle_code (['./build'] ++ ARGS)) diff --git a/misc/just/codeql_test_run.py b/misc/just/codeql_test_run.py new file mode 100755 index 000000000000..5bda2d5b86eb --- /dev/null +++ b/misc/just/codeql_test_run.py @@ -0,0 +1,155 @@ +#!/usr/bin/env python3 +"""Run CodeQL tests with appropriate configuration. + +Called from just recipes as: + python3 codeql_test_run.py LANGUAGE [ARG...] + +Arguments are already split by `just` (see `set lists`), so each one is taken verbatim. +`--all-checks=FLAG` contributes FLAG to the set of extra checks that `--all-checks` (or +its `+` abbreviation) turns on. +""" + +import os +import re +import subprocess +import sys +from pathlib import Path + +JUST = os.environ.get("JUST_EXECUTABLE", "just") +ERROR = os.environ.get("JUST_ERROR", "error: ") +CMD_BEGIN = os.environ.get("CMD_BEGIN", "") +CMD_END = os.environ.get("CMD_END", "") +SEMMLE_CODE = os.environ.get("SEMMLE_CODE") + +ALL_CHECKS_PREFIX = "--all-checks=" +ENV_RE = re.compile(r"^[A-Z_][A-Z_0-9]*=.*$") + + +def invoke(invocation, *, cwd=None, log_prefix=""): + prefix = f"{log_prefix} " if log_prefix else "" + print(f"{CMD_BEGIN}{prefix}{' '.join(invocation)}{CMD_END}") + try: + subprocess.run(invocation, check=True, cwd=cwd) + except subprocess.CalledProcessError as e: + return e.returncode + return 0 + + +def error(message): + print(f"{ERROR}{message}", file=sys.stderr) + + +def parse_args(args, argv): + """Sort arguments into tests, flags and environment assignments.""" + for arg in argv: + if not arg: + # an empty argument can come from a caller interpolating an unset variable + continue + if arg.startswith(ALL_CHECKS_PREFIX): + args["all_checks"].append(arg[len(ALL_CHECKS_PREFIX) :]) + elif arg.startswith("--codeql="): + args["codeql"] = arg.split("=", 1)[1] + elif arg in ("+", "--all-checks"): + args["all"] = True + elif arg.startswith("-"): + args["flags"].append(arg) + elif ENV_RE.match(arg): + args["env"].append(arg) + else: + args["tests"].append(arg) + + +def env_value(args, name, default): + """Resolve a setting from test arguments, then the environment, then a default.""" + for assignment in reversed(args["env"]): + key, _, value = assignment.partition("=") + if key == name and value: + return value + return os.environ.get(name) or default + + +def main(): + argv = sys.argv[1:] + if not argv: + error("Usage: codeql_test_run.py LANGUAGE [ARG...]") + return 1 + + language, *rest = argv + + args = { + "tests": [], + "flags": [], + "env": [], + "all_checks": [], + "codeql": "build" if SEMMLE_CODE else "host", + "all": False, + } + parse_args(args, rest) + if args["all"]: + parse_args(args, args["all_checks"]) + + if not SEMMLE_CODE and args["codeql"] in ("build", "built"): + error( + "Using `--codeql=build` or `--codeql=built` requires working " + "with the internal repository" + ) + return 1 + + if not args["tests"]: + args["tests"].append(".") + + # Resolve these only once all arguments are known, so that a `RAM_PER_THREAD=` test + # argument can lower the default on memory-heavy suites. + default_ram = 3000 if sys.platform == "linux" else 2048 + ram_per_thread = int(env_value(args, "RAM_PER_THREAD", default_ram)) + cpus = int(env_value(args, "CPUS", os.cpu_count() or 1)) + args["flags"][:0] = [f"--ram={ram_per_thread * cpus}", f"-j{cpus}"] + + if args["codeql"] == "build": + if invoke([JUST, language, "build"], cwd=SEMMLE_CODE) != 0: + return 1 + + if args["codeql"] != "host": + # Disable the default implicit config file, but keep an explicit one. + # Same behavior wrt --codeql as the integration test runner. + os.environ.setdefault("CODEQL_CONFIG_FILE", ".") + + for env_var in args["env"]: + key, _, value = env_var.partition("=") + if not key: + error(f"Invalid environment variable assignment: {env_var}") + return 1 + os.environ[key] = value + + # Resolve codeql executable + if args["codeql"] in ("built", "build"): + codeql = Path(SEMMLE_CODE, "target", "intree", f"codeql-{language}", "codeql") + elif args["codeql"] == "host": + codeql = Path("codeql") + else: + codeql = Path(args["codeql"]) + + if codeql.is_dir(): + codeql = codeql / "codeql" + + # On Windows, prefer codeql.exe over the Unix shell wrapper + if sys.platform == "win32" and codeql.suffix != ".exe": + exe = codeql.with_suffix(".exe") + if exe.exists(): + codeql = exe + + if args["codeql"] != "host" and not codeql.exists(): + error(f"CodeQL executable not found: {codeql}") + return 1 + + return invoke( + [str(codeql), "test", "run", *args["flags"], "--", *args["tests"]], + log_prefix=" ".join(args["env"]), + ) + + +if __name__ == "__main__": + try: + sys.exit(main()) + except KeyboardInterrupt: + sys.exit(128 + 2) diff --git a/misc/just/defs.just b/misc/just/defs.just new file mode 100644 index 000000000000..47cedd124b44 --- /dev/null +++ b/misc/just/defs.just @@ -0,0 +1,61 @@ +import? '../../../semmle-code.just' # internal repo just file, if present +import 'semmle-code-stub.just' + +# `set lists` is what lets recipes forward argument lists without encoding them as +# whitespace separated strings. It is still unstable as of just 1.58. +set unstable +set lists +set fallback +set allow-duplicate-recipes +set allow-duplicate-variables + +export PATH_SEP := if os() == "windows" { ";" } else { ":" } +export JUST_EXECUTABLE := just_executable() + +error := f'{{ style("error") }}error{{ NORMAL }}: ' +cmd_sep := "\n#--------------------------------------------------------\n" +export CMD_BEGIN := style("command") + cmd_sep +export CMD_END := cmd_sep + NORMAL +export JUST_ERROR := error + +py := "python3" + +default_db_checks := ['--check-databases', '--check-diff-informed', '--fail-on-trap-errors'] + +[no-exit-message] +@_require_semmle_code: + {{ if SEMMLE_CODE == "" { f''' + echo "{error} running this recipe requires doing so from an internal repository checkout" >&2 + exit 1 + ''' } else { "" } }} + +[no-cd] +_run +ARGS: + {{ cmd_sep }}{{ ARGS }}{{ cmd_sep }} + +[no-cd] +_run_in DIR +ARGS: + {{ cmd_sep }}cd "{{ DIR }}"; {{ ARGS }}{{ cmd_sep }} + +[no-cd] +_run_in_semmle_code +ARGS: _require_semmle_code (_run_in "$SEMMLE_CODE" ARGS) + +[no-cd] +[no-exit-message] +[positional-arguments] +@_just +ARGS: + echo "-> just $@" + "{{ JUST_EXECUTABLE }}" "$@" + +[no-cd] +[positional-arguments] +@_if_not_on_ci_just +ARGS: + if [ "${GITHUB_ACTIONS:-}" != "true" ]; then \ + echo "-> just $@"; \ + "$JUST_EXECUTABLE" "$@"; \ + fi + +[no-cd] +[no-exit-message] +_if_in_semmle_code THEN ELSE *ARGS: + {{ cmd_sep }}{{ if SEMMLE_CODE != "" { THEN } else { ELSE } }} {{ ARGS }}{{ cmd_sep }} diff --git a/misc/just/format.just b/misc/just/format.just new file mode 100644 index 000000000000..1c28344bc422 --- /dev/null +++ b/misc/just/format.just @@ -0,0 +1,29 @@ +import "build.just" + +_ql_formatter := if SEMMLE_CODE != "" { '"$SEMMLE_CODE/target/intree/codeql-nolang/codeql"' } else { "codeql" } + +_py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" } + +_cpp_formatter := if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" } + +# `codeql query format` and `clang-format` take files rather than directories, so the +# files are collected by `run_on_files.py`. Arguments are passed positionally so that +# paths containing spaces survive, of which this repository has many. + +[no-cd] +[no-exit-message] +[positional-arguments] +_format_ql +ARGS: (_maybe_build_dist "nolang") + {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" .ql,.qll {{ _ql_formatter }} query format --in-place -v -- "$@"{{ cmd_sep }} + +[no-cd] +[no-exit-message] +[positional-arguments] +_format_py *ARGS=".": + {{ cmd_sep }}{{ _py_formatter }} "$@"{{ cmd_sep }} + +[no-cd] +[no-exit-message] +[positional-arguments] +_format_cpp *ARGS=".": + {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" .h,.cpp {{ _cpp_formatter }} -i --verbose -- "$@"{{ cmd_sep }} diff --git a/misc/just/forward.just b/misc/just/forward.just new file mode 100644 index 000000000000..571f5e31a806 --- /dev/null +++ b/misc/just/forward.just @@ -0,0 +1,30 @@ +# Common verbs +# See README.md in this directory for an overview. + +import "lib.just" + +# Verbs are recipe names, so each one needs its own recipe. They all delegate to the +# same forwarder, which decides where the verb is actually implemented. + +[no-cd] +[no-exit-message] +[positional-arguments] +@_forward VERB *ARGS: + {{ py }} "{{ source_dir() }}/forward_command.py" "$@" + +alias t := test +alias b := build +alias g := generate +alias gen := generate +alias f := format +alias l := lint + +test *ARGS: (_forward "test" ARGS) + +build *ARGS: (_forward "build" ARGS) + +generate *ARGS: (_forward "generate" ARGS) + +lint *ARGS: (_forward "lint" ARGS) + +format *ARGS: (_forward "format" ARGS) diff --git a/misc/just/forward_command.py b/misc/just/forward_command.py new file mode 100644 index 000000000000..1814a0617dac --- /dev/null +++ b/misc/just/forward_command.py @@ -0,0 +1,383 @@ +#!/usr/bin/env python3 +"""Forward a common verb to the justfiles that implement it. + +Verbs like `test`, `build` and `format` are spelled the same everywhere, but what they +mean is defined per language, next to the code they act on. This finds the justfiles +implementing a verb for each of its arguments and runs every one of them, so that +`just test rust` or `just format .` work without a central list of who implements what. + +Justfiles are looked for in both directions from an argument: + +- above it, where a recipe is passed the argument itself, as that says what to act on +- below it, where a recipe is passed its own directory, as there the argument only said + where to look + +Every distinct recipe found this way runs. Recipes are compared by value, so one reached +through `import` is recognised as the same job and runs once, while a cross-cutting +recipe higher up composes with the more specific ones below instead of hiding them. + +Two things keep the search useful: recipes delegating back here are skipped, so it never +settles on a forwarder, and a directory can set `explicit_verbs` to stay out of reach of +a verb aimed at one of its parents. See README.md for the whole picture. + +Called from just recipes as: + python3 forward_command.py COMMAND [ARGS...] +""" + +import json +import os +import re +import subprocess +import sys +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +JUST = os.environ.get("JUST_EXECUTABLE", "just") +ERROR = os.environ.get("JUST_ERROR", "") + +# Recipes that delegate to this one do not implement a verb, they pass it on. Skipping +# them is what stops the search from settling on a forwarder, be it this one or the root +# justfile of a nested repository. +FORWARD_RECIPE = "_forward" + +# A justfile that forwards a verb has already spent the plain name on the forwarder, so +# it names its own implementation of that verb `_root_`. This is how a repository +# root gets to answer a verb for itself while still dispatching it everywhere else. +ROOT_PREFIX = "_root_" + +# Justfiles may list verbs that must be spelled out explicitly instead of being picked +# up by a verb aimed at one of their parent directories. +EXPLICIT_VERBS = "explicit_verbs" + +PROBE_WORKERS = 16 + + +def error(message): + # Anything already reported on stdout belongs before this, and the two streams are + # buffered differently when they are not both a terminal. + sys.stdout.flush() + print(f"{ERROR}{message}", file=sys.stderr) + + +def get_just_context(justfile, recipe, flags, positional_args): + """Get the (cwd, args) for invoking just with the given justfile.""" + if ( + len(positional_args) == 1 + and justfile == Path(positional_args[0]) / "justfile" + ): + # If there's only one positional argument and it matches the justfile + # path, suppress arguments so e.g. `just build ql/rust` becomes + # `just build` in the `ql/rust` directory + return positional_args[0], [recipe, *flags] + else: + return None, ["--justfile", str(justfile), recipe, *flags, *positional_args] + + +def dump_justfile(justfile): + """Parse a justfile with `just`, returning its JSON dump or an error message.""" + result = subprocess.run( + [JUST, "--dump", "--dump-format", "json", "--justfile", str(justfile)], + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + ) + if result.returncode != 0: + return None, result.stderr.strip() + return json.loads(result.stdout), None + + +def list_value(assignments, name): + """Read a list literal assignment from a justfile dump.""" + value = assignments.get(name, {}).get("value") + # A literal list is dumped as ["list", element...]. Anything else is an expression + # that cannot be evaluated without running just, and counts as absent. + if isinstance(value, list) and value[:1] == ["list"]: + return value[1:] + return [] + + +def accepts(recipe, argc): + """Check whether a recipe can be called with a given number of arguments.""" + parameters = recipe["parameters"] + variadic = parameters and parameters[-1]["kind"] in ("star", "plus") + required = sum( + 1 + for parameter in parameters + if parameter["default"] is None and parameter["kind"] != "star" + ) + return required <= argc and (variadic or argc <= len(parameters)) + + +def implements(dump, command, argc): + """Return the recipe a justfile runs for a command, if it has a usable one.""" + recipes = dump["recipes"] + recipe = recipes.get(dump["aliases"].get(command, command)) + if recipe is None or recipe["private"]: + return None + if any( + dependency["recipe"] == FORWARD_RECIPE for dependency in recipe["dependencies"] + ): + # Here the plain name is the forwarder's own, so it says nothing about what this + # directory does. A justfile that both forwards and answers the command itself + # spells its own answer `_root_`, the one name the two can share. + recipe = recipes.get(f"{ROOT_PREFIX}{command}") + if recipe is None: + return None + return recipe if accepts(recipe, argc) else None + + +def opts_out(dump, command): + """Whether a justfile asks to be named rather than found by a command.""" + return command in list_value(dump["assignments"], EXPLICIT_VERBS) + + +def dump_all(justfiles): + """Parse justfiles in parallel, reporting the ones that cannot be read.""" + with ThreadPoolExecutor(PROBE_WORKERS) as executor: + dumps = list(executor.map(dump_justfile, justfiles)) + parsed = [] + for justfile, (dump, failure) in zip(justfiles, dumps): + if dump is None: + error(f"could not read {justfile}:\n{failure}") + else: + parsed.append((justfile, dump)) + return parsed + + +def git(directory, *args): + """Run a git command in a directory, returning its output lines.""" + result = subprocess.run( + ["git", "-C", directory, *args], + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + ) + if result.returncode != 0: + error(f"`git {' '.join(args)}` failed in {directory}:\n{result.stderr.strip()}") + return [] + return result.stdout.splitlines() + + +def submodules(directory): + """List the initialised submodules under a directory.""" + toplevel = git(directory, "rev-parse", "--show-toplevel") + if not toplevel or not (Path(toplevel[0]) / ".gitmodules").exists(): + return [] + paths = [ + Path(toplevel[0]) / line.split(" ", 1)[1] + for line in git( + toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$" + ) + ] + within = Path(directory).resolve() + return [ + Path(directory) / os.path.relpath(path, within) + for path in paths + # An uninitialised submodule is an empty directory, with nothing to run. + if path.is_relative_to(within) and (path / ".git").exists() + ] + + +def find_justfiles(directory): + """List every justfile under a directory. + + Submodules are listed separately, as `git ls-files` can either recurse into them or + report untracked files, but not both, and a justfile that has just been written is + worth finding. + """ + justfiles = set() + for repository in [directory, *submodules(directory)]: + justfiles.update( + Path(repository) / line + for line in git( + repository, + "ls-files", + "--cached", + "--others", + "--exclude-standard", + "--", + "justfile", + "*/justfile", + ) + ) + return justfiles + + +def invocation_path(path, *, like): + """Spell an absolute path like the user spelled the argument.""" + if Path(like).is_absolute(): + return path + return Path(os.path.relpath(path, Path.cwd())) + + +def find_justfiles_above(command, arg): + """Search up the directory tree for justfiles implementing the command. + + All of them are collected rather than just the nearest, because a recipe higher up + is often doing a different job from one further down rather than a broader version + of it. Returns (justfile, recipe) pairs, nearest first. + """ + directory = Path(arg).resolve() + candidates = [ + invocation_path(p / "justfile", like=arg) + for p in [directory, *directory.parents] + if (p / "justfile").exists() + ] + found = [] + seen = [] + for justfile, dump in dump_all(candidates): + # A justfile sitting exactly on the argument is called without it, as the + # argument would only repeat where it already is. + argc = 0 if justfile.parent.resolve() == directory else 1 + recipe = implements(dump, command, argc) + # These justfiles are nested, so a recipe that was seen already is one this + # one merely imported, and the nearest spelling of it has been taken. + if recipe is not None and recipe not in seen: + seen.append(recipe) + found.append((justfile, recipe)) + return found + + +def find_justfiles_below(command, directory, covered=()): + """Search down a directory for justfiles implementing the command. + + A justfile is skipped when the recipe it would run is one an enclosing directory + already contributes, which is what `import` makes happen: the recipe is the same + job, so running it once is enough. `covered` holds the recipes already found above + the directory. + + Returns the justfiles to run and, separately, the ones that implement the command + but ask to be named rather than found. + """ + # The justfile at `directory` is covered by the search above it. + candidates = sorted(find_justfiles(directory) - {Path(directory) / "justfile"}) + matches = [] + opted_out = [] + for justfile, dump in dump_all(candidates): + recipe = implements(dump, command, 0) + if recipe is None: + continue + if opts_out(dump, command): + opted_out.append(justfile) + else: + matches.append((justfile, recipe)) + contributed = {Path(directory): list(covered)} + found = [] + # Shallowest first, so that an enclosing justfile is always decided before the ones + # it may account for. + for justfile, recipe in sorted(matches, key=lambda match: len(match[0].parts)): + if any(recipe in contributed.get(p, []) for p in justfile.parent.parents): + continue + contributed.setdefault(justfile.parent, []).append(recipe) + found.append((justfile, recipe)) + return sorted(found, key=lambda match: match[0]), sorted(opted_out) + + +def resolve(command, arg): + """Find the justfiles implementing a command for an argument. + + Returns a list of (justfile, argument, recipe) triples, from both above and below + the argument. One found above gets the argument itself, as that selects what to act + on. One found below gets its own directory instead, as there the argument only said + where to look. Justfiles below that asked to be named are returned separately. + """ + above = find_justfiles_above(command, arg) + resolved = [(justfile, arg, recipe["name"]) for justfile, recipe in above] + opted_out = [] + if os.path.isdir(arg): + below, opted_out = find_justfiles_below( + command, arg, [recipe for _, recipe in above] + ) + resolved += [ + (justfile, str(justfile.parent), recipe["name"]) + for justfile, recipe in below + ] + return resolved, opted_out + + +def report_opted_out(command, justfiles, *, ran): + """Name the justfiles a command passed over because they ask to be named. + + Worth saying even when other recipes did run, as otherwise a command that looks + like it covered a whole directory quietly left parts of it alone. That case is + informational and goes to stdout with the rest of the account of what ran: the + command did what was asked of it. Only matching nothing at all is an error. + """ + if not justfiles: + return + directories = sorted(str(jf.parent) for jf in set(justfiles)) + # One per line: there can be dozens, and a single wrapped line is unreadable. + listed = "\n".join(f" {directory}" for directory in directories) + message = f"not run, as {command} must name these explicitly:\n{listed}" + if ran: + print(message) + else: + error(message) + + +def invoke_just(cwd, args): + """Run just with the given arguments.""" + # This process' stdout is block-buffered off a terminal, while the child writes to the + # same descriptor at once: without this the account lands after what it describes. + sys.stdout.flush() + try: + subprocess.run([JUST, *args], check=True, cwd=cwd) + except subprocess.CalledProcessError as e: + return e.returncode + return 0 + + +def forward(cmd, args): + """Forward a command to language-specific justfiles.""" + is_non_positional = re.compile(r"^(-.*|\+|[A-Z_][A-Z_0-9]*=.*)$") + flags = [arg for arg in args if is_non_positional.match(arg)] + positional_args = [arg for arg in args if not is_non_positional.match(arg)] + + justfiles = {} + opted_out = [] + for arg in positional_args or ["."]: + resolved, skipped = resolve(cmd, arg) + opted_out += skipped + if not resolved: + error(f"No justfile found for {cmd} on {arg}") + report_opted_out(cmd, skipped, ran=False) + return 1 + for justfile, justfile_arg, recipe in resolved: + justfiles.setdefault(justfile, (recipe, []))[1].append(justfile_arg) + + invocations = [] + for justfile, (recipe, pos_args) in justfiles.items(): + # An argument standing for the whole directory subsumes any more specific one + # that ended up on the same justfile. + whole_directory = str(justfile.parent) + if whole_directory in pos_args: + pos_args = [whole_directory] + cwd, just_args = get_just_context(justfile, recipe, flags, pos_args) + prefix = f"cd {cwd}; " if cwd else "" + print(f"-> {prefix}just {' '.join(just_args)}") + invocations.append((cwd, just_args)) + + report_opted_out(cmd, opted_out, ran=True) + + for cwd, just_args in invocations: + if invoke_just(cwd, just_args) != 0: + # Say which one, as a verb can fan out over a great many directories. + where = f" in {cwd}" if cwd else "" + error(f"{cmd} failed{where}: just {' '.join(just_args)}") + return 1 + return 0 + + +def main(): + argv = sys.argv[1:] + if not argv: + error("No command provided") + return 1 + return forward(argv[0], argv[1:]) + + +if __name__ == "__main__": + try: + sys.exit(main()) + except KeyboardInterrupt: + sys.exit(128 + 2) diff --git a/misc/just/justfile b/misc/just/justfile new file mode 100644 index 000000000000..bfa7bed4db2e --- /dev/null +++ b/misc/just/justfile @@ -0,0 +1,2 @@ +format *ARGS=".": + npx prettier --write {{ ARGS }} diff --git a/misc/just/language_tests.py b/misc/just/language_tests.py new file mode 100755 index 000000000000..988a77d73cf3 --- /dev/null +++ b/misc/just/language_tests.py @@ -0,0 +1,66 @@ +#!/usr/bin/env python3 +"""Run a whole language test suite for CI. + +Called from just recipes as: + python3 language_tests.py ROOT [ARG...] + +Arguments are already split by `just` (see `set lists`). The first one must be a test +root, which is used to locate the justfile implementing `test` for that suite. +""" + +import os +import subprocess +import sys +from pathlib import Path + + +def main(): + argv = sys.argv[1:] + if not argv: + print("Usage: language_tests.py ROOT [ARG...]", file=sys.stderr) + return 1 + + semmle_code = Path(os.environ["SEMMLE_CODE"]) + # Test roots are absolute, as justfiles build them from `source_dir()`. We run from + # the internal checkout, so relativize them there to keep command lines readable. + # Anything else (flags, environment assignments, relative paths) is passed verbatim. + args = [ + os.path.relpath(arg, semmle_code) if os.path.isabs(arg) else arg + for arg in argv + if arg + ] + + just = os.environ.get("JUST_EXECUTABLE", "just") + + # Find the nearest justfile at or above the first root + justfile_dir = Path(args[0]) + while not (semmle_code / justfile_dir / "justfile").exists(): + parent = justfile_dir.parent + if parent == justfile_dir: + print(f"No justfile found above {args[0]}", file=sys.stderr) + return 1 + justfile_dir = parent + + invocation = [ + just, + "--justfile", + str(justfile_dir / "justfile"), + "test", + "--all-checks", + "--codeql=built", + *args, + ] + + print(f"-> just {' '.join(invocation[1:])}") + try: + subprocess.run(invocation, check=True, cwd=semmle_code) + except subprocess.CalledProcessError as e: + return e.returncode + return 0 + + +if __name__ == "__main__": + try: + sys.exit(main()) + except KeyboardInterrupt: + sys.exit(128 + 2) diff --git a/misc/just/lib.just b/misc/just/lib.just new file mode 100644 index 000000000000..8ce335677287 --- /dev/null +++ b/misc/just/lib.just @@ -0,0 +1,31 @@ +# Helper recipes + +import "build.just" +import "format.just" + +# Run language tests for LANGUAGE. +# +# Arguments tagged with `--all-checks=` are held back and only applied when `--all-checks` +# or `+` is passed along, which is how per-language justfiles express the extra checks CI +# runs on top of the default ones. +[no-cd] +[no-exit-message] +[positional-arguments] +@_codeql_test LANGUAGE *ARGS: + {{ py }} "{{ source_dir() }}/codeql_test_run.py" "$@" + +# Run a whole language test suite. The first argument must be a test root. This is +# intended to be called by CI +[no-cd] +[no-exit-message] +[positional-arguments] +@_language_tests *ARGS: _require_semmle_code + {{ py }} "{{ source_dir() }}/language_tests.py" "$@" + +# Run integration tests. Requires an internal repository checkout +[no-cd] +[no-exit-message] +[positional-arguments] +@_integration_test *ARGS: _require_semmle_code + echo "$CMD_BEGIN$SEMMLE_CODE/tools/pytest --codeql=build-as-test $*$CMD_END" + "$SEMMLE_CODE/tools/pytest" --codeql=build-as-test "$@" diff --git a/misc/just/run_on_files.py b/misc/just/run_on_files.py new file mode 100644 index 000000000000..658fbe79d238 --- /dev/null +++ b/misc/just/run_on_files.py @@ -0,0 +1,120 @@ +"""Run a command on the files matching the given patterns below the given paths. + +This is a portable `find ... -name -exec {} +`. It exists +because `find` is an unrelated program on Windows, and because a shell command +substitution splits the file names it produces on whitespace, which mangles the many +paths in this repository that contain spaces. + +The command is run once per batch of file names rather than once per file, and the +batches are sized so that no single command line runs into a length limit. Nothing is +run at all when no file matches. + +Usage: run_on_files.py [option...] [,...] [...] + -- [...] + +Options: + --exclude leave out files whose path matches, repeatable + --absolute pass absolute file names, needed when the command runs elsewhere + +The command is separated from the paths by the last `--`, so that it may contain one of +its own, as `bazel run -- ...` does. +""" + +import os +import subprocess +import sys +from fnmatch import fnmatch +from pathlib import Path + +def batch_limit(): + """How many characters of file names to put on one command line. + + Windows caps a whole command line at 32767 characters. Elsewhere the cap is + `ARG_MAX`, which the environment is counted against as well, so that is taken off + along with some slack. This is worth doing rather than assuming the tightest of the + two: `ARG_MAX` is 2MB on Linux, which turns the couple of thousand QL files of a + language into a single invocation rather than several. + """ + if sys.platform == "win32": + return 30000 + try: + arg_max = os.sysconf("SC_ARG_MAX") + except (ValueError, OSError): + return 30000 + environment = sum(len(name) + len(value) + 2 for name, value in os.environ.items()) + return max(4096, arg_max - environment - 4096) + + +def files_under(paths, patterns, excludes=(), absolute=False): + """Collect the files matching one of the patterns at or below each path. + + Patterns are matched against the file name, as bazel files are identified by name + rather than by extension. Exclusions are matched against the whole path instead, + which is how a directory of generated files is left alone. + + Symbolic links are not followed, which is what keeps the `bazel-*` convenience + links out of the walk. + """ + + def wanted(path): + return any(fnmatch(path.name, p) for p in patterns) and not any( + fnmatch(str(path), e) for e in excludes + ) + + found = set() + for path in map(Path, paths): + if path.is_file(): + if wanted(path): + found.add(path) + continue + for directory, _, names in os.walk(path): + found.update(p for p in map(Path(directory).joinpath, names) if wanted(p)) + return sorted(os.path.abspath(p) if absolute else str(p) for p in found) + + +def batched(files, limit): + """Split file names into groups that each fit on one command line.""" + batch, length = [], 0 + for file in files: + if batch and length + len(file) + 1 > limit: + yield batch + batch, length = [], 0 + batch.append(file) + length += len(file) + 1 + if batch: + yield batch + + +def parse_options(args): + """Take the leading options off the argument list, returning what they asked for.""" + excludes, absolute = [], False + while args and args[0] != "--" and args[0].startswith("--"): + option = args.pop(0) + if option == "--absolute": + absolute = True + elif option == "--exclude": + excludes.append(args.pop(0)) + else: + sys.exit(f"run_on_files.py: unknown option {option}") + return excludes, absolute + + +def main(): + args = sys.argv[1:] + excludes, absolute = parse_options(args) + patterns = set(args[0].split(",")) + rest = args[1:] + # The command may hold a `--` of its own, so the paths start after the last one. + separator = len(rest) - 1 - rest[::-1].index("--") + command, paths = rest[:separator], rest[separator + 1 :] + + files = files_under(paths, patterns, excludes, absolute) + limit = batch_limit() - sum(len(arg) + 1 for arg in command) + status = 0 + for batch in batched(files, limit): + status = subprocess.run([*command, *batch]).returncode or status + return status + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/misc/just/semmle-code-stub.just b/misc/just/semmle-code-stub.just new file mode 100644 index 000000000000..14733ffb648e --- /dev/null +++ b/misc/just/semmle-code-stub.just @@ -0,0 +1 @@ +export SEMMLE_CODE := "" diff --git a/python/justfile b/python/justfile new file mode 100644 index 000000000000..33580aa1e05c --- /dev/null +++ b/python/justfile @@ -0,0 +1,26 @@ +import '../lib.just' +import 'ql/justfile' + +[group('build')] +build: (_build_dist "python") + +# Long filename needed for extractor tests (too long for Git on Windows) +[no-cd] +@_ensure_long_filename: + #!/usr/bin/env bash + longfile="$SEMMLE_CODE/ql/python/ql/test/extractor-tests/long_path/really_rather_too_long_for_windows_path_length/with_unecessarily_longwinded_and_verbose_sub_folder/extremely_long_module_name_with_lots_of_digits_at_the_end_000000000000000000000000000000000000000000000000000000000000000000/test0000000000000000000000000000000000000000000000000000000.py" + mkdir -p "$(dirname "$longfile")" + touch "$longfile" + +_tests := source_dir() / 'ql/test' + +_shared_roots := [_tests / 'library-tests', _tests / 'query-tests', _tests / 'extractor-tests', _tests / 'experimental'] + +roots_2 := _shared_roots ++ [_tests / '2'] +roots_3 := _shared_roots ++ [_tests / 'modelling', _tests / '3'] + +[group('test')] +language-tests-2 *EXTRA_ARGS: _ensure_long_filename (_language_tests (roots_2 ++ _v2_env ++ EXTRA_ARGS)) + +[group('test')] +language-tests-3 *EXTRA_ARGS: _ensure_long_filename (_language_tests (roots_3 ++ _v3_env ++ EXTRA_ARGS)) diff --git a/python/ql/integration-tests/justfile b/python/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/python/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/python/ql/justfile b/python/ql/justfile new file mode 100644 index 000000000000..45ed8d733cff --- /dev/null +++ b/python/ql/justfile @@ -0,0 +1,12 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" + +python_version := env("python_version", "3") + +_v2_env := ['CODEQL_EXTRACTOR_PYTHON_ANALYSIS_VERSION=2', 'CODEQL_PYTHON_LEGACY_TEST_EXTRACTION_VERSION=2'] +_v3_env := ['CODEQL_PYTHON_LEGACY_TEST_EXTRACTION_VERSION=3'] +_python_env := if python_version == "2" { _v2_env } else { _v3_env } diff --git a/python/ql/test/justfile b/python/ql/test/justfile new file mode 100644 index 000000000000..f12a08176a06 --- /dev/null +++ b/python/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := _python_env + +all_checks := default_db_checks ++ ['--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "python" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/ruby/justfile b/ruby/justfile new file mode 100644 index 000000000000..b9cc748f169f --- /dev/null +++ b/ruby/justfile @@ -0,0 +1,9 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "ruby") + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/ruby/ql/integration-tests/justfile b/ruby/ql/integration-tests/justfile new file mode 100644 index 000000000000..8392c2b3ba40 --- /dev/null +++ b/ruby/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_integration_test ARGS) diff --git a/ruby/ql/justfile b/ruby/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/ruby/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/ruby/ql/test/justfile b/ruby/ql/test/justfile new file mode 100644 index 000000000000..8b671785f258 --- /dev/null +++ b/ruby/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +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] + +[no-cd] +test *ARGS=".": (_codeql_test "ruby" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/rust/justfile b/rust/justfile new file mode 100644 index 000000000000..9877da7b9f1f --- /dev/null +++ b/rust/justfile @@ -0,0 +1,17 @@ +import '../lib.just' + +install: (_bazel ['run', '@codeql//rust:install']) + +[group('build')] +build: (_if_not_on_ci_just ['generate', source_dir()]) (_build_dist "rust") + +generate: (_bazel ['run', '@codeql//rust/codegen']) + +lint: (_run_in source_dir() ['python3', 'lint.py']) + +format: (_run_in source_dir() ['python3', 'lint.py', '--format-only']) + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) diff --git a/rust/ql/integration-tests/justfile b/rust/ql/integration-tests/justfile new file mode 100644 index 000000000000..fa96473894df --- /dev/null +++ b/rust/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_if_not_on_ci_just ['generate', source_dir()]) (_integration_test ARGS) diff --git a/rust/ql/justfile b/rust/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/rust/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/rust/ql/test/justfile b/rust/ql/test/justfile new file mode 100644 index 000000000000..5fad8ab2d0d3 --- /dev/null +++ b/rust/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +all_checks := default_db_checks ++ ['--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "rust" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/swift/justfile b/swift/justfile new file mode 100644 index 000000000000..b565923cae08 --- /dev/null +++ b/swift/justfile @@ -0,0 +1,18 @@ +import '../lib.just' + +install: (_bazel ['run', '@codeql//swift:install']) + +[group('build')] +build: (_build_dist "swift") + +generate: (_bazel ['run', '@codeql//swift/codegen']) + +format *ARGS=".": (_format_cpp ARGS) + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) + +[group('test')] +extra-tests: (_sembuild "target/test/check-queries-swift") (_sembuild "target/test/check-db-upgrades-swift") (_sembuild "target/test/check-db-downgrades-swift") diff --git a/swift/ql/integration-tests/justfile b/swift/ql/integration-tests/justfile new file mode 100644 index 000000000000..097faf5baebf --- /dev/null +++ b/swift/ql/integration-tests/justfile @@ -0,0 +1,8 @@ +import "../../../lib.just" + +# Integration tests are slow and need an internal checkout, so they only run when +# asked for by name. +explicit_verbs := ['test'] + +[no-cd] +test *ARGS=".": (_just "generate") (_integration_test ARGS) diff --git a/swift/ql/justfile b/swift/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/swift/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/swift/ql/test/justfile b/swift/ql/test/justfile new file mode 100644 index 000000000000..d4d752eed15b --- /dev/null +++ b/swift/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +all_checks := default_db_checks ++ ['--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "swift" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/unified/extractor/justfile b/unified/extractor/justfile new file mode 100644 index 000000000000..f6a6417ed867 --- /dev/null +++ b/unified/extractor/justfile @@ -0,0 +1,4 @@ +import '../../lib.just' + +[group('test')] +test *BAZEL_ARGS: (_bazel (['test', '--build_tests_only', '@codeql//unified/extractor/...'] ++ BAZEL_ARGS)) diff --git a/unified/justfile b/unified/justfile new file mode 100644 index 000000000000..610ec84901c0 --- /dev/null +++ b/unified/justfile @@ -0,0 +1,12 @@ +import '../lib.just' + +[group('build')] +build: (_build_dist "unified") + +roots := [source_dir() / 'ql/test'] + +[group('test')] +language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) + +[group('test')] +extractor-tests *BAZEL_ARGS: (_bazel (['test', '--build_tests_only', '@codeql//unified/...'] ++ BAZEL_ARGS)) diff --git a/unified/ql/justfile b/unified/ql/justfile new file mode 100644 index 000000000000..ff0e4c1090f3 --- /dev/null +++ b/unified/ql/justfile @@ -0,0 +1,6 @@ +import "../../lib.just" + +[no-cd] +format *ARGS=".": (_format_ql ARGS) + +consistency_queries := source_dir() / "consistency-queries" diff --git a/unified/ql/test/justfile b/unified/ql/test/justfile new file mode 100644 index 000000000000..9367615ddf6d --- /dev/null +++ b/unified/ql/test/justfile @@ -0,0 +1,12 @@ +import "../justfile" + +# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when +# asked for by name. +explicit_verbs := ['test'] + +base_flags := [] + +all_checks := default_db_checks ++ ['--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] + +[no-cd] +test *ARGS=".": (_codeql_test "unified" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) diff --git a/unified/swift-syntax-rs/justfile b/unified/swift-syntax-rs/justfile new file mode 100644 index 000000000000..021bb0e9e75e --- /dev/null +++ b/unified/swift-syntax-rs/justfile @@ -0,0 +1,4 @@ +import '../../lib.just' + +[group('test')] +test *BAZEL_ARGS: (_bazel (['test', '--build_tests_only', '@codeql//unified/swift-syntax-rs/...'] ++ BAZEL_ARGS)) From e2892eec9f4c2fdb0cf8705045979ffa270782ab Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:50:53 +0200 Subject: [PATCH 04/15] Just: start formatting bazel files, including in the internal checkout `format` learns to reformat bazel files (`BUILD.bazel`, `WORKSPACE`, `*.bzl`) through buildifier, alongside the existing QL and C++ formatters. The root justfile owns it, since bazel files are spread across the tree rather than gathered under one language; the buildifier target cannot be driven directly, since its generated wrapper ignores the paths given to it, so the binary is invoked instead. Formatting reports which files it rewrote and nothing else, filtering out buildifier's and bazel's noise about files left untouched. The internal checkout gets bazel formatting too, rather than being excluded because the target here is a bazel dev dependency unreachable from a build rooted there: both repositories depend on the same buildifier binary as the root module of their own checkout, so asking for it directly resolves either way. What differs is which bazel to ask and from where, which the file runner can now be told. Also stops the distribution install log from printing on every command that happens to need one, showing it only when the install actually fails. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- justfile | 5 ++ misc/bazel/buildifier/BUILD.bazel | 8 +++ misc/just/README.md | 17 +++++ misc/just/build.just | 8 ++- misc/just/format.just | 40 ++++++++++- misc/just/run_on_files.py | 116 +++++++++++++++++++++--------- 6 files changed, 158 insertions(+), 36 deletions(-) diff --git a/justfile b/justfile index 94cf7d2f4bb3..6fe875f6facb 100644 --- a/justfile +++ b/justfile @@ -2,3 +2,8 @@ import 'lib.just' import 'misc/just/forward.just' + +# bazel files live all over the repository rather than under any one language, so they +# are formatted from here. `format` itself is the forwarder, hence `_root_`; see +# misc/just/README.md. +_root_format *ARGS=".": (_format_bazel ARGS) diff --git a/misc/bazel/buildifier/BUILD.bazel b/misc/bazel/buildifier/BUILD.bazel index b71712515595..ec7a152a144d 100644 --- a/misc/bazel/buildifier/BUILD.bazel +++ b/misc/bazel/buildifier/BUILD.bazel @@ -8,3 +8,11 @@ buildifier( ], lint_mode = "fix", ) + +# The binary behind the target above, which formats the paths it is given rather than +# always the whole workspace. `just format` goes through this so that formatting a +# directory formats that directory. +alias( + name = "binary", + actual = "@buildifier_prebuilt//:buildifier", +) diff --git a/misc/just/README.md b/misc/just/README.md index 74b64283b7a6..7acb6dd35f21 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -32,6 +32,23 @@ broader version of it: `rust` formats Rust sources while `rust/ql` formats QL, s `just format rust` has to do both. A recipe that only arrived through `import` is the same job, though, and runs once. +A repository root forwards every verb, which leaves it no way to answer one itself: a +recipe written next to the `import` overrides the imported one and takes the forwarder's +place, so `just format cpp` would stop finding anything. The root spells its own +implementation `_root_` instead, and the forwarder picks that up wherever the +plain name turns out to be the forwarder's own: + +```just +import 'misc/just/forward.just' + +_root_format *ARGS=".": (_format_bazel ARGS) +``` + +This is for work that belongs to no single directory. bazel files are the case in hand: +they sit throughout the tree rather than under any one language, so formatting them is +the root's job, and taking the argument keeps `just format cpp` to the bazel files under +`cpp`. + A directory that only makes sense when named explicitly can opt out of being found from above: diff --git a/misc/just/build.just b/misc/just/build.just index f564c4aa9041..f9739f40f397 100644 --- a/misc/just/build.just +++ b/misc/just/build.just @@ -7,8 +7,14 @@ _build_dist LANGUAGE: _require_semmle_code (_maybe_build_dist LANGUAGE) # Build the language-specific distribution if we are in an internal repository checkout # Otherwise, do nothing +# +# The install log is worth reading only when the install fails, and printing it +# regardless buried whatever was actually asked for underneath it. Note that bazel is +# not quietened any further than that here: unlike an error, a failing test's log is not +# something `--ui_event_filters` can let back through, and a build this long is one to +# see the progress of. [no-exit-message] -_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=all') '# using codeql from PATH, if any') +_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=errors') '# using codeql from PATH, if any') # Call bazel. Uses our official bazel wrapper if we are in an internal repository checkout [no-cd] diff --git a/misc/just/format.just b/misc/just/format.just index 1c28344bc422..c7276bc011bb 100644 --- a/misc/just/format.just +++ b/misc/just/format.just @@ -6,15 +6,45 @@ _py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" } _cpp_formatter := if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" } +# The `buildifier` bazel target always covers the whole workspace, so the binary behind +# it is used instead and given paths. Both repositories depend on it, each as the root +# module of its own checkout, so the same label resolves either way; what differs is +# which bazel to ask and from where, as the internal repository's workspace encloses +# this one and a nested checkout would otherwise be taken for the root. + +_bazel_command := if SEMMLE_CODE != "" { '"$SEMMLE_CODE/tools/bazel"' } else { "bazel" } + +_bazel_workspace := if SEMMLE_CODE != "" { '"$SEMMLE_CODE"' } else { quote(parent_directory(parent_directory(source_dir()))) } + +_bazel_formatter := _bazel_command + " run --noshow_progress --ui_event_filters=,+error,+fail @buildifier_prebuilt//:buildifier --" + +# bazel files are named rather than suffixed, and buildifier has no exclude option of its +# own, so the generated files skipped by the target above are skipped here too. +# +# As with the QL formatter, buildifier only names what it rewrote if it also accounts for +# every file it did not, so that accounting is dropped. It counts the warnings it could +# not fix there, which are left for linting to report rather than raised on every format. +_bazel_names := "BUILD,BUILD.*,WORKSPACE,WORKSPACE.*,*.bazel,*.bzl,*.sky" + +_bazel_generated := "*misc/bazel/3rdparty/*_deps/*" + +_bazel_accounting := ': applied fixes, [0-9]+ warnings left$' + # `codeql query format` and `clang-format` take files rather than directories, so the # files are collected by `run_on_files.py`. Arguments are passed positionally so that # paths containing spaces survive, of which this repository has many. +# +# The files that were rewritten are worth reporting, but `codeql query format` only +# names those once it also names every file it leaves alone, which buries them under +# thousands of lines. So it is asked for all of it and the lines about files it did not +# touch are dropped. Only those are dropped, so errors still come through, as does +# anything unforeseen. [no-cd] [no-exit-message] [positional-arguments] _format_ql +ARGS: (_maybe_build_dist "nolang") - {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" .ql,.qll {{ _ql_formatter }} query format --in-place -v -- "$@"{{ cmd_sep }} + {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" --drop '^Formatting ' --drop '^No change for ' "*.ql,*.qll" {{ _ql_formatter }} query format --in-place -v -- "$@"{{ cmd_sep }} [no-cd] [no-exit-message] @@ -26,4 +56,10 @@ _format_py *ARGS=".": [no-exit-message] [positional-arguments] _format_cpp *ARGS=".": - {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" .h,.cpp {{ _cpp_formatter }} -i --verbose -- "$@"{{ cmd_sep }} + {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" "*.h,*.cpp" {{ _cpp_formatter }} -i -- "$@"{{ cmd_sep }} + +[no-cd] +[no-exit-message] +[positional-arguments] +_format_bazel *ARGS=".": + {{ cmd_sep }}export MSYS2_ARG_CONV_EXCL="*"; {{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --drop '{{ _bazel_accounting }}' --exclude '{{ _bazel_generated }}' "{{ _bazel_names }}" {{ _bazel_formatter }} -mode=fix -lint=fix -v -- "$@"{{ cmd_sep }} diff --git a/misc/just/run_on_files.py b/misc/just/run_on_files.py index 658fbe79d238..a28db75d62ed 100644 --- a/misc/just/run_on_files.py +++ b/misc/just/run_on_files.py @@ -8,19 +8,11 @@ The command is run once per batch of file names rather than once per file, and the batches are sized so that no single command line runs into a length limit. Nothing is run at all when no file matches. - -Usage: run_on_files.py [option...] [,...] [...] - -- [...] - -Options: - --exclude leave out files whose path matches, repeatable - --absolute pass absolute file names, needed when the command runs elsewhere - -The command is separated from the paths by the last `--`, so that it may contain one of -its own, as `bazel run -- ...` does. """ +import argparse import os +import re import subprocess import sys from fnmatch import fnmatch @@ -85,34 +77,92 @@ def batched(files, limit): yield batch -def parse_options(args): - """Take the leading options off the argument list, returning what they asked for.""" - excludes, absolute = [], False - while args and args[0] != "--" and args[0].startswith("--"): - option = args.pop(0) - if option == "--absolute": - absolute = True - elif option == "--exclude": - excludes.append(args.pop(0)) - else: - sys.exit(f"run_on_files.py: unknown option {option}") - return excludes, absolute +def parse_args(): + """Work out what to run, on which files, and what to hide of what it says.""" + parser = argparse.ArgumentParser( + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + usage="%(prog)s [option...] [,...] " + " [...] -- [...]", + ) + parser.add_argument( + "--exclude", + action="append", + default=[], + metavar="", + help="leave out files whose path matches, repeatable", + ) + parser.add_argument( + "--absolute", + action="store_true", + help="pass absolute file names, needed when the command runs elsewhere", + ) + parser.add_argument( + "--chdir", + metavar="", + help="run the command from here, for one that must be run from a project root", + ) + parser.add_argument( + "--drop", + action="append", + default=[], + metavar="", + help="hide matching lines of the command's output, repeatable", + ) + parser.add_argument( + "patterns", + metavar="[,...]", + type=lambda patterns: set(patterns.split(",")), + help="what to match file names against", + ) + parser.add_argument( + "rest", + nargs=argparse.REMAINDER, + metavar=" [...] -- [...]", + help="the command, then the paths to search, separated by the last `--` so " + "that the command may contain one of its own", + ) + args = parser.parse_args() + if "--" not in args.rest: + parser.error("the paths must be separated from the command by `--`") + separator = len(args.rest) - 1 - args.rest[::-1].index("--") + args.command, args.paths = args.rest[:separator], args.rest[separator + 1 :] + if not args.command: + parser.error("no command given") + return args + + +def run(command, drops, chdir=None): + """Run the command, hiding the lines of its output that were asked to be hidden. + + Told nothing to hide, the command keeps this process' own output streams, so that + it can do as it likes with them. Otherwise its diagnostics are read a line at a + time and passed on as they arrive, which is what keeps a long run's progress + visible. Only what was named is hidden, so an unforeseen message still gets out. + + Note that these tools report on their progress over standard error rather than + standard output, which is left alone here. + """ + if not drops: + return subprocess.run(command, cwd=chdir).returncode + hidden = re.compile("|".join(drops)) + process = subprocess.Popen( + command, cwd=chdir, stderr=subprocess.PIPE, text=True, bufsize=1 + ) + for line in process.stderr: + if not hidden.search(line): + sys.stderr.write(line) + sys.stderr.flush() + return process.wait() def main(): - args = sys.argv[1:] - excludes, absolute = parse_options(args) - patterns = set(args[0].split(",")) - rest = args[1:] - # The command may hold a `--` of its own, so the paths start after the last one. - separator = len(rest) - 1 - rest[::-1].index("--") - command, paths = rest[:separator], rest[separator + 1 :] - - files = files_under(paths, patterns, excludes, absolute) - limit = batch_limit() - sum(len(arg) + 1 for arg in command) + args = parse_args() + files = files_under(args.paths, args.patterns, args.exclude, args.absolute) + limit = batch_limit() - sum(len(argument) + 1 for argument in args.command) status = 0 for batch in batched(files, limit): - status = subprocess.run([*command, *batch]).returncode or status + status = run([*args.command, *batch], args.drop, args.chdir) or status return status From 4bda24bbecc339c7678dbfed4503454308a42b54 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Tue, 15 Sep 2026 15:19:36 +0200 Subject: [PATCH 05/15] Just: drop the CLI from the reason a suite must be named Needing a CodeQL CLI is not a reason to hide a suite, as every QL recipe here needs one and the rest stay discoverable. Being slow is the whole of it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- actions/ql/test/justfile | 3 +-- cpp/ql/test/justfile | 3 +-- csharp/ql/test/justfile | 3 +-- go/ql/test/justfile | 3 +-- java/ql/test/justfile | 3 +-- javascript/ql/test/justfile | 3 +-- misc/just/README.md | 6 +++--- python/ql/test/justfile | 3 +-- ruby/ql/test/justfile | 3 +-- rust/ql/test/justfile | 3 +-- swift/ql/test/justfile | 3 +-- unified/ql/test/justfile | 3 +-- 12 files changed, 14 insertions(+), 25 deletions(-) diff --git a/actions/ql/test/justfile b/actions/ql/test/justfile index 8c06f3e5c155..a824f3029972 100644 --- a/actions/ql/test/justfile +++ b/actions/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/cpp/ql/test/justfile b/cpp/ql/test/justfile index 4ab3ef69856a..7ccd81541018 100644 --- a/cpp/ql/test/justfile +++ b/cpp/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := ['--include-location-in-star'] diff --git a/csharp/ql/test/justfile b/csharp/ql/test/justfile index 3efa95d340ca..ba3e238580c5 100644 --- a/csharp/ql/test/justfile +++ b/csharp/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/go/ql/test/justfile b/go/ql/test/justfile index 60b5d78b053a..e4f9665c1773 100644 --- a/go/ql/test/justfile +++ b/go/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/java/ql/test/justfile b/java/ql/test/justfile index 53a5d2d9dee3..0c0d98652c50 100644 --- a/java/ql/test/justfile +++ b/java/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] # The Kotlin extractor must see the diagnostic limit set, but blank: hence the single diff --git a/javascript/ql/test/justfile b/javascript/ql/test/justfile index 18daff51c273..366b4e1e43dd 100644 --- a/javascript/ql/test/justfile +++ b/javascript/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/misc/just/README.md b/misc/just/README.md index 7acb6dd35f21..0999dc2d1220 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -63,9 +63,9 @@ covered more than it did. That listing is part of the account of what ran and le exit status alone; only a verb that matched nothing at all fails. The QL test suites use this: `test` on a language runs the whole suite, which takes a -long time and needs a CodeQL CLI, so that has to be asked for by name. Integration tests -and the sharded Kotlin suites that CI runs opt out for the same reason. What is left -discoverable from above is what is cheap enough to run without meaning to. +long time, so that has to be asked for by name. Integration tests and the sharded +Kotlin suites that CI runs opt out for the same reason. What is left discoverable from +above is what is cheap enough to run without meaning to. Being an ordinary variable, `explicit_verbs` is inherited by justfiles importing one that sets it. That is normally what is wanted, as importing a suite's justfile means diff --git a/python/ql/test/justfile b/python/ql/test/justfile index f12a08176a06..0f44a489e82f 100644 --- a/python/ql/test/justfile +++ b/python/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := _python_env diff --git a/ruby/ql/test/justfile b/ruby/ql/test/justfile index 8b671785f258..9673cebe0370 100644 --- a/ruby/ql/test/justfile +++ b/ruby/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/rust/ql/test/justfile b/rust/ql/test/justfile index 5fad8ab2d0d3..8d5d6c05da2d 100644 --- a/rust/ql/test/justfile +++ b/rust/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/swift/ql/test/justfile b/swift/ql/test/justfile index d4d752eed15b..6f15ac6d0723 100644 --- a/swift/ql/test/justfile +++ b/swift/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] diff --git a/unified/ql/test/justfile b/unified/ql/test/justfile index 9367615ddf6d..1ca509465bd8 100644 --- a/unified/ql/test/justfile +++ b/unified/ql/test/justfile @@ -1,7 +1,6 @@ import "../justfile" -# A whole language test suite is slow and needs a CodeQL CLI, so it only runs when -# asked for by name. +# A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] base_flags := [] From 9306593e297a97ddfd59e2d311941dbbe3996556 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:50:54 +0200 Subject: [PATCH 06/15] Just: let each repository format only its own bazel files Asking bazel from this repository's root formatted a checkout enclosing this one with this repository's own buildifier version and exclusions, rather than the other's own. Formatting now bounds the files to the root doing the asking, so each repository formats what it owns, with its own pin, and a verb spanning both is answered once by each root. Along with that: - a repository can name several sets of generated files to exclude, rather than spelling the option twice - a file named `BUILD.` that is not `BUILD.bazel` is not a bazel file, since bazel only knows `BUILD`/`WORKSPACE` by name and the rest by extension; matching such templates failed every format whose scope contained one - batches are sized to the argument limit a command actually hands on to a child process, not to the shell's own line length limit - a directory's own formatter recipe no longer looks up its argument twice, once itself and once via `cd` - `[no-cd]` is documented, since the forwarder relies on it to keep a relative argument meaning the caller's directory - the formatter's exclusions are back in step with the canonical bazel target's, which also excludes `.git` Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- misc/just/README.md | 12 ++++++++-- misc/just/format.just | 44 +++++++++++++++++++++++++----------- misc/just/justfile | 4 +++- misc/just/run_on_files.py | 47 +++++++++++++++++++++++++++++++++------ 4 files changed, 84 insertions(+), 23 deletions(-) diff --git a/misc/just/README.md b/misc/just/README.md index 0999dc2d1220..81b04c4011ca 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -23,8 +23,9 @@ The core of the functionality is given by forwarding. The idea is that: - finally, the forwarder also looks _below_ each argument, so that `just test ql/cpp` runs the tests defined underneath it. The argument only says where to look in this case, so each recipe found is run on its own directory rather than being passed the - argument. Several may be found, in which case they run sequentially: `just format - ql/cpp` formats everything under `ql/cpp` that knows how to format itself. + argument. Several may be found, in which case they run sequentially: + `just format ql/cpp` formats everything under `ql/cpp` that knows how to format + itself. Both directions are searched, and every distinct recipe found runs. This matters because a verb higher up is usually doing a different job from one further down rather than a @@ -49,6 +50,13 @@ they sit throughout the tree rather than under any one language, so formatting t the root's job, and taking the argument keeps `just format cpp` to the bazel files under `cpp`. +Being a recipe like any other, a `_root_` is inherited by a justfile importing the +one defining it, which is how the internal repository gets this one for free. It runs +once either way, as the two spellings are the same recipe. A root that defines its own +instead replaces it, and then both run, each over the files of the repository that +defines it: bazel formatting asks bazel from the root of the checkout the files belong +to, so that a repository formats its own files with its own pin. + A directory that only makes sense when named explicitly can opt out of being found from above: diff --git a/misc/just/format.just b/misc/just/format.just index c7276bc011bb..ba6885b590d8 100644 --- a/misc/just/format.just +++ b/misc/just/format.just @@ -7,26 +7,39 @@ _py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" } _cpp_formatter := if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" } # The `buildifier` bazel target always covers the whole workspace, so the binary behind -# it is used instead and given paths. Both repositories depend on it, each as the root -# module of its own checkout, so the same label resolves either way; what differs is -# which bazel to ask and from where, as the internal repository's workspace encloses -# this one and a nested checkout would otherwise be taken for the root. +# it is used instead and given paths. bazel is asked from the root of this repository, +# even when it sits inside another, and the files are bounded to that root as well: each +# repository then formats its own bazel files with the buildifier version it pins, and a +# verb aimed at a tree spanning both is answered once by each. This is the opposite of +# building, where a target needs the enclosing workspace to resolve at all, hence +# `_bazel` in build.just going the other way. +_bazel_workspace := quote(parent_directory(parent_directory(source_dir()))) -_bazel_command := if SEMMLE_CODE != "" { '"$SEMMLE_CODE/tools/bazel"' } else { "bazel" } - -_bazel_workspace := if SEMMLE_CODE != "" { '"$SEMMLE_CODE"' } else { quote(parent_directory(parent_directory(source_dir()))) } - -_bazel_formatter := _bazel_command + " run --noshow_progress --ui_event_filters=,+error,+fail @buildifier_prebuilt//:buildifier --" +_bazel_formatter := "bazel run --noshow_progress --ui_event_filters=,+error,+fail //misc/bazel/buildifier:binary --" # bazel files are named rather than suffixed, and buildifier has no exclude option of its -# own, so the generated files skipped by the target above are skipped here too. +# own, so the files skipped by the target above are skipped here too. That target formats +# the whole workspace at once and so cannot take the path this recipe is given, which is +# why the two run the same binary through different entry points. Their exclusions are +# therefore stated twice, in two places, in two syntaxes: keep them in step, or `just +# format` rewrites what pre-commit and CI deliberately leave alone. +# +# bazel knows `BUILD` and `WORKSPACE` by those names and everything else by the `.bazel` +# extension, so a file named `BUILD.` else is a template or a generator's input +# rather than a bazel file, and is none of the formatter's business to parse. +# +# Both lists are comma-separated, so a root defining its own `_root_format` can name +# several patterns in one variable. It has no need to repeat the ones here: the files +# they cover belong to this repository, which formats them itself. Exclusions match the +# path as walked rather than as spelled on the command line, so one naming a directory +# has to cover both the path it is reached by and the path it is walked from. # # As with the QL formatter, buildifier only names what it rewrote if it also accounts for # every file it did not, so that accounting is dropped. It counts the warnings it could # not fix there, which are left for linting to report rather than raised on every format. -_bazel_names := "BUILD,BUILD.*,WORKSPACE,WORKSPACE.*,*.bazel,*.bzl,*.sky" +_bazel_names := "BUILD,WORKSPACE,*.bazel,*.bzl,*.sky" -_bazel_generated := "*misc/bazel/3rdparty/*_deps/*" +_bazel_excluded := ".git/*,*/.git/*,*misc/bazel/3rdparty/*_deps/*" _bazel_accounting := ': applied fixes, [0-9]+ warnings left$' @@ -39,6 +52,11 @@ _bazel_accounting := ': applied fixes, [0-9]+ warnings left$' # thousands of lines. So it is asked for all of it and the lines about files it did not # touch are dropped. Only those are dropped, so errors still come through, as does # anything unforeseen. +# +# `[no-cd]` is what keeps a relative argument meaning the directory the caller is in. +# The forwarder reaches a recipe above its argument with `--justfile`, which otherwise +# runs it from that justfile's own directory: dropping the attribute would silently turn +# the default `.` into the whole repository, and the only symptom would be slowness. [no-cd] [no-exit-message] @@ -62,4 +80,4 @@ _format_cpp *ARGS=".": [no-exit-message] [positional-arguments] _format_bazel *ARGS=".": - {{ cmd_sep }}export MSYS2_ARG_CONV_EXCL="*"; {{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --drop '{{ _bazel_accounting }}' --exclude '{{ _bazel_generated }}' "{{ _bazel_names }}" {{ _bazel_formatter }} -mode=fix -lint=fix -v -- "$@"{{ cmd_sep }} + {{ cmd_sep }}export MSYS2_ARG_CONV_EXCL="*"; {{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --within {{ _bazel_workspace }} --drop '{{ _bazel_accounting }}' --exclude '{{ _bazel_excluded }}' "{{ _bazel_names }}" {{ _bazel_formatter }} -mode=fix -lint=fix -v -- "$@"{{ cmd_sep }} diff --git a/misc/just/justfile b/misc/just/justfile index bfa7bed4db2e..679295f47801 100644 --- a/misc/just/justfile +++ b/misc/just/justfile @@ -1,2 +1,4 @@ +[no-cd] +[positional-arguments] format *ARGS=".": - npx prettier --write {{ ARGS }} + npx prettier --write "$@" diff --git a/misc/just/run_on_files.py b/misc/just/run_on_files.py index a28db75d62ed..bed9c66b8b93 100644 --- a/misc/just/run_on_files.py +++ b/misc/just/run_on_files.py @@ -26,29 +26,43 @@ def batch_limit(): along with some slack. This is worth doing rather than assuming the tightest of the two: `ARG_MAX` is 2MB on Linux, which turns the couple of thousand QL files of a language into a single invocation rather than several. + + A single argument is capped far lower than the whole line, at 128KB on Linux, and a + command that hands its arguments on through a shell arrives as one of them. Batches + are kept below that too, as the resulting failure is reported by whatever did the + handing on rather than by anything naming this file. """ if sys.platform == "win32": return 30000 + single_argument = 100000 try: arg_max = os.sysconf("SC_ARG_MAX") except (ValueError, OSError): return 30000 environment = sum(len(name) + len(value) + 2 for name, value in os.environ.items()) - return max(4096, arg_max - environment - 4096) + return max(4096, min(arg_max - environment - 4096, single_argument)) -def files_under(paths, patterns, excludes=(), absolute=False): +def files_under(paths, patterns, excludes=(), absolute=False, within=None): """Collect the files matching one of the patterns at or below each path. Patterns are matched against the file name, as bazel files are identified by name rather than by extension. Exclusions are matched against the whole path instead, - which is how a directory of generated files is left alone. + which is how a directory of generated files is left alone. That path is the one the + walk built, so an exclusion has to allow for how the paths it is given are spelled: + `*//*` does not match what is walked from `` itself. + + A `within` directory bounds the result to the files below it, for a command that + answers for one project and may be handed a path reaching outside it. Symbolic links are not followed, which is what keeps the `bazel-*` convenience links out of the walk. """ + boundary = Path(within).resolve() if within else None def wanted(path): + if boundary is not None and not path.resolve().is_relative_to(boundary): + return False return any(fnmatch(path.name, p) for p in patterns) and not any( fnmatch(str(path), e) for e in excludes ) @@ -77,6 +91,16 @@ def batched(files, limit): yield batch +def comma_separated(value): + """Split an option value listing several patterns. + + Patterns tend to come in groups, and a justfile passes them as one variable, so they + are spelled as one argument here rather than repeated. Repeating the option works + too, which is what lets a list be extended rather than restated. + """ + return value.split(",") + + def parse_args(): """Work out what to run, on which files, and what to hide of what it says.""" parser = argparse.ArgumentParser( @@ -87,9 +111,10 @@ def parse_args(): ) parser.add_argument( "--exclude", - action="append", + action="extend", default=[], - metavar="", + type=comma_separated, + metavar="[,...]", help="leave out files whose path matches, repeatable", ) parser.add_argument( @@ -102,6 +127,12 @@ def parse_args(): metavar="", help="run the command from here, for one that must be run from a project root", ) + parser.add_argument( + "--within", + metavar="", + help="leave out files outside this directory, for a command answering for one " + "project that may be handed a path reaching beyond it", + ) parser.add_argument( "--drop", action="append", @@ -112,7 +143,7 @@ def parse_args(): parser.add_argument( "patterns", metavar="[,...]", - type=lambda patterns: set(patterns.split(",")), + type=comma_separated, help="what to match file names against", ) parser.add_argument( @@ -158,7 +189,9 @@ def run(command, drops, chdir=None): def main(): args = parse_args() - files = files_under(args.paths, args.patterns, args.exclude, args.absolute) + files = files_under( + args.paths, args.patterns, args.exclude, args.absolute, args.within + ) limit = batch_limit() - sum(len(argument) + 1 for argument in args.command) status = 0 for batch in batched(files, limit): From 3e29fbb6ea76edd9f8baa9a3ad032a9a6e9557d8 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:00 +0200 Subject: [PATCH 07/15] Just: explain why root recipes delegate instead of duplicating work Two comments record traps in the `_root_` pattern for whoever extends it next: a body added to it is easy to reach for, but that is exactly where a relative argument stops meaning the caller's directory; and two roots are told apart only by comparing their recipes with `doc` excluded, which silently collapses several of the paired-repo cases if that exclusion is removed as an obvious cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- misc/just/README.md | 6 ++++++ misc/just/forward_command.py | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/misc/just/README.md b/misc/just/README.md index 81b04c4011ca..b513d5067e99 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -50,6 +50,12 @@ they sit throughout the tree rather than under any one language, so formatting t the root's job, and taking the argument keeps `just format cpp` to the bazel files under `cpp`. +Note that this one delegates rather than doing the work itself. The forwarder reaches a +recipe above its argument with `--justfile`, which runs it from the directory of the +justfile defining it unless the recipe is `[no-cd]`. A `_root_` that grows a body +therefore reads its default `.` as the whole repository rather than the directory the +caller is in, so one that does its own work needs `[no-cd]` itself. + Being a recipe like any other, a `_root_` is inherited by a justfile importing the one defining it, which is how the internal repository gets this one for free. It runs once either way, as the two spellings are the same recipe. A root that defines its own diff --git a/misc/just/forward_command.py b/misc/just/forward_command.py index 1814a0617dac..e0dba8c59195 100644 --- a/misc/just/forward_command.py +++ b/misc/just/forward_command.py @@ -232,6 +232,13 @@ def find_justfiles_above(command, arg): recipe = implements(dump, command, argc) # These justfiles are nested, so a recipe that was seen already is one this # one merely imported, and the nearest spelling of it has been taken. + # + # Two repositories that each define a root recipe are not that case: the text + # can match while the workspace, the tool it runs and the paths it excludes all + # differ, so they have to stay apart. Nothing here says so. They are told apart + # only by the doc comment one of them happens to carry, which means dropping + # `doc` from this comparison silently discards an invocation unless a real + # discriminator arrives in the same change. if recipe is not None and recipe not in seen: seen.append(recipe) found.append((justfile, recipe)) From df5cf9f2e2610e0d5c476247470fa7e263e45261 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:09 +0200 Subject: [PATCH 08/15] Just: fix small per-language issues found while integrating the common verbs A handful of unrelated fixups surfaced while wiring each language into the shared verbs: - Go's 32-bit language test recipe was dropped by the rollout; restored - the Kotlin diagnostic limit reads as empty rather than as a lone space when unset - Rust's integration tests ask for codegen without also saying where to put it, which is now the callee's job - the C++ consistency queries moved earlier opt into implicit-this warnings, matching the rest of the C++ QL libraries Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- cpp/ql/consistency-queries/qlpack.yml | 1 + go/justfile | 8 ++++++++ java/ql/test/justfile | 5 ++--- rust/ql/integration-tests/justfile | 2 +- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/cpp/ql/consistency-queries/qlpack.yml b/cpp/ql/consistency-queries/qlpack.yml index fed0e22e17ba..303f2271be12 100644 --- a/cpp/ql/consistency-queries/qlpack.yml +++ b/cpp/ql/consistency-queries/qlpack.yml @@ -3,3 +3,4 @@ groups: [cpp, test, consistency-queries] dependencies: codeql/cpp-all: ${workspace} extractor: cpp +warnOnImplicitThis: true diff --git a/go/justfile b/go/justfile index e1ea5203166e..fa4c18266af6 100644 --- a/go/justfile +++ b/go/justfile @@ -5,5 +5,13 @@ build: (_build_dist "go") roots := [source_dir() / 'ql/test'] +# The `IncorrectIntegerConversion` query treats `math.MaxInt`/`math.MaxUint` differently on 32- and +# 64-bit targets, so we run its test under `GOARCH=386` as well. `GOOS=linux` because +# `GOOS=darwin GOARCH=386` is no longer supported. +roots_386 := [source_dir() / 'ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.qlref'] + [group('test')] language-tests *EXTRA_ARGS: (_language_tests (roots ++ EXTRA_ARGS)) + +[group('test')] +language-tests-386 *EXTRA_ARGS: (_language_tests (roots_386 ++ ['GOOS=linux', 'GOARCH=386'] ++ EXTRA_ARGS)) diff --git a/java/ql/test/justfile b/java/ql/test/justfile index 0c0d98652c50..aedf78381a05 100644 --- a/java/ql/test/justfile +++ b/java/ql/test/justfile @@ -3,9 +3,8 @@ import "../justfile" # A whole language test suite is slow, so it only runs when asked for by name. explicit_verbs := ['test'] -# The Kotlin extractor must see the diagnostic limit set, but blank: hence the single -# trailing space. -base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT= '] +# Kotlin tests may fail the diags.ql consistency test if the diagnostic limit is set. +base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT='] all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] diff --git a/rust/ql/integration-tests/justfile b/rust/ql/integration-tests/justfile index fa96473894df..2ee4b833c128 100644 --- a/rust/ql/integration-tests/justfile +++ b/rust/ql/integration-tests/justfile @@ -5,4 +5,4 @@ import "../../../lib.just" explicit_verbs := ['test'] [no-cd] -test *ARGS=".": (_if_not_on_ci_just ['generate', source_dir()]) (_integration_test ARGS) +test *ARGS=".": (_if_not_on_ci_just ['generate']) (_integration_test ARGS) From 1a783b5ba04171be49acf73c9127a61d213a6569 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:13 +0200 Subject: [PATCH 09/15] Just: document the override contract for shared formatting variables `set allow-duplicate-variables` lets an importing justfile assign a variable defined here and win, which is how a consuming root points the bazel formatter at its own workspace, buildifier and exclusions. That makes these variables an interface, not an implementation detail: `just` has no notion of an assignment that fails to override, so renaming one leaves a root parsing, listing and passing CI while silently reverting to the value here, and it stays invisible since the leading underscore that keeps these out of `just --list` also hides them from `--variables` and a bare `--evaluate`. Asked by name they do answer, which is the diagnostic worth reaching for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- misc/just/README.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/misc/just/README.md b/misc/just/README.md index b513d5067e99..16c91f90220a 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -63,6 +63,34 @@ instead replaces it, and then both run, each over the files of the repository th defines it: bazel formatting asks bazel from the root of the checkout the files belong to, so that a repository formats its own files with its own pin. +That last part is arranged by variables rather than by recipes. `set +allow-duplicate-variables` in `defs.just` lets an importing justfile assign a variable +defined here and have its value win, which is how a consuming root points the bazel +formatter at its own workspace, its own buildifier and its own exclusions. The leading +underscore says these are not meant to be run, not that they are private: any of them a +root might reasonably want to redirect is an interface between the two repositories. + +Renaming one is therefore a breaking change that nothing reports. `just` has no notion +of an assignment that fails to override, so a root assigning the old name keeps parsing, +keeps listing, keeps passing CI, and silently reverts to the value here. Worse, a root +overriding several loses only the renamed one, leaving a half-applied configuration: +total failure would land in a state someone designed, while partial failure lands in one +nobody has ever seen. + +Nothing can see it either, because the underscore that keeps these out of `just --list` +keeps them out of `--variables` and a bare `--evaluate` as well. Asked by name they do +answer, which is how a root checks that an override of its own still overrides anything: + +```sh +just --evaluate _bazel_excluded # what mine is now +just --justfile /justfile --evaluate _bazel_excluded # what it would be +``` + +A name that has gone says so rather than reporting an empty value. That is a diagnostic +to reach for once something looks wrong, though: it answers whether a name still exists, +not whether its meaning has changed, so it passes happily when the value here gains or +loses a pattern. Rename freely, but say so when handing the change over. + A directory that only makes sense when named explicitly can opt out of being found from above: From 567f6077ee36605e1afe74b46fb624f35cff0248 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:34 +0200 Subject: [PATCH 10/15] Just: make forwarded command output readable Two things made the output of a forwarded command hard to follow: the separator between commands was a fixed 56 dashes, reading as a short dash in a wide terminal rather than a break between commands; and the banner shown before each one was `just` echoing the recipe line verbatim, over 500 characters for the bazel one and carrying an unexpanded `"$@"` that matched no file in another shell. The separator is now drawn from `stty`-measured terminal width, repeating `#` rather than following it with dashes so the whole line reads as one thing, a shell comment, instead of two -- which also matters because the line is interpolated into a shell script, so a value that is not itself a shell comment would silently run. The width is exported as `JUST_CMD_RULE` so a forwarded verb inherits it instead of re-measuring in every justfile it reaches, and nothing changes without a terminal: piped output, CI logs and Windows still get the original 56 dashes. The banner is replaced by a `-> `-prefixed report of what the command actually is, built by the same file runner that decides which files matched, so it reports rather than merely intends. Buildifier's own output is filtered down to the files it actually rewrote, keeping only the tally of warnings it could not fix, since fix mode reports no other detail and always exits 0; its Starlark-file detection gains the one name this repository's list was missing (`*.star`); an exclusion is now matched against the absolute path as well as the given one, and resolved through symlinks first, both of which had let a repository excluding a nested one format it anyway; and naming a path that does not exist is now an error rather than silent, exit-0 success. Also folds in a few nearby fixes found the same way: opting out of MSYS2's path-like argument rewriting once, centrally, instead of per bazel-invoking recipe; printing the actual `error:` prefix instead of `{error}`, which was Python's interpolation syntax rather than just's; and moving each formatter's own values next to the recipe that reads them, instead of a shared header that had to be read whole to make sense of any one of them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- misc/bazel/buildifier/BUILD.bazel | 8 --- misc/just/README.md | 37 ++++++++++ misc/just/build.just | 4 +- misc/just/defs.just | 44 +++++++++++- misc/just/format.just | 115 ++++++++++++++---------------- misc/just/lib.just | 2 +- misc/just/run_on_files.py | 95 +++++++++++++++++++----- 7 files changed, 214 insertions(+), 91 deletions(-) diff --git a/misc/bazel/buildifier/BUILD.bazel b/misc/bazel/buildifier/BUILD.bazel index ec7a152a144d..b71712515595 100644 --- a/misc/bazel/buildifier/BUILD.bazel +++ b/misc/bazel/buildifier/BUILD.bazel @@ -8,11 +8,3 @@ buildifier( ], lint_mode = "fix", ) - -# The binary behind the target above, which formats the paths it is given rather than -# always the whole workspace. `just format` goes through this so that formatting a -# directory formats that directory. -alias( - name = "binary", - actual = "@buildifier_prebuilt//:buildifier", -) diff --git a/misc/just/README.md b/misc/just/README.md index 16c91f90220a..8d62b276bcce 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -149,3 +149,40 @@ to be understood by all of them. That is fine when they speak the same language, a broad `just test .` reaches bazel and pytest suites alike, and a flag meant for one of them will fail on the other. It fails rather than being quietly ignored, so the answer is to aim the verb at something narrower. + +# Command separators + +Commands are echoed between rules that span the terminal. The formatters echo a summary +rather than the line that runs: they name the formatter and what it is told to do, and +leave out the wrapper that collects the files, the patterns it walks and the flags that +only shape output. A leading `-> ` marks a line as that summary. `just -n` prints what +actually runs, which it does whether or not the recipe is `@`-quiet. + +Nothing is lost by this, as a banner has never been something to paste: the echoed line +carried an unexpanded `"$@"`, which matches no file in another shell, so pasting one +formatted nothing and exited 0. + +What a formatter says for itself is filtered down to what happened, as several name every +file they considered and most of them were left alone. One such line is kept on purpose: +buildifier's count of the warnings it could not fix, which is the only notice of them, +since it reports no detail in fix mode and exits 0 whether or not any remain. That tally +grows with the tree and is mostly lint about docstrings. Finding it tiresome is a reason +to configure what buildifier lints, never to widen the filter back over it, which would +take the warnings worth having along with the rest. + +Measuring the width means a `shell()` call, and that runs on every parse, so the result +is exported as `JUST_CMD_RULE` and an inherited value is preferred to measuring again. + +Inheritance crosses processes, which is what forwarding creates: a child per justfile +reached, each of them measuring nothing. A `mod` spawns no process, so a module measures +for itself, and the count is one per `mod` reached, however deeply nested, plus one for +the file itself. That is cheap, and modules agree anyway since they share a terminal, +but it is worth knowing before counting measurements. Presetting `JUST_CMD_RULE` skips +all of them, and is also how to fix the width in CI or in a recording — it is the whole +separator and ends up in a shell script, so every line of it has to be a comment, or it +runs. + +With no terminal to ask — a pipe, a log, a shell without `stty` — it falls back to a +fixed 57 columns, so logs and CI output are the same width every time. That is one +branch rather than a platform test: `just` runs `sh` everywhere, so Windows takes +whichever arm fits rather than a path of its own. diff --git a/misc/just/build.just b/misc/just/build.just index f9739f40f397..efb85aaa7e81 100644 --- a/misc/just/build.just +++ b/misc/just/build.just @@ -14,12 +14,12 @@ _build_dist LANGUAGE: _require_semmle_code (_maybe_build_dist LANGUAGE) # something `--ui_event_filters` can let back through, and a build this long is one to # see the progress of. [no-exit-message] -_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=errors') '# using codeql from PATH, if any') +_maybe_build_dist LANGUAGE: (_if_in_semmle_code (f'cd "$SEMMLE_CODE"; tools/bazel test //language-packs:intree-{{ LANGUAGE }}-as-test --test_output=errors') '# using codeql from PATH, if any') # Call bazel. Uses our official bazel wrapper if we are in an internal repository checkout [no-cd] [no-exit-message] -_bazel *ARGS: (_if_in_semmle_code 'cd "$SEMMLE_CODE"; MSYS2_ARG_CONV_EXCL="*" tools/bazel' 'bazel' ARGS) +_bazel *ARGS: (_if_in_semmle_code 'cd "$SEMMLE_CODE"; tools/bazel' 'bazel' ARGS) # Call sembuild (requires an internal repository checkout) [no-cd] diff --git a/misc/just/defs.just b/misc/just/defs.just index 47cedd124b44..16a9c18c0aa3 100644 --- a/misc/just/defs.just +++ b/misc/just/defs.just @@ -1,3 +1,8 @@ +# The first definition of a variable wins, not the last, so this order is what lets the +# internal file set `SEMMLE_CODE` while the stub is the fallback when it is absent. +# Swapped, the stub wins even where the real file exists and that checkout then behaves +# as if it were external. Nothing here can catch that: with the file absent, as it +# always is in this repository, both orders evaluate the same. import? '../../../semmle-code.just' # internal repo just file, if present import 'semmle-code-stub.just' @@ -12,8 +17,43 @@ set allow-duplicate-variables export PATH_SEP := if os() == "windows" { ";" } else { ":" } export JUST_EXECUTABLE := just_executable() +# MSYS2 rewrites arguments that look like paths as it hands them to a native program, +# which mangles bazel's `//target` labels. Set once here rather than at each bazel call: +# a call that forgets it works everywhere except Windows, which is where nobody looks. +# The price of setting it once is that it covers every command a recipe runs, so a tool +# that wants Windows path conversion has to ask for it back. +export MSYS2_ARG_CONV_EXCL := if os() == "windows" { "*" } else { "" } + error := f'{{ style("error") }}error{{ NORMAL }}: ' -cmd_sep := "\n#--------------------------------------------------------\n" + +# The separator line, sized to the terminal. +# +# `stty` rather than `tput`: it needs no terminfo, so it survives `TERM=dumb`, and reads +# the terminal from stdin, so it answers when stdout is piped. The arithmetic is in the +# shell because just has no integers. +# +# With no terminal the value is non-numeric and this falls back to a fixed 57 columns. +# That is a branch, not a platform test: just runs `sh` everywhere. +# +# `JUST_CMD_RULE` is the whole separator and lands in a shell script, so a hand-set value +# has to be a shell comment, every line of it, or it runs. Left unchecked: setting it at +# all means being able to run commands already, so there is nothing to defend. +# +# Exported and preferred over measuring: `shell()` runs on every parse, so a forwarded +# verb would otherwise re-measure in every child it spawns, and `if` is lazy in its +# branches. Inheritance needs a process, so a `mod` measures for itself; presetting +# `JUST_CMD_RULE` skips measuring entirely. +_given_horizontal_rule := env('JUST_CMD_RULE', '') + +_horizontal_rule := if _given_horizontal_rule == '' { shell(''' + w=$(stty size 2>/dev/null | cut -d" " -f2) + case "$w" in '' | *[!0-9]*) w=57 ;; esac + printf "%*s" $w "" | tr " " '#' +''') } else { _given_horizontal_rule } + +export JUST_CMD_RULE := _horizontal_rule + +cmd_sep := "\n" + _horizontal_rule + "\n" export CMD_BEGIN := style("command") + cmd_sep export CMD_END := cmd_sep + NORMAL export JUST_ERROR := error @@ -25,7 +65,7 @@ default_db_checks := ['--check-databases', '--check-diff-informed', '--fail-on-t [no-exit-message] @_require_semmle_code: {{ if SEMMLE_CODE == "" { f''' - echo "{error} running this recipe requires doing so from an internal repository checkout" >&2 + echo "{{ error }}running this recipe requires doing so from an internal repository checkout" >&2 exit 1 ''' } else { "" } }} diff --git a/misc/just/format.just b/misc/just/format.just index ba6885b590d8..8175a84bbb6d 100644 --- a/misc/just/format.just +++ b/misc/just/format.just @@ -1,83 +1,74 @@ import "build.just" -_ql_formatter := if SEMMLE_CODE != "" { '"$SEMMLE_CODE/target/intree/codeql-nolang/codeql"' } else { "codeql" } - -_py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" } - -_cpp_formatter := if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" } - -# The `buildifier` bazel target always covers the whole workspace, so the binary behind -# it is used instead and given paths. bazel is asked from the root of this repository, -# even when it sits inside another, and the files are bounded to that root as well: each -# repository then formats its own bazel files with the buildifier version it pins, and a -# verb aimed at a tree spanning both is answered once by each. This is the opposite of -# building, where a target needs the enclosing workspace to resolve at all, hence -# `_bazel` in build.just going the other way. -_bazel_workspace := quote(parent_directory(parent_directory(source_dir()))) - -_bazel_formatter := "bazel run --noshow_progress --ui_event_filters=,+error,+fail //misc/bazel/buildifier:binary --" - -# bazel files are named rather than suffixed, and buildifier has no exclude option of its -# own, so the files skipped by the target above are skipped here too. That target formats -# the whole workspace at once and so cannot take the path this recipe is given, which is -# why the two run the same binary through different entry points. Their exclusions are -# therefore stated twice, in two places, in two syntaxes: keep them in step, or `just -# format` rewrites what pre-commit and CI deliberately leave alone. -# -# bazel knows `BUILD` and `WORKSPACE` by those names and everything else by the `.bazel` -# extension, so a file named `BUILD.` else is a template or a generator's input -# rather than a bazel file, and is none of the formatter's business to parse. +# These formatters take files rather than directories, so `run_on_files.py` collects them +# and passes them positionally, which is what lets paths contain spaces. Both name every +# file they were given, so the lines saying nothing happened are dropped: for buildifier +# that is an empty warning tally, and a non-zero one is left alone, being the only notice +# of what it could not fix now that the exit code is 0 either way. # -# Both lists are comma-separated, so a root defining its own `_root_format` can name -# several patterns in one variable. It has no need to repeat the ones here: the files -# they cover belong to this repository, which formats them itself. Exclusions match the -# path as walked rather than as spelled on the command line, so one naming a directory -# has to cover both the path it is reached by and the path it is walked from. +# The collector announces the run, rather than each recipe saying what it is about to do. +# A recipe reaches that line knowing neither whether any file matched nor which of the +# paths it was handed hold one, and both are worth waiting for: a path whose files are all +# excluded is the ordinary case here, as a repository formatting its own bazel files is +# handed a nested repository's path too. The price of letting the collector speak is that +# it names the command as it is really spelled, flags that only shape output and all. It +# is a report rather than something to paste in any case, the collecting being the point; +# `just -n` prints what actually runs, `@` or no `@`. `_format_py` echoes its own, having +# no collector to defer to and nothing to decide. # -# As with the QL formatter, buildifier only names what it rewrote if it also accounts for -# every file it did not, so that accounting is dropped. It counts the warnings it could -# not fix there, which are left for linting to report rather than raised on every format. -_bazel_names := "BUILD,WORKSPACE,*.bazel,*.bzl,*.sky" - -_bazel_excluded := ".git/*,*/.git/*,*misc/bazel/3rdparty/*_deps/*" +# `[no-cd]` keeps a relative argument meaning the caller's directory: the forwarder passes +# `--justfile`, so without it the default `.` would silently become the whole repository. -_bazel_accounting := ': applied fixes, [0-9]+ warnings left$' - -# `codeql query format` and `clang-format` take files rather than directories, so the -# files are collected by `run_on_files.py`. Arguments are passed positionally so that -# paths containing spaces survive, of which this repository has many. -# -# The files that were rewritten are worth reporting, but `codeql query format` only -# names those once it also names every file it leaves alone, which buries them under -# thousands of lines. So it is asked for all of it and the lines about files it did not -# touch are dropped. Only those are dropped, so errors still come through, as does -# anything unforeseen. -# -# `[no-cd]` is what keeps a relative argument meaning the directory the caller is in. -# The forwarder reaches a recipe above its argument with `--justfile`, which otherwise -# runs it from that justfile's own directory: dropping the attribute would silently turn -# the default `.` into the whole repository, and the only symptom would be slowness. +_ql_formatter := (if SEMMLE_CODE != "" { '"$SEMMLE_CODE/target/intree/codeql-nolang/codeql"' } else { "codeql" }) + " query format --in-place" [no-cd] [no-exit-message] [positional-arguments] -_format_ql +ARGS: (_maybe_build_dist "nolang") - {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" --drop '^Formatting ' --drop '^No change for ' "*.ql,*.qll" {{ _ql_formatter }} query format --in-place -v -- "$@"{{ cmd_sep }} +@_format_ql +ARGS: (_maybe_build_dist "nolang") + {{ py }} "{{ source_dir() }}/run_on_files.py" --drop '^Formatting ' --drop '^No change for ' "*.ql,*.qll" {{ _ql_formatter }} -v -- "$@" + +_py_formatter := if SEMMLE_CODE != "" { "uv run black" } else { "black" } [no-cd] [no-exit-message] [positional-arguments] -_format_py *ARGS=".": - {{ cmd_sep }}{{ _py_formatter }} "$@"{{ cmd_sep }} +@_format_py *ARGS=".": + echo "$CMD_BEGIN-> "{{ quote(_py_formatter) }}" $*$CMD_END" >&2 + {{ _py_formatter }} "$@" + +_cpp_formatter := (if SEMMLE_CODE != "" { "uv run clang-format" } else { "clang-format" }) + " -i" [no-cd] [no-exit-message] [positional-arguments] -_format_cpp *ARGS=".": - {{ cmd_sep }}{{ py }} "{{ source_dir() }}/run_on_files.py" "*.h,*.cpp" {{ _cpp_formatter }} -i -- "$@"{{ cmd_sep }} +@_format_cpp *ARGS=".": + {{ py }} "{{ source_dir() }}/run_on_files.py" "*.h,*.cpp" {{ _cpp_formatter }} -- "$@" + +# The `buildifier` target formats a whole workspace and so cannot take a path, so its +# binary is driven directly. bazel is asked from this repository's root even when it sits +# inside another, and the files are bounded to it, so each repository formats its own with +# the version it pins. Building needs the enclosing workspace instead, hence `_bazel`. +_bazel_workspace := quote(parent_directory(parent_directory(source_dir()))) + +_bazel_formatter := (if SEMMLE_CODE != "" { '"$SEMMLE_CODE/tools/bazel"' } else { "bazel" }) + " run @buildifier_prebuilt//:buildifier" + +# Kept out of the formatter, unlike the other three: bazel's own flags have to sit between +# the target and `--`, so no single string spans both sides of it. +_bazel_args := "-mode=fix -lint=fix" + +# Keep both in step with the `buildifier` target. The names are what it recognises walking a +# workspace, a check it skips for paths handed to it, so anything extra here is rewritten +# regardless. Being wider is deliberate: what it recognises differs between versions, and +# each repository pins its own, so only a list covering every one of them can be shared. A +# directory of one file per candidate name, formatted with `-r`, says what a version takes. +# Exclusions match the path as walked and its absolute form, so one that has to hold +# however the path was reached is anchored absolutely. +_bazel_names := "BUILD,WORKSPACE,*.bazel,*.bzl,*.sky,*.star" + +_bazel_excluded := ".git/*,*/.git/*,*misc/bazel/3rdparty/*_deps/*" [no-cd] [no-exit-message] [positional-arguments] -_format_bazel *ARGS=".": - {{ cmd_sep }}export MSYS2_ARG_CONV_EXCL="*"; {{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --within {{ _bazel_workspace }} --drop '{{ _bazel_accounting }}' --exclude '{{ _bazel_excluded }}' "{{ _bazel_names }}" {{ _bazel_formatter }} -mode=fix -lint=fix -v -- "$@"{{ cmd_sep }} +@_format_bazel *ARGS=".": + {{ py }} "{{ source_dir() }}/run_on_files.py" --absolute --chdir {{ _bazel_workspace }} --within {{ _bazel_workspace }} --drop ': applied fixes, 0 warnings left$' --exclude '{{ _bazel_excluded }}' "{{ _bazel_names }}" {{ _bazel_formatter }} --noshow_progress --ui_event_filters=,+error,+fail -- {{ _bazel_args }} -v -- "$@" diff --git a/misc/just/lib.just b/misc/just/lib.just index 8ce335677287..6a254b4e99b8 100644 --- a/misc/just/lib.just +++ b/misc/just/lib.just @@ -27,5 +27,5 @@ import "format.just" [no-exit-message] [positional-arguments] @_integration_test *ARGS: _require_semmle_code - echo "$CMD_BEGIN$SEMMLE_CODE/tools/pytest --codeql=build-as-test $*$CMD_END" + echo "$CMD_BEGIN-> $SEMMLE_CODE/tools/pytest --codeql=build-as-test $*$CMD_END" >&2 "$SEMMLE_CODE/tools/pytest" --codeql=build-as-test "$@" diff --git a/misc/just/run_on_files.py b/misc/just/run_on_files.py index bed9c66b8b93..dd77fcb7db2d 100644 --- a/misc/just/run_on_files.py +++ b/misc/just/run_on_files.py @@ -7,7 +7,9 @@ The command is run once per batch of file names rather than once per file, and the batches are sized so that no single command line runs into a length limit. Nothing is -run at all when no file matches. +run at all when no file matches, and nothing is announced either, so silence means that +nothing here matched rather than that nothing was there: a path that does not exist is +refused instead, naming one being an assertion that it does. """ import argparse @@ -48,34 +50,68 @@ def files_under(paths, patterns, excludes=(), absolute=False, within=None): Patterns are matched against the file name, as bazel files are identified by name rather than by extension. Exclusions are matched against the whole path instead, - which is how a directory of generated files is left alone. That path is the one the - walk built, so an exclusion has to allow for how the paths it is given are spelled: - `*//*` does not match what is walked from `` itself. + which is how a directory of generated files is left alone. Both the path the walk + built and its resolved form are tried, as the walk only ever extends the path it + was given: walking `cpp` from inside a directory builds nothing naming that + directory, so an exclusion naming it could never match. An exclusion that has to + hold however the path was reached is therefore anchored absolutely, and resolving + is what makes that spelling hold for a path reached through a symbolic link too. A `within` directory bounds the result to the files below it, for a command that answers for one project and may be handed a path reaching outside it. Symbolic links are not followed, which is what keeps the `bazel-*` convenience links out of the walk. + + Returns the file names, and the given paths that yielded one. The two differ + whenever a path is excluded or simply holds nothing matching, and telling them + apart is what lets the banner name the paths being acted on rather than the paths + that were asked about. """ boundary = Path(within).resolve() if within else None def wanted(path): - if boundary is not None and not path.resolve().is_relative_to(boundary): + # The name decides most files and costs nothing, so it is asked first: resolving + # is a system call, and is only owed for a file that could still be collected. + if not any(fnmatch(path.name, p) for p in patterns): + return False + if boundary is None and not excludes: + return True + resolved = path.resolve() + if boundary is not None and not resolved.is_relative_to(boundary): return False - return any(fnmatch(path.name, p) for p in patterns) and not any( - fnmatch(str(path), e) for e in excludes + # A relative pattern is anchored at the start, so it cannot match the resolved + # spelling: trying both only ever gives a pattern the reach it was written with. + spellings = (str(path), str(resolved)) + return not any( + fnmatch(spelling, e) for e in excludes for spelling in spellings ) - found = set() - for path in map(Path, paths): + def collect(path): if path.is_file(): - if wanted(path): - found.add(path) - continue - for directory, _, names in os.walk(path): - found.update(p for p in map(Path(directory).joinpath, names) if wanted(p)) - return sorted(os.path.abspath(p) if absolute else str(p) for p in found) + return {path} if wanted(path) else set() + return { + file + for directory, _, names in os.walk(path) + for file in map(Path(directory).joinpath, names) + if wanted(file) + } + + # Kept per path rather than in one set, as which path a file came from is not a + # question the collected names can be asked afterwards without resolving each of + # them again. A file reached by two paths still only appears once below. + contributed = {} + for given in paths: + collected = collect(Path(given)) + if collected: + # Keyed by the spelling that was given rather than a normalised one, as + # this goes back to whoever wrote it and is theirs to recognise. + contributed[str(given)] = collected + files = sorted( + os.path.abspath(file) if absolute else str(file) + for file in set().union(*contributed.values()) + ) + return files, list(contributed) def batched(files, limit): @@ -160,6 +196,9 @@ def parse_args(): args.command, args.paths = args.rest[:separator], args.rest[separator + 1 :] if not args.command: parser.error("no command given") + missing = [path for path in args.paths if not os.path.exists(path)] + if missing: + parser.error("no such path: " + ", ".join(missing)) return args @@ -187,11 +226,35 @@ def run(command, drops, chdir=None): return process.wait() +def banner(command, paths): + """Announce a command over the paths it turned out to have something to do in. + + Only the paths that yielded a file are named: one whose files were all excluded is + not being acted on, and naming it claims work that is not about to happen. The file + names are left out, there being thousands of them and the paths being what was + asked for. + + So this is a report rather than something to paste, the collecting being the whole + point. `just -n` prints what really runs. + + `CMD_BEGIN` and `CMD_END` are the rules the justfiles put around a command; with + neither set this is a plain line. + """ + begin = os.environ.get("CMD_BEGIN", "") + end = os.environ.get("CMD_END", "") + return f"{begin}-> {' '.join(command)} -- {' '.join(paths)}{end}" + + def main(): args = parse_args() - files = files_under( + files, contributing = files_under( args.paths, args.patterns, args.exclude, args.absolute, args.within ) + if files: + # Neither half of this is known where the caller would have to say it: whether + # anything is going to run at all, and which of the paths it named hold any of + # it. + print(banner(args.command, contributing), file=sys.stderr, flush=True) limit = batch_limit() - sum(len(argument) + 1 for argument in args.command) status = 0 for batch in batched(files, limit): From cf3d79ff7fed3e91c3c3a89560af69e37d82f74f Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:48 +0200 Subject: [PATCH 11/15] Just: add a test suite for the tooling, and fix what it immediately caught Nothing under `misc/just` had tests, and most of the ways it can break exit zero: an exclusion that stops excluding, a verb that stops being found, an argument that arrives split in two. Adds a plain-`unittest` suite, each case paired with a positive control so a passing test means the rule held rather than that nothing ran, reachable through the usual `test` recipe and run in CI on any change here. Writing it immediately caught real bugs: `just --dump` maps an alias name to an object rather than to its target, so resolving one handed a dict to a dict lookup and raised; fixed by reading the target where the plain recipe is looked up, and doing so once rather than once per use, so an alias landing on a forwarder is recognised as that forwarder rather than reached under the wrong root recipe. The two shapes a consuming root can actually take -- inheriting a `_root_` or replacing it -- are pinned by a test, since neither had ever been exercised; the README's claim about which paired repository gets a shared root recipe for free is dropped along with it, since it is no longer true of the one repository it named and nothing here could have caught it going stale. The suite itself gets fixed along the way: fixtures are checked against a real `just --dump`, comparing only the fields the code reads so they stay valid as `just` gains more; it honours `JUST_EXECUTABLE` so it runs reliably under `bazel test`'s sandboxed `PATH`; a mis-scoped entry point had left two thirds of it never run at all; and a verb now fails outright when a candidate justfile cannot be parsed, instead of silently dropping it and still exiting zero. Also points the Python tooling workflow's own path filter at itself, rather than at a file that does not exist in this repository, so editing the tooling actually triggers its CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/python-tooling.yml | 7 +- misc/just/BUILD.bazel | 27 ++ misc/just/README.md | 28 +- misc/just/forward_command.py | 136 ++++--- misc/just/justfile | 11 + misc/just/language_tests.py | 9 +- misc/just/test_codeql_test_run.py | 119 ++++++ misc/just/test_forward_command.py | 553 +++++++++++++++++++++++++++ misc/just/test_language_tests.py | 133 +++++++ misc/just/test_run_on_files.py | 415 ++++++++++++++++++++ 10 files changed, 1379 insertions(+), 59 deletions(-) create mode 100644 misc/just/BUILD.bazel create mode 100644 misc/just/test_codeql_test_run.py create mode 100644 misc/just/test_forward_command.py create mode 100644 misc/just/test_language_tests.py create mode 100644 misc/just/test_run_on_files.py diff --git a/.github/workflows/python-tooling.yml b/.github/workflows/python-tooling.yml index a3ad9900ea47..51ca49533cc3 100644 --- a/.github/workflows/python-tooling.yml +++ b/.github/workflows/python-tooling.yml @@ -5,9 +5,10 @@ on: paths: - "misc/bazel/**" - "misc/codegen/**" + - "misc/just/**" - "misc/scripts/models-as-data/*.py" - "*.bazel*" - - .github/workflows/codegen.yml + - .github/workflows/python-tooling.yml - .pre-commit-config.yaml branches: - main @@ -33,3 +34,7 @@ jobs: shell: bash run: | bazel test //misc/codegen/... + - name: Run just tooling tests + shell: bash + run: | + bazel test //misc/just/... diff --git a/misc/just/BUILD.bazel b/misc/just/BUILD.bazel new file mode 100644 index 000000000000..f94804c9fcd0 --- /dev/null +++ b/misc/just/BUILD.bazel @@ -0,0 +1,27 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +py_library( + name = "tooling", + srcs = [ + "codeql_test_run.py", + "forward_command.py", + "language_tests.py", + "run_on_files.py", + ], + imports = ["."], + visibility = ["//visibility:public"], +) + +[ + py_test( + name = src[:-len(".py")], + size = "small", + srcs = [src], + deps = [":tooling"], + ) + for src in glob(["test_*.py"]) +] + +test_suite( + name = "test", +) diff --git a/misc/just/README.md b/misc/just/README.md index 8d62b276bcce..6a738ac79349 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -57,11 +57,15 @@ therefore reads its default `.` as the whole repository rather than the director caller is in, so one that does its own work needs `[no-cd]` itself. Being a recipe like any other, a `_root_` is inherited by a justfile importing the -one defining it, which is how the internal repository gets this one for free. It runs -once either way, as the two spellings are the same recipe. A root that defines its own -instead replaces it, and then both run, each over the files of the repository that -defines it: bazel formatting asks bazel from the root of the checkout the files belong -to, so that a repository formats its own files with its own pin. +one defining it. It runs once either way, as the two spellings are the same recipe. A +root that defines its own instead replaces it, and then both run, each over the files of +the repository that defines it: bazel formatting asks bazel from the root of the checkout +the files belong to, so that a repository formats its own files with its own pin. + +Nothing in a justfile says which of the two happened, so they are told apart by comparing +the recipes. A root whose own copy is identical to the one it would otherwise inherit, +down to the comment above it, is therefore taken for the inherited one and runs once. +Copy such a recipe to start from if it helps, but leave its comment behind. That last part is arranged by variables rather than by recipes. `set allow-duplicate-variables` in `defs.just` lets an importing justfile assign a variable @@ -186,3 +190,17 @@ With no terminal to ask — a pipe, a log, a shell without `stty` — it falls b fixed 57 columns, so logs and CI output are the same width every time. That is one branch rather than a platform test: `just` runs `sh` everywhere, so Windows takes whichever arm fits rather than a path of its own. + +# Tests + +The scripts here have tests, run by `just test misc/just`, by `bazel test +//misc/just/...`, or by CI on any change under this directory. They are plain +`unittest`: this is the layer that runs other tooling, so it should not need a package +manager to check itself. + +They exist for the class of bug that leaves no trace. An exclusion that stops excluding, +a verb that stops being found, an argument that arrives split in two — each of those +still exits zero, and the only symptom is work quietly not done. So the tests come in +pairs: one asserting the quiet outcome, and one positive control asserting the same +setup can produce the loud one. Without the second, a passing first test is also what a +test that runs nothing at all looks like. diff --git a/misc/just/forward_command.py b/misc/just/forward_command.py index e0dba8c59195..300e1a0a8263 100644 --- a/misc/just/forward_command.py +++ b/misc/just/forward_command.py @@ -96,22 +96,31 @@ def list_value(assignments, name): return [] +def is_variadic(recipe): + parameters = recipe["parameters"] + return bool(parameters and parameters[-1]["kind"] in ("star", "plus")) + + def accepts(recipe, argc): """Check whether a recipe can be called with a given number of arguments.""" parameters = recipe["parameters"] - variadic = parameters and parameters[-1]["kind"] in ("star", "plus") required = sum( 1 for parameter in parameters if parameter["default"] is None and parameter["kind"] != "star" ) - return required <= argc and (variadic or argc <= len(parameters)) + return required <= argc and (is_variadic(recipe) or argc <= len(parameters)) def implements(dump, command, argc): """Return the recipe a justfile runs for a command, if it has a usable one.""" recipes = dump["recipes"] - recipe = recipes.get(dump["aliases"].get(command, command)) + # An alias dumps as an object rather than as its target, so the name has to be read + # out of it. Resolved once and used throughout: a verb reached by an alias is the + # verb, so the justfile's own answer to it is named after the target too. + alias = dump["aliases"].get(command) + name = alias["target"] if alias else command + recipe = recipes.get(name) if recipe is None or recipe["private"]: return None if any( @@ -120,7 +129,7 @@ def implements(dump, command, argc): # Here the plain name is the forwarder's own, so it says nothing about what this # directory does. A justfile that both forwards and answers the command itself # spells its own answer `_root_`, the one name the two can share. - recipe = recipes.get(f"{ROOT_PREFIX}{command}") + recipe = recipes.get(f"{ROOT_PREFIX}{name}") if recipe is None: return None return recipe if accepts(recipe, argc) else None @@ -136,12 +145,14 @@ def dump_all(justfiles): with ThreadPoolExecutor(PROBE_WORKERS) as executor: dumps = list(executor.map(dump_justfile, justfiles)) parsed = [] + failed = False for justfile, (dump, failure) in zip(justfiles, dumps): if dump is None: + failed = True error(f"could not read {justfile}:\n{failure}") else: parsed.append((justfile, dump)) - return parsed + return parsed, failed def git(directory, *args): @@ -154,28 +165,26 @@ def git(directory, *args): ) if result.returncode != 0: error(f"`git {' '.join(args)}` failed in {directory}:\n{result.stderr.strip()}") - return [] - return result.stdout.splitlines() + return [], True + return result.stdout.splitlines(), False def submodules(directory): """List the initialised submodules under a directory.""" - toplevel = git(directory, "rev-parse", "--show-toplevel") + toplevel, failed = git(directory, "rev-parse", "--show-toplevel") if not toplevel or not (Path(toplevel[0]) / ".gitmodules").exists(): - return [] - paths = [ - Path(toplevel[0]) / line.split(" ", 1)[1] - for line in git( - toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$" - ) - ] + return [], failed + lines, config_failed = git( + toplevel[0], "config", "--file", ".gitmodules", "--get-regexp", r"\.path$" + ) + paths = [Path(toplevel[0]) / line.split(" ", 1)[1] for line in lines] within = Path(directory).resolve() return [ Path(directory) / os.path.relpath(path, within) for path in paths # An uninitialised submodule is an empty directory, with nothing to run. if path.is_relative_to(within) and (path / ".git").exists() - ] + ], failed or config_failed def find_justfiles(directory): @@ -186,21 +195,21 @@ def find_justfiles(directory): worth finding. """ justfiles = set() - for repository in [directory, *submodules(directory)]: - justfiles.update( - Path(repository) / line - for line in git( - repository, - "ls-files", - "--cached", - "--others", - "--exclude-standard", - "--", - "justfile", - "*/justfile", - ) + repositories, failed = submodules(directory) + for repository in [directory, *repositories]: + lines, git_failed = git( + repository, + "ls-files", + "--cached", + "--others", + "--exclude-standard", + "--", + "justfile", + "*/justfile", ) - return justfiles + failed = failed or git_failed + justfiles.update(Path(repository) / line for line in lines) + return justfiles, failed def invocation_path(path, *, like): @@ -225,7 +234,8 @@ def find_justfiles_above(command, arg): ] found = [] seen = [] - for justfile, dump in dump_all(candidates): + parsed, failed = dump_all(candidates) + for justfile, dump in parsed: # A justfile sitting exactly on the argument is called without it, as the # argument would only repeat where it already is. argc = 0 if justfile.parent.resolve() == directory else 1 @@ -236,13 +246,14 @@ def find_justfiles_above(command, arg): # Two repositories that each define a root recipe are not that case: the text # can match while the workspace, the tool it runs and the paths it excludes all # differ, so they have to stay apart. Nothing here says so. They are told apart - # only by the doc comment one of them happens to carry, which means dropping - # `doc` from this comparison silently discards an invocation unless a real - # discriminator arrives in the same change. + # only by whatever the two happened not to write identically, which today is a + # doc comment on one of them, so dropping a field from this comparison silently + # discards an invocation unless a real discriminator arrives in the same change. + # `TestFindJustfilesAbove` holds both shapes. if recipe is not None and recipe not in seen: seen.append(recipe) found.append((justfile, recipe)) - return found + return found, failed def find_justfiles_below(command, directory, covered=()): @@ -257,10 +268,13 @@ def find_justfiles_below(command, directory, covered=()): but ask to be named rather than found. """ # The justfile at `directory` is covered by the search above it. - candidates = sorted(find_justfiles(directory) - {Path(directory) / "justfile"}) + justfiles, failed = find_justfiles(directory) + candidates = sorted(justfiles - {Path(directory) / "justfile"}) matches = [] opted_out = [] - for justfile, dump in dump_all(candidates): + parsed, dump_failed = dump_all(candidates) + failed = failed or dump_failed + for justfile, dump in parsed: recipe = implements(dump, command, 0) if recipe is None: continue @@ -277,7 +291,7 @@ def find_justfiles_below(command, directory, covered=()): continue contributed.setdefault(justfile.parent, []).append(recipe) found.append((justfile, recipe)) - return sorted(found, key=lambda match: match[0]), sorted(opted_out) + return sorted(found, key=lambda match: match[0]), sorted(opted_out), failed def resolve(command, arg): @@ -288,18 +302,18 @@ def resolve(command, arg): on. One found below gets its own directory instead, as there the argument only said where to look. Justfiles below that asked to be named are returned separately. """ - above = find_justfiles_above(command, arg) - resolved = [(justfile, arg, recipe["name"]) for justfile, recipe in above] + above, failed = find_justfiles_above(command, arg) + resolved = [(justfile, arg, recipe) for justfile, recipe in above] opted_out = [] if os.path.isdir(arg): - below, opted_out = find_justfiles_below( + below, opted_out, below_failed = find_justfiles_below( command, arg, [recipe for _, recipe in above] ) + failed = failed or below_failed resolved += [ - (justfile, str(justfile.parent), recipe["name"]) - for justfile, recipe in below + (justfile, str(justfile.parent), recipe) for justfile, recipe in below ] - return resolved, opted_out + return resolved, opted_out, failed def report_opted_out(command, justfiles, *, ran): @@ -334,6 +348,19 @@ def invoke_just(cwd, args): return 0 +def invocation_argument_groups(recipe, pos_args): + """Split arguments into what the recipe can be called with at once. + + Every recipe reachable from a verb today is variadic or takes none, so this only + ever yields one group. It is here because the arguments are paths and running the + verb once per path is what a fixed-arity recipe would mean, where passing them + together would fail on arity alone and say nothing useful about why. + """ + if is_variadic(recipe) or len(pos_args) <= 1: + return [pos_args] + return [[arg] for arg in pos_args] + + def forward(cmd, args): """Forward a command to language-specific justfiles.""" is_non_positional = re.compile(r"^(-.*|\+|[A-Z_][A-Z_0-9]*=.*)$") @@ -342,16 +369,26 @@ def forward(cmd, args): justfiles = {} opted_out = [] + resolution_failed = False for arg in positional_args or ["."]: - resolved, skipped = resolve(cmd, arg) + resolved, skipped, failed = resolve(cmd, arg) opted_out += skipped + resolution_failed = resolution_failed or failed if not resolved: + # A candidate that could not be read is reported below rather than here: + # saying nothing matched would blame the argument for a broken justfile. + if failed: + continue error(f"No justfile found for {cmd} on {arg}") report_opted_out(cmd, skipped, ran=False) return 1 for justfile, justfile_arg, recipe in resolved: justfiles.setdefault(justfile, (recipe, []))[1].append(justfile_arg) + if resolution_failed: + report_opted_out(cmd, opted_out, ran=False) + return 1 + invocations = [] for justfile, (recipe, pos_args) in justfiles.items(): # An argument standing for the whole directory subsumes any more specific one @@ -359,10 +396,11 @@ def forward(cmd, args): whole_directory = str(justfile.parent) if whole_directory in pos_args: pos_args = [whole_directory] - cwd, just_args = get_just_context(justfile, recipe, flags, pos_args) - prefix = f"cd {cwd}; " if cwd else "" - print(f"-> {prefix}just {' '.join(just_args)}") - invocations.append((cwd, just_args)) + for group in invocation_argument_groups(recipe, pos_args): + cwd, just_args = get_just_context(justfile, recipe["name"], flags, group) + prefix = f"cd {cwd}; " if cwd else "" + print(f"-> {prefix}just {' '.join(just_args)}") + invocations.append((cwd, just_args)) report_opted_out(cmd, opted_out, ran=True) diff --git a/misc/just/justfile b/misc/just/justfile index 679295f47801..29f9659fe863 100644 --- a/misc/just/justfile +++ b/misc/just/justfile @@ -1,4 +1,15 @@ +import "defs.just" + [no-cd] [positional-arguments] format *ARGS=".": npx prettier --write "$@" + +# Test the tooling in this directory. +# Arguments are `unittest` ones, so a test file or a dotted test name rather than a +# directory. A path given stays valid as this does not change directory: `PYTHONPATH` is +# what lets the tests import the modules they are about, wherever they are run from. +[no-cd] +[positional-arguments] +@test *ARGS=['discover', '-s', source_dir()]: + PYTHONPATH="{{ source_dir() }}" {{ py }} -m unittest "$@" diff --git a/misc/just/language_tests.py b/misc/just/language_tests.py index 988a77d73cf3..efd5e3f244f0 100755 --- a/misc/just/language_tests.py +++ b/misc/just/language_tests.py @@ -15,7 +15,10 @@ def main(): - argv = sys.argv[1:] + # Blank arguments are dropped before the count is taken: one comes of a caller + # interpolating a variable that was never set, and a list of nothing but those is no + # arguments at all rather than a root to find a justfile above. + argv = [arg for arg in sys.argv[1:] if arg] if not argv: print("Usage: language_tests.py ROOT [ARG...]", file=sys.stderr) return 1 @@ -25,9 +28,7 @@ def main(): # the internal checkout, so relativize them there to keep command lines readable. # Anything else (flags, environment assignments, relative paths) is passed verbatim. args = [ - os.path.relpath(arg, semmle_code) if os.path.isabs(arg) else arg - for arg in argv - if arg + os.path.relpath(arg, semmle_code) if os.path.isabs(arg) else arg for arg in argv ] just = os.environ.get("JUST_EXECUTABLE", "just") diff --git a/misc/just/test_codeql_test_run.py b/misc/just/test_codeql_test_run.py new file mode 100644 index 000000000000..28e8cfffd8bc --- /dev/null +++ b/misc/just/test_codeql_test_run.py @@ -0,0 +1,119 @@ +#!/usr/bin/env python3 +"""Tests for `codeql_test_run.py`. + +Sorting arguments is the whole of what this file decides, and it decides it by looking +at each one: a word is a test, a `-` is a flag, `NAME=value` is an environment +assignment. Several of these pin down that an argument arrives whole, spaces and all, +which is what taking them as a list rather than re-splitting a string bought. +""" + +import os +import unittest +from unittest import mock + +import codeql_test_run + + +def empty_args(): + """What `main` builds before sorting, limited to what `parse_args` fills in.""" + return { + "tests": [], + "flags": [], + "env": [], + "all_checks": [], + "codeql": "host", + "all": False, + } + + +def sorted_args(*argv): + args = empty_args() + codeql_test_run.parse_args(args, list(argv)) + return args + + +class TestParseArgs(unittest.TestCase): + def test_a_plain_word_is_a_test(self): + self.assertEqual(sorted_args("ql/test/Foo")["tests"], ["ql/test/Foo"]) + + def test_a_dash_is_a_flag(self): + self.assertEqual(sorted_args("--fail-on-trap-errors")["flags"], ["--fail-on-trap-errors"]) + + def test_an_uppercase_assignment_is_an_environment_variable(self): + self.assertEqual(sorted_args("CPUS=4")["env"], ["CPUS=4"]) + + def test_a_lowercase_assignment_is_a_test(self): + # Only shouting counts, so a path that happens to contain `=` stays a path. + self.assertEqual(sorted_args("dir/a=b")["tests"], ["dir/a=b"]) + + def test_codeql_selects_the_executable(self): + self.assertEqual(sorted_args("--codeql=built")["codeql"], "built") + + def test_the_last_codeql_wins(self): + self.assertEqual(sorted_args("--codeql=host", "--codeql=built")["codeql"], "built") + + def test_all_checks_is_asked_for_by_either_spelling(self): + self.assertTrue(sorted_args("--all-checks")["all"]) + self.assertTrue(sorted_args("+")["all"]) + + def test_an_extra_check_is_held_back_until_it_is_asked_for(self): + held = sorted_args("--all-checks=--check-databases") + self.assertEqual(held["all_checks"], ["--check-databases"]) + # Held back means held back: it is not a flag until `--all-checks` arrives. + self.assertEqual(held["flags"], []) + self.assertFalse(held["all"]) + + def test_an_empty_argument_is_ignored(self): + # One of these comes of a caller interpolating a variable that was never set. + self.assertEqual(sorted_args("", "test")["tests"], ["test"]) + + def test_a_test_path_containing_a_space_stays_one_argument(self): + self.assertEqual(sorted_args("some dir/test")["tests"], ["some dir/test"]) + + def test_an_assignment_whose_value_contains_a_space_stays_whole(self): + """The value is the point, and a split one used to arrive as three characters. + + An argument list carries this; a whitespace-separated string cannot, as there + is nothing left in it to tell a separator from part of a value. + """ + self.assertEqual(sorted_args("EXTRA=a b")["env"], ["EXTRA=a b"]) + + def test_sorts_a_whole_command_line_at_once(self): + args = sorted_args("-j2", "CPUS=4", "ql/test", "+", "--all-checks=--check-diff") + self.assertEqual(args["flags"], ["-j2"]) + self.assertEqual(args["env"], ["CPUS=4"]) + self.assertEqual(args["tests"], ["ql/test"]) + self.assertEqual(args["all_checks"], ["--check-diff"]) + self.assertTrue(args["all"]) + + +class TestEnvValue(unittest.TestCase): + def test_prefers_a_test_argument(self): + args = sorted_args("CPUS=4") + with mock.patch.dict(os.environ, {"CPUS": "8"}): + self.assertEqual(codeql_test_run.env_value(args, "CPUS", "1"), "4") + + def test_falls_back_to_the_environment(self): + with mock.patch.dict(os.environ, {"CPUS": "8"}): + self.assertEqual(codeql_test_run.env_value(empty_args(), "CPUS", "1"), "8") + + def test_falls_back_to_the_default(self): + with mock.patch.dict(os.environ, {}, clear=True): + self.assertEqual(codeql_test_run.env_value(empty_args(), "CPUS", "1"), "1") + + def test_the_last_assignment_wins(self): + args = sorted_args("CPUS=4", "CPUS=2") + self.assertEqual(codeql_test_run.env_value(args, "CPUS", "1"), "2") + + def test_an_empty_value_does_not_count_as_a_setting(self): + args = sorted_args("CPUS=") + with mock.patch.dict(os.environ, {"CPUS": "8"}): + self.assertEqual(codeql_test_run.env_value(args, "CPUS", "1"), "8") + + def test_a_value_containing_a_space_survives(self): + args = sorted_args("EXTRA=a b") + self.assertEqual(codeql_test_run.env_value(args, "EXTRA", "none"), "a b") + + +if __name__ == "__main__": + unittest.main() diff --git a/misc/just/test_forward_command.py b/misc/just/test_forward_command.py new file mode 100644 index 000000000000..512423389773 --- /dev/null +++ b/misc/just/test_forward_command.py @@ -0,0 +1,553 @@ +#!/usr/bin/env python3 +"""Tests for `forward_command.py`. + +These cover the deciding rather than the running: which justfile answers a verb, with +how many arguments, and which ones ask to be passed over. All of that is read out of +`just --dump`, so the shapes below were taken from what `just` really emits rather than +imagined. That is checked rather than claimed: the last class here dumps a real justfile +and holds the fixtures against it, because a hand-written shape is otherwise only as +good as the day it was written, and agrees with itself long after `just` has moved on. +""" + +import json +import shutil +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +import forward_command + + +def parameter(name, kind="singular", default=None): + return {"name": name, "kind": kind, "default": default} + + +def recipe(name, parameters=(), dependencies=(), private=False, doc=None): + return { + "name": name, + "private": private, + "doc": doc, + "parameters": list(parameters), + "dependencies": [{"recipe": dependency} for dependency in dependencies], + } + + +def alias(name, target): + return {"attributes": [], "name": name, "target": target} + + +def dump(*recipes, aliases=None, assignments=None): + return { + "recipes": {recipe["name"]: recipe for recipe in recipes}, + "aliases": aliases or {}, + "assignments": assignments or {}, + } + + +def forwarding(command): + """The pair a justfile has when it both forwards a verb and answers it itself.""" + return dump( + recipe( + command, + [parameter("ARGS", kind="star")], + dependencies=[forward_command.FORWARD_RECIPE], + ), + recipe(f"{forward_command.ROOT_PREFIX}{command}", [parameter("ARGS", "star")]), + ) + + +class TestAccepts(unittest.TestCase): + def accepts(self, parameters, argc): + return forward_command.accepts(recipe("r", parameters), argc) + + def test_a_recipe_without_parameters_takes_none(self): + self.assertTrue(self.accepts([], 0)) + self.assertFalse(self.accepts([], 1)) + + def test_a_parameter_without_a_default_must_be_given(self): + self.assertFalse(self.accepts([parameter("X")], 0)) + self.assertTrue(self.accepts([parameter("X")], 1)) + self.assertFalse(self.accepts([parameter("X")], 2)) + + def test_a_defaulted_parameter_may_be_left_out(self): + defaulted = [parameter("X", default=".")] + self.assertTrue(self.accepts(defaulted, 0)) + self.assertTrue(self.accepts(defaulted, 1)) + self.assertFalse(self.accepts(defaulted, 2)) + + def test_a_star_parameter_takes_any_number(self): + star = [parameter("ARGS", kind="star")] + for argc in (0, 1, 7): + self.assertTrue(self.accepts(star, argc), argc) + + def test_a_plus_parameter_takes_at_least_one(self): + plus = [parameter("ARGS", kind="plus")] + self.assertFalse(self.accepts(plus, 0)) + self.assertTrue(self.accepts(plus, 1)) + self.assertTrue(self.accepts(plus, 7)) + + +class TestImplements(unittest.TestCase): + def test_finds_the_recipe_named_after_the_command(self): + found = forward_command.implements(dump(recipe("test")), "test", 0) + self.assertEqual(found["name"], "test") + + def test_follows_an_alias(self): + found = forward_command.implements( + dump(recipe("test"), aliases={"t": alias("t", "test")}), "t", 0 + ) + self.assertEqual(found["name"], "test") + + def test_follows_an_alias_into_a_forwarding_justfile(self): + # The recipe named after the alias forwards, so the answer is the root one -- + # and that is named after the verb, which is the target rather than the alias. + # Reaching here needs the alias to be a verb's own name, the only spelling the + # forwarder ever passes. + found = forward_command.implements( + dump( + recipe("build", dependencies=[forward_command.FORWARD_RECIPE]), + recipe(f"{forward_command.ROOT_PREFIX}build"), + aliases={"format": alias("format", "build")}, + ), + "format", + 0, + ) + self.assertEqual(found["name"], f"{forward_command.ROOT_PREFIX}build") + + def test_does_not_settle_on_a_root_recipe_named_after_the_alias(self): + # `_root_format` exists in most repository roots, so looking the alias up + # unresolved finds a real recipe rather than nothing: the wrong directory's + # answer, run in earnest. The version of this without one returns None, which + # is indistinguishable from a justfile that does not implement the verb at all. + found = forward_command.implements( + dump( + recipe("build", dependencies=[forward_command.FORWARD_RECIPE]), + recipe(f"{forward_command.ROOT_PREFIX}build"), + recipe(f"{forward_command.ROOT_PREFIX}format"), + aliases={"format": alias("format", "build")}, + ), + "format", + 0, + ) + self.assertEqual(found["name"], f"{forward_command.ROOT_PREFIX}build") + + def test_passes_over_a_private_recipe(self): + self.assertIsNone( + forward_command.implements(dump(recipe("test", private=True)), "test", 0) + ) + + def test_passes_over_a_recipe_that_cannot_take_the_arguments(self): + self.assertIsNone(forward_command.implements(dump(recipe("test")), "test", 1)) + + def test_takes_the_root_recipe_when_the_plain_name_forwards(self): + """A forwarder's own name says nothing about what its directory does. + + Settling on it would make the search find itself, so the one name the two can + share is `_root_`. + """ + found = forward_command.implements(forwarding("format"), "format", 1) + self.assertEqual(found["name"], "_root_format") + + def test_finds_nothing_when_a_forwarder_has_no_recipe_of_its_own(self): + forwarder = dump( + recipe( + "format", + [parameter("ARGS", kind="star")], + dependencies=[forward_command.FORWARD_RECIPE], + ) + ) + self.assertIsNone(forward_command.implements(forwarder, "format", 1)) + + +class TestListValue(unittest.TestCase): + def read(self, value): + return forward_command.list_value({"explicit_verbs": {"value": value}}, "explicit_verbs") + + def test_reads_a_list_literal(self): + self.assertEqual(self.read(["list", "test", "build"]), ["test", "build"]) + + def test_reads_an_empty_list_literal(self): + self.assertEqual(self.read(["list"]), []) + + def test_counts_an_expression_as_absent(self): + # What `['a'] ++ ['b']` dumps as. Evaluating it would mean running just. + concatenation = ["list-concatenate", ["list", "a"], ["list", "b"]] + self.assertEqual(self.read(concatenation), []) + + def test_counts_a_plain_string_as_absent(self): + self.assertEqual(self.read("test"), []) + + def test_counts_an_unset_name_as_absent(self): + self.assertEqual(forward_command.list_value({}, "explicit_verbs"), []) + + +class TestOptsOut(unittest.TestCase): + def test_a_listed_verb_asks_to_be_named(self): + listed = dump(assignments={"explicit_verbs": {"value": ["list", "test"]}}) + self.assertTrue(forward_command.opts_out(listed, "test")) + self.assertFalse(forward_command.opts_out(listed, "build")) + + def test_listing_nothing_opts_out_of_nothing(self): + self.assertFalse(forward_command.opts_out(dump(), "test")) + + +class TestGetJustContext(unittest.TestCase): + def test_a_justfile_sitting_on_the_argument_is_run_from_there(self): + # `just build ql/rust` becomes `just build` inside `ql/rust`, as repeating the + # directory it is already in says nothing. + cwd, args = forward_command.get_just_context( + Path("ql/rust/justfile"), "build", [], ["ql/rust"] + ) + self.assertEqual(cwd, "ql/rust") + self.assertEqual(args, ["build"]) + + def test_flags_survive_being_run_from_there(self): + cwd, args = forward_command.get_just_context( + Path("ql/rust/justfile"), "test", ["--all-checks"], ["ql/rust"] + ) + self.assertEqual(cwd, "ql/rust") + self.assertEqual(args, ["test", "--all-checks"]) + + def test_anything_else_names_the_justfile_and_keeps_the_arguments(self): + cwd, args = forward_command.get_just_context( + Path("ql/justfile"), "build", ["-x"], ["ql/rust"] + ) + self.assertIsNone(cwd) + self.assertEqual( + args, ["--justfile", str(Path("ql/justfile")), "build", "-x", "ql/rust"] + ) + + def test_two_arguments_are_both_kept(self): + cwd, args = forward_command.get_just_context( + Path("ql/justfile"), "format", [], ["ql/rust", "ql/cpp"] + ) + self.assertIsNone(cwd) + self.assertEqual(args[-2:], ["ql/rust", "ql/cpp"]) + + +class TestInvocationPath(unittest.TestCase): + def test_spells_a_path_relatively_when_the_argument_was(self): + found = Path.cwd() / "sub" / "justfile" + self.assertEqual( + forward_command.invocation_path(found, like="sub"), Path("sub/justfile") + ) + + def test_leaves_a_path_absolute_when_the_argument_was(self): + found = Path.cwd() / "sub" / "justfile" + self.assertEqual( + forward_command.invocation_path(found, like=str(Path.cwd())), found + ) + + +class TestFindJustfilesAbove(unittest.TestCase): + """The two shapes a consuming root can have, both of which the README promises. + + A root that imports the justfile defining a `_root_` inherits it, and the verb + is then reached twice under two spellings of one recipe, which has to run once. A + root that defines its own replaces it, and both have to run, each over the files of + the repository defining it. No dump says which of the two happened, so they are told + apart by comparing the recipes themselves, and the fields that carry the difference + are whatever the two repositories happened not to write identically. + + That makes a verbatim copy of one root recipe into another indistinguishable from + inheritance, comment included, and a comment is the part of a recipe most likely to + survive the paste that produces this. + """ + + def setUp(self): + directory = tempfile.TemporaryDirectory() + self.addCleanup(directory.cleanup) + root = Path(directory.name).resolve() + self.outer = root / "justfile" + self.inner = root / "inner" / "justfile" + self.argument = root / "inner" / "below" + self.argument.mkdir(parents=True) + for justfile in (self.outer, self.inner): + justfile.write_text("") + + def implementing(self, doc=None, dependencies=()): + """A justfile that forwards `format` and answers it itself.""" + return dump( + recipe( + "format", + [parameter("ARGS", "star")], + dependencies=[forward_command.FORWARD_RECIPE], + ), + recipe( + f"{forward_command.ROOT_PREFIX}format", + [parameter("ARGS", "star")], + dependencies=dependencies, + doc=doc, + ), + ) + + def found(self, outer, inner): + dumps = {self.outer: outer, self.inner: inner} + with mock.patch.object( + forward_command, + "dump_justfile", + side_effect=lambda justfile: (dumps[Path(justfile)], None), + ): + found, failed = forward_command.find_justfiles_above( + "format", str(self.argument) + ) + self.assertFalse(failed) + return found + + def test_a_recipe_reached_under_two_spellings_runs_once(self): + found = self.found(self.implementing(), self.implementing()) + self.assertEqual([justfile for justfile, _ in found], [self.inner]) + + def test_two_roots_differing_only_in_their_comment_both_run(self): + found = self.found(self.implementing(), self.implementing(doc="From here.")) + self.assertEqual([justfile for justfile, _ in found], [self.inner, self.outer]) + + def test_two_roots_sharing_a_comment_still_both_run_if_they_do_different_work(self): + found = self.found( + self.implementing(doc="From here.", dependencies=["_format_other"]), + self.implementing(doc="From here."), + ) + self.assertEqual([justfile for justfile, _ in found], [self.inner, self.outer]) + + +class TestDiscoveryFailures(unittest.TestCase): + def test_dump_all_reports_every_failure(self): + justfiles = [Path("one/justfile"), Path("two/justfile"), Path("three/justfile")] + dumps = [(None, "bad one"), (dump(), None), (None, "bad three")] + with mock.patch.object( + forward_command, "dump_justfile", side_effect=dumps + ), mock.patch.object(forward_command, "error") as report: + parsed, failed = forward_command.dump_all(justfiles) + self.assertTrue(failed) + self.assertEqual(parsed, [(Path("two/justfile"), dump())]) + self.assertEqual(report.call_count, 2) + + def test_git_failure_is_reported_to_callers(self): + result = subprocess.CompletedProcess( + ["git"], 128, stdout="", stderr="fatal: not a repository" + ) + with mock.patch.object( + subprocess, "run", return_value=result + ), mock.patch.object(forward_command, "error"): + lines, failed = forward_command.git(".", "ls-files") + self.assertEqual(lines, []) + self.assertTrue(failed) + + def test_forward_fails_without_running_after_resolution_failure(self): + justfile = Path("pkg/justfile") + resolved = [(justfile, "pkg", recipe("test", [parameter("ARGS", "star")]))] + with mock.patch.object( + forward_command, "resolve", return_value=(resolved, [], True) + ), mock.patch.object(forward_command, "invoke_just") as invoke: + self.assertEqual(forward_command.forward("test", ["pkg"]), 1) + invoke.assert_not_called() + + +class TestForwardInvocations(unittest.TestCase): + def test_accumulates_variadic_recipe_arguments(self): + justfile = Path("pkg/justfile") + test_recipe = recipe("test", [parameter("ARGS", "star")]) + + def resolve(command, arg): + return [(justfile, arg, test_recipe)], [], False + + with mock.patch.object( + forward_command, "resolve", side_effect=resolve + ), mock.patch.object(forward_command, "invoke_just", return_value=0) as invoke: + self.assertEqual(forward_command.forward("test", ["pkg/a", "pkg/b"]), 0) + + invoke.assert_called_once_with( + None, ["--justfile", "pkg/justfile", "test", "pkg/a", "pkg/b"] + ) + + def test_splits_non_variadic_recipe_arguments(self): + justfile = Path("pkg/justfile") + test_recipe = recipe("test", [parameter("ARG")]) + + def resolve(command, arg): + return [(justfile, arg, test_recipe)], [], False + + with mock.patch.object( + forward_command, "resolve", side_effect=resolve + ), mock.patch.object(forward_command, "invoke_just", return_value=0) as invoke: + self.assertEqual(forward_command.forward("test", ["pkg/a", "pkg/b"]), 0) + + self.assertEqual( + invoke.call_args_list, + [ + mock.call(None, ["--justfile", "pkg/justfile", "test", "pkg/a"]), + mock.call(None, ["--justfile", "pkg/justfile", "test", "pkg/b"]), + ], + ) + + +# Resolve the same binary `forward_command` will run: it honours JUST_EXECUTABLE, so a +# pinned `just` would otherwise have these fixtures checked against a different binary +# than the code uses. `which` covers both spellings, returning an explicit path as given +# and looking a bare name up on PATH. +JUST = shutil.which(forward_command.JUST) + +# The justfile below uses every construct the fixtures above model, so that a dump of it +# can be checked against them. +CONSTRUCTS = """ +set unstable +set lists + +alias t := test + +explicit_verbs := ['test'] + +test *ARGS='.': _helper + echo {{ ARGS }} + +# A comment above a recipe becomes its doc. +build X Y='y': + echo {{ X }} {{ Y }} + +lint +ARGS: + echo {{ ARGS }} + +[private] +_helper: + echo helper +""" + + +@unittest.skipUnless(JUST, "needs `just` on PATH") +class TestFixturesStillMatchJust(unittest.TestCase): + """Check the fixtures above against what `just` really dumps. + + Everything else here reads a shape written by hand, which is only as good as the + day it was written: `just` changed how it dumps an alias once already, and the test + covering aliases went on passing against the shape that had gone away. A fixture + cannot notice that on its own, so this asks the real thing. + + Only the fields the code reads are compared. `just` is free to dump more, and a + test that failed whenever it did would be noise rather than a warning. + """ + + @classmethod + def setUpClass(cls): + with tempfile.TemporaryDirectory() as directory: + justfile = Path(directory) / "justfile" + justfile.write_text(CONSTRUCTS) + dumped = subprocess.run( + [JUST, "--justfile", str(justfile), "--dump", "--dump-format", "json"], + capture_output=True, + text=True, + check=True, + ) + cls.dump = json.loads(dumped.stdout) + + def test_a_dump_is_read_by_the_keys_the_fixtures_use(self): + self.assertLessEqual(set(dump()), set(self.dump)) + + def test_an_alias_names_its_target(self): + real = self.dump["aliases"]["t"] + self.assertEqual(set(alias("t", "test")), set(real)) + self.assertEqual(real["target"], "test") + + def test_a_recipe_is_read_by_the_keys_the_fixtures_use(self): + self.assertLessEqual(set(recipe("test")), set(self.dump["recipes"]["test"])) + + def test_a_comment_above_a_recipe_is_the_doc_the_comparison_reads(self): + # Two root recipes doing different jobs are often told apart by this alone. + recipes = self.dump["recipes"] + self.assertEqual( + recipes["build"]["doc"], "A comment above a recipe becomes its doc." + ) + self.assertIsNone(recipes["test"]["doc"]) + + def test_a_private_recipe_says_so(self): + self.assertIs(self.dump["recipes"]["_helper"]["private"], True) + self.assertIs(self.dump["recipes"]["test"]["private"], False) + + def test_a_dependency_names_its_recipe(self): + dependencies = self.dump["recipes"]["test"]["dependencies"] + self.assertEqual([d["recipe"] for d in dependencies], ["_helper"]) + + def test_parameters_keep_the_kinds_and_defaults_accepts_reads(self): + kinds = { + name: [(p["kind"], p["default"]) for p in recipe["parameters"]] + for name, recipe in self.dump["recipes"].items() + } + self.assertEqual(kinds["test"], [("star", ".")]) + self.assertEqual(kinds["build"], [("singular", None), ("singular", "y")]) + self.assertEqual(kinds["lint"], [("plus", None)]) + self.assertEqual(kinds["_helper"], []) + + def test_a_list_assignment_is_the_shape_list_value_unwraps(self): + self.assertEqual( + forward_command.list_value(self.dump["assignments"], "explicit_verbs"), + ["test"], + ) + + +@unittest.skipUnless(JUST, "needs `just` on PATH") +class TestImportingDoesNotChangeARecipe(unittest.TestCase): + """Ask `just` for the premise the deduplication rests on rather than assuming it. + + A root importing the justfile that defines a `_root_` reaches one recipe under + two spellings, and it is told apart from two repositories each defining their own by + comparing the recipes whole. That comparison reads every field `just` emits, not the + few the rest of this file models, so a release adding a per-recipe field that varies + between justfiles -- a source path, a line number, anything saying where a recipe was + written -- would stop the two spellings comparing equal and run an inherited recipe + twice. + + Such a field is precisely the discriminator this code would otherwise want, so it + would arrive looking like a feature. `namepath`, the closest thing to one today, is + a module path rather than a file one, and so reads identically for two top-level + recipes. The fixtures above cannot see any of this: they are compared as subsets, + which is right for reading a field by name and blind to a field nobody thought to + model. Nor would a repository notice, since defining a recipe of one's own only + moves it further from the imported one: the shape that would start running twice is + the one built here and kept nowhere else. + """ + + def recipes(self, justfile): + dumped = subprocess.run( + [JUST, "--justfile", str(justfile), "--dump", "--dump-format", "json"], + capture_output=True, + text=True, + check=True, + ) + return json.loads(dumped.stdout)["recipes"] + + def setUp(self): + directory = tempfile.TemporaryDirectory() + self.addCleanup(directory.cleanup) + root = Path(directory.name) + (root / "inner").mkdir() + self.inner = root / "inner" / "justfile" + self.inner.write_text("# Shared.\n_root_format:\n echo shared\n") + self.outer = root / "justfile" + + def test_an_imported_recipe_is_the_one_it_came_from(self): + self.outer.write_text("import 'inner/justfile'\n") + self.assertEqual( + self.recipes(self.outer)["_root_format"], + self.recipes(self.inner)["_root_format"], + ) + + def test_a_root_defining_its_own_is_not(self): + # Same body, so only the comment separates them: the narrowest the difference + # between the two shapes ever gets. + self.outer.write_text( + "set allow-duplicate-recipes\n" + "import 'inner/justfile'\n" + "\n" + "# Mine.\n" + "_root_format:\n" + " echo shared\n" + ) + self.assertNotEqual( + self.recipes(self.outer)["_root_format"], + self.recipes(self.inner)["_root_format"], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/misc/just/test_language_tests.py b/misc/just/test_language_tests.py new file mode 100644 index 000000000000..d1865599f9f4 --- /dev/null +++ b/misc/just/test_language_tests.py @@ -0,0 +1,133 @@ +#!/usr/bin/env python3 +"""Tests for `language_tests.py`. + +`just` is stubbed out here: what this file decides is the invocation, and running it +needs a built CLI and a whole test suite. The invocation is also where the interesting +property lives, as an argument has to reach the suite exactly as it was written. +""" + +import contextlib +import io +import os +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +import language_tests + + +class TestMain(unittest.TestCase): + def setUp(self): + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + # Resolved once: macOS puts temporary directories behind a symbolic link, and + # an unresolved root would not be a prefix of the paths built from it. + self.semmle_code = Path(temporary.name).resolve() + self.suite = self.semmle_code / "ql" / "rust" / "ql" / "test" + self.suite.mkdir(parents=True) + (self.semmle_code / "ql" / "rust" / "justfile").touch() + + def run_main(self, *argv, environment=None, side_effect=None): + """Run `main` with `just` stubbed out, returning its status and that stub.""" + env = {"SEMMLE_CODE": str(self.semmle_code), "JUST_EXECUTABLE": "just"} + env.update(environment or {}) + printed = io.StringIO() + with ( + mock.patch.object( + language_tests.sys, "argv", ["language_tests.py", *argv] + ), + mock.patch.dict(os.environ, env), + mock.patch.object( + language_tests.subprocess, "run", side_effect=side_effect + ) as run, + contextlib.redirect_stdout(printed), + # The messages it writes here are expected by the tests below, and reading + # them among the results would suggest something had gone wrong. + contextlib.redirect_stderr(io.StringIO()), + ): + status = language_tests.main() + return status, run, printed.getvalue() + + def invocation(self, *argv, **kwargs): + status, run, _ = self.run_main(*argv, **kwargs) + self.assertEqual(status, 0) + return list(run.call_args.args[0]) + + def test_relativizes_an_absolute_root_against_the_checkout(self): + # Roots are absolute because a justfile builds them from `source_dir()`, and + # the command line is read by people. + self.assertEqual( + self.invocation(str(self.suite))[-1], + os.path.join("ql", "rust", "ql", "test"), + ) + + def test_finds_the_nearest_justfile_above_the_root(self): + invocation = self.invocation(str(self.suite)) + self.assertEqual( + invocation[invocation.index("--justfile") + 1], + str(Path("ql/rust/justfile")), + ) + + def test_asks_for_the_checks_ci_wants(self): + invocation = self.invocation(str(self.suite)) + self.assertIn("--all-checks", invocation) + self.assertIn("--codeql=built", invocation) + + def test_runs_from_the_checkout(self): + _, run, _ = self.run_main(str(self.suite)) + self.assertEqual(run.call_args.kwargs["cwd"], self.semmle_code) + + def test_an_argument_containing_a_space_stays_one_argument(self): + """The reason these arrive as a list rather than one string to re-split. + + Splitting on whitespace made this reach the suite as two arguments, and a value + that was only whitespace reached it as its own separators. + """ + self.assertIn("EXTRA=a b", self.invocation(str(self.suite), "EXTRA=a b")) + + def test_a_relative_argument_is_passed_verbatim(self): + self.assertIn("--fail-fast", self.invocation(str(self.suite), "--fail-fast")) + + def test_keeps_the_arguments_in_the_order_they_were_given(self): + invocation = self.invocation(str(self.suite), "CPUS=2", "--verbose") + self.assertEqual(invocation[-3:], [os.path.join("ql", "rust", "ql", "test"), "CPUS=2", "--verbose"]) + + def test_uses_the_just_it_was_given(self): + invocation = self.invocation( + str(self.suite), environment={"JUST_EXECUTABLE": "/opt/just"} + ) + self.assertEqual(invocation[0], "/opt/just") + + def test_says_what_it_is_about_to_run(self): + _, _, printed = self.run_main(str(self.suite)) + self.assertIn("-> just", printed) + + def test_needs_a_root(self): + status, run, _ = self.run_main() + self.assertEqual(status, 1) + run.assert_not_called() + + def test_nothing_but_blank_arguments_is_no_arguments(self): + # An unset variable interpolated by a caller arrives as one of these. Counting + # it as an argument and then dropping it left nothing to take a root from. + status, run, _ = self.run_main("", "") + self.assertEqual(status, 1) + run.assert_not_called() + + def test_reports_a_root_with_no_justfile_above_it(self): + orphan = self.semmle_code / "elsewhere" + orphan.mkdir() + status, run, _ = self.run_main(str(orphan)) + self.assertEqual(status, 1) + run.assert_not_called() + + def test_passes_on_the_status_of_a_failing_suite(self): + failure = subprocess.CalledProcessError(3, "just") + status, _, _ = self.run_main(str(self.suite), side_effect=failure) + self.assertEqual(status, 3) + + +if __name__ == "__main__": + unittest.main() diff --git a/misc/just/test_run_on_files.py b/misc/just/test_run_on_files.py new file mode 100644 index 000000000000..ce2517547c06 --- /dev/null +++ b/misc/just/test_run_on_files.py @@ -0,0 +1,415 @@ +#!/usr/bin/env python3 +"""Tests for `run_on_files.py`. + +Two of these guard properties that cost nothing to break and say nothing when broken. +An exclusion that stops excluding does not fail: it formats files it was told to leave +alone, with a tool the other repository did not choose, and the only way to notice is +to go looking. These are that noticing, done once and kept. + +Each of those two carries a positive control, as the assertion they make is that a +collection is empty, and an empty collection is also what a mistyped pattern, a wrong +directory or a walk that never ran produce. +""" + +import contextlib +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +import run_on_files + +HERE = Path(__file__).resolve().parent + +# Print each argument on its own line, so that a test can tell one argument containing a +# space from two arguments. +ECHO_ARGUMENTS = "import sys\nfor a in sys.argv[1:]: print(a)" +ECHO_TO_STDERR = "import sys\nfor a in sys.argv[1:]: print(a, file=sys.stderr)" + + +def can_symlink(): + """Whether this process may create symbolic links, which Windows restricts.""" + with tempfile.TemporaryDirectory() as directory: + try: + os.symlink(directory, Path(directory) / "link") + return True + except (OSError, NotImplementedError): + return False + + +CAN_SYMLINK = can_symlink() + + +@contextlib.contextmanager +def working_directory(directory): + previous = os.getcwd() + os.chdir(directory) + try: + yield + finally: + os.chdir(previous) + + +class TemporaryTree(unittest.TestCase): + """A scratch tree of empty files, named by `files`.""" + + files = () + + def setUp(self): + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + # Resolved once here: macOS puts temporary directories behind a symbolic link, + # and these tests compare against absolute paths. + self.root = Path(temporary.name).resolve() + for name in self.files: + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.touch() + + +class TestFilesUnder(TemporaryTree): + files = ( + "outer/BUILD", + "outer/BUILD.bazel", + "outer/notes.md", + "outer/nested/BUILD.bazel", + "outer/nested/deep/BUILD.bazel", + ) + + def collect(self, paths, patterns, *args, **kwargs): + files, _ = run_on_files.files_under(paths, patterns, *args, **kwargs) + return files + + def test_matches_the_whole_name_rather_than_an_extension(self): + # A bazel file may be called `BUILD`, with no extension to match on. + with working_directory(self.root / "outer"): + self.assertEqual(self.collect(["."], ["BUILD"]), ["BUILD"]) + + def test_leaves_out_a_file_no_pattern_names(self): + with working_directory(self.root): + found = self.collect(["outer"], ["*.bazel"]) + self.assertNotIn(os.path.join("outer", "notes.md"), found) + + def test_walks_the_whole_tree_below_a_directory(self): + with working_directory(self.root): + found = self.collect(["outer"], ["*.bazel"]) + self.assertEqual( + found, + [ + os.path.join("outer", "BUILD.bazel"), + os.path.join("outer", "nested", "BUILD.bazel"), + os.path.join("outer", "nested", "deep", "BUILD.bazel"), + ], + ) + + def test_takes_a_file_as_well_as_a_directory(self): + with working_directory(self.root): + path = os.path.join("outer", "BUILD.bazel") + self.assertEqual(self.collect([path], ["*.bazel"]), [path]) + + def test_leaves_out_a_file_named_directly_that_no_pattern_matches(self): + with working_directory(self.root): + self.assertEqual(self.collect([os.path.join("outer", "notes.md")], ["*.bazel"]), []) + + def test_excludes_are_matched_against_the_path_not_the_name(self): + with working_directory(self.root): + found = self.collect(["outer"], ["*.bazel"], ["*/nested/*"]) + self.assertEqual(found, [os.path.join("outer", "BUILD.bazel")]) + + def test_absolute_exclusion_holds_for_a_path_walked_from_inside(self): + """The walk only ever extends the path it was given. + + Walking `nested` from inside `outer` builds no path naming `outer`, so an + exclusion spelled relative to the enclosing directory cannot match it. One + anchored absolutely has to, which is what a repository relies on to stay out of + a nested one that is being formatted from within. + """ + with working_directory(self.root / "outer"): + # Positive control: there is something here to exclude, so an empty result + # below means the exclusion worked rather than that the walk found nothing. + self.assertEqual(len(self.collect(["nested"], ["*.bazel"])), 2) + self.assertEqual( + self.collect(["nested"], ["*.bazel"], [f"{self.root / 'outer'}/*"]), + [], + ) + + @unittest.skipUnless(CAN_SYMLINK, "symbolic links need a privilege on Windows") + def test_absolute_exclusion_holds_for_a_path_reached_through_a_link(self): + """An absolute path may still be spelled through a symbolic link. + + Joining it with the working directory leaves that spelling alone, so only + resolving it makes an absolutely anchored exclusion hold here too. + """ + link = self.root / "link" + link.symlink_to(self.root / "outer", target_is_directory=True) + reached_through_link = str(link / "nested") + # Positive control, as above. + self.assertEqual(len(self.collect([reached_through_link], ["*.bazel"])), 2) + self.assertEqual( + self.collect([reached_through_link], ["*.bazel"], [f"{self.root / 'outer'}/*"]), + [], + ) + + def test_a_relative_exclusion_still_holds_for_a_relative_walk(self): + # Trying both spellings must not cost a pattern the reach it was written with. + with working_directory(self.root): + self.assertEqual(self.collect(["outer"], ["*.bazel"], ["outer/*"]), []) + + def test_within_leaves_out_what_lies_beyond_it(self): + with working_directory(self.root): + self.assertEqual( + self.collect(["outer"], ["*.bazel"], within=str(self.root / "outer" / "nested")), + [ + os.path.join("outer", "nested", "BUILD.bazel"), + os.path.join("outer", "nested", "deep", "BUILD.bazel"), + ], + ) + + @unittest.skipUnless(CAN_SYMLINK, "symbolic links need a privilege on Windows") + def test_does_not_walk_through_a_symbolic_link(self): + # This is what keeps the `bazel-*` convenience links out of a walk, which would + # otherwise reach the whole output tree. + (self.root / "outer" / "bazel-out").symlink_to( + self.root / "outer" / "nested", target_is_directory=True + ) + with working_directory(self.root): + found = self.collect(["outer"], ["*.bazel"]) + self.assertFalse([path for path in found if "bazel-out" in path]) + + def test_absolute_asks_for_absolute_names(self): + with working_directory(self.root): + found = self.collect(["outer"], ["BUILD"], absolute=True) + self.assertEqual(found, [str(self.root / "outer" / "BUILD")]) + + def test_a_file_is_collected_once_however_many_paths_reach_it(self): + with working_directory(self.root): + found = self.collect(["outer", os.path.join("outer", "BUILD.bazel")], ["*.bazel"]) + self.assertEqual(len(found), len(set(found))) + + +class TestContributingPaths(TemporaryTree): + """The second half of what a walk learns, and the reason the banner is honest. + + A path that was asked about is not the same as a path being acted on. Naming the + first is what made a run over a repository and a nested one report the nested path + twice, once from an invocation that had excluded every file in it. + """ + + files = ( + "outer/BUILD.bazel", + "outer/nested/BUILD.bazel", + "outer/barren/notes.md", + ) + + def contributing(self, paths, patterns, *args, **kwargs): + _, contributing = run_on_files.files_under(paths, patterns, *args, **kwargs) + return contributing + + def test_names_only_a_path_that_yielded_a_file(self): + with working_directory(self.root): + self.assertEqual( + self.contributing( + [os.path.join("outer", "nested"), os.path.join("outer", "barren")], + ["*.bazel"], + ), + [os.path.join("outer", "nested")], + ) + + def test_leaves_out_a_path_whose_files_were_all_excluded(self): + with working_directory(self.root): + self.assertEqual( + self.contributing( + ["outer"], + ["*.bazel"], + [f"{self.root / 'outer'}/*"], + ), + [], + ) + + def test_keeps_the_spelling_the_path_was_given_in(self): + # It goes back to whoever wrote it, so it has to be recognisable as theirs. + with working_directory(self.root): + self.assertEqual(self.contributing(["./outer"], ["*.bazel"]), ["./outer"]) + + def test_names_both_paths_that_reach_the_same_file(self): + # The file is collected once, but each path did have something in it. + with working_directory(self.root): + paths = ["outer", os.path.join("outer", "nested")] + files, contributing = run_on_files.files_under(paths, ["*.bazel"]) + self.assertEqual(contributing, paths) + self.assertEqual(len(files), 2) + + +class TestBanner(unittest.TestCase): + def test_names_the_command_and_the_paths_it_has_something_to_do_in(self): + # The bare form has to be asked for rather than assumed: `just` exports these + # two, so run through the `test` recipe the ambient environment is not empty, + # and a test that reads it would pass or fail by how it was started. + with mock.patch.dict(os.environ): + os.environ.pop("CMD_BEGIN", None) + os.environ.pop("CMD_END", None) + self.assertEqual( + run_on_files.banner(["clang-format", "-i"], ["cpp", "swift"]), + "-> clang-format -i -- cpp swift", + ) + + def test_is_wrapped_in_the_rules_the_justfiles_set(self): + with mock.patch.dict( + os.environ, {"CMD_BEGIN": "", "CMD_END": ""} + ): + self.assertEqual( + run_on_files.banner(["black"], ["."]), "-> black -- ." + ) + + +class TestBatched(unittest.TestCase): + def test_keeps_everything_in_one_batch_when_it_fits(self): + self.assertEqual(list(run_on_files.batched(["a", "b"], 100)), [["a", "b"]]) + + def test_splits_once_the_limit_is_reached(self): + batches = list(run_on_files.batched(["aaa", "bbb", "ccc"], 8)) + self.assertEqual(batches, [["aaa", "bbb"], ["ccc"]]) + + def test_loses_no_file_and_keeps_their_order(self): + files = [f"file{n}" for n in range(50)] + batched = [file for batch in run_on_files.batched(files, 20) for file in batch] + self.assertEqual(batched, files) + + def test_yields_nothing_for_no_files(self): + self.assertEqual(list(run_on_files.batched([], 100)), []) + + def test_still_yields_a_file_longer_than_the_limit(self): + # Dropping it would be silent, and the command is a better place for the + # complaint than a batch that never happens. + long = "x" * 200 + self.assertEqual(list(run_on_files.batched([long], 10)), [[long]]) + + +class TestBatchLimit(unittest.TestCase): + def test_leaves_room_for_the_environment_and_a_single_argument(self): + limit = run_on_files.batch_limit() + self.assertGreaterEqual(limit, 4096) + # A single argument is capped far lower than the whole command line, and a + # command handing its arguments on through a shell arrives as one of them. + self.assertLessEqual(limit, 100000) + + +class TestCommaSeparated(unittest.TestCase): + def test_splits_a_group_given_as_one_argument(self): + self.assertEqual(run_on_files.comma_separated("a,b,c"), ["a", "b", "c"]) + + def test_leaves_a_single_pattern_alone(self): + self.assertEqual(run_on_files.comma_separated("a"), ["a"]) + + +class TestCommandLine(TemporaryTree): + files = ( + "project/BUILD.bazel", + "project/with space/BUILD.bazel", + "project/notes.md", + "elsewhere/BUILD.bazel", + ) + + def run_script(self, *arguments, cwd=None): + return subprocess.run( + [sys.executable, str(HERE / "run_on_files.py"), *arguments], + cwd=cwd or self.root, + capture_output=True, + text=True, + ) + + def echo(self, script=ECHO_ARGUMENTS): + return [sys.executable, "-c", script] + + def test_passes_a_name_containing_a_space_as_one_argument(self): + """The reason this program exists rather than a shell command substitution.""" + result = self.run_script("*.bazel", *self.echo(), "--", "project") + self.assertEqual(result.returncode, 0, result.stderr) + printed = result.stdout.splitlines() + self.assertIn(os.path.join("project", "with space", "BUILD.bazel"), printed) + + def test_announces_the_command_once_a_file_has_matched(self): + result = self.run_script("*.bazel", *self.echo(), "--", "project") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("-> ", result.stderr) + self.assertIn(" -- project", result.stderr) + + def test_the_banner_leaves_out_a_path_that_contributed_nothing(self): + """What a repository formatting its own files over a nested one used to say. + + Both paths were named while one of them had every file excluded, so the run + read as covering ground it had already declined to touch. + """ + result = self.run_script( + "--exclude", + f"{self.root / 'project'}/*", + "*.bazel", + *self.echo(), + "--", + "elsewhere", + "project", + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(" -- elsewhere", result.stderr) + self.assertNotIn("project", result.stderr) + + def test_says_nothing_at_all_when_no_file_matches(self): + """Silence has to mean that nothing matched, not that nothing was looked at. + + The banner is the whole point: announced before the collection, it would claim + a formatter ran over a directory it never opened. + """ + result = self.run_script("*.nomatch", *self.echo(), "--", "project") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, "") + self.assertEqual(result.stderr, "") + + def test_refuses_a_path_that_does_not_exist(self): + # Naming a path is an assertion that it is there, so this is not the silent case + # above: an unexpanded glob would otherwise look exactly like nothing to do. + result = self.run_script("*.bazel", *self.echo(), "--", "absent") + self.assertNotEqual(result.returncode, 0) + self.assertIn("no such path: absent", result.stderr) + + def test_needs_the_paths_separated_from_the_command(self): + result = self.run_script("*.bazel", *self.echo(), "project") + self.assertNotEqual(result.returncode, 0) + self.assertIn("--", result.stderr) + + def test_needs_a_command(self): + # Two separators: argparse takes the first for its own end-of-options marker + # when nothing precedes it, so one alone leaves no separator to find and is + # reported as the missing one above. + result = self.run_script("*.bazel", "--", "--", "project") + self.assertNotEqual(result.returncode, 0) + self.assertIn("no command given", result.stderr) + + def test_separates_on_the_last_dash_dash_so_a_command_may_contain_one(self): + result = self.run_script("*.bazel", *self.echo(), "--", "--", "project") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("--", result.stdout.splitlines()) + + def test_drops_the_lines_it_was_told_to_drop(self): + result = self.run_script( + "--drop", + "notes", + "*.bazel,*.md", + *self.echo(ECHO_TO_STDERR), + "--", + "project", + ) + self.assertEqual(result.returncode, 0, result.stdout) + self.assertNotIn("notes.md", result.stderr) + self.assertIn("BUILD.bazel", result.stderr) + + def test_reports_the_command_failing(self): + failing = [sys.executable, "-c", "import sys; sys.exit(3)"] + result = self.run_script("*.bazel", *failing, "--", "project") + self.assertEqual(result.returncode, 3) + + +if __name__ == "__main__": + unittest.main() From b2891024abeb715dc7ad7e0fdbaa4dba147dd243 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:54 +0200 Subject: [PATCH 12/15] Just: document that overriding a shared variable freezes it Overriding one of the shared bazel-formatting variables replaces the whole value, so a root wanting most of the default exclusions plus one of its own must restate all of them, not just add to the list. The same `--evaluate name=value` diagnostic used elsewhere for the override contract itself also shows this in practice. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- misc/just/README.md | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/misc/just/README.md b/misc/just/README.md index 6a738ac79349..a90213544cf1 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -81,7 +81,16 @@ overriding several loses only the renamed one, leaving a half-applied configurat total failure would land in a state someone designed, while partial failure lands in one nobody has ever seen. -Nothing can see it either, because the underscore that keeps these out of `just --list` +An override also freezes what it replaces. A root assigns the whole value, so an +exclusion or a name added here later never reaches one, and `just` offers no way to +append: a root writing `_bazel_excluded := _bazel_excluded + ",mine"` is told the +variable is defined in terms of itself. This runs the opposite way from a rename, where +the override stops applying and the value here wins. Here the override keeps applying +exactly as written, and the roots that never see the addition are the ones that cared +enough about the setting to redirect it. Adding to one of these values is therefore a +change to make on both sides at once. + +Neither shows up anywhere, because the underscore that keeps these out of `just --list` keeps them out of `--variables` and a bare `--evaluate` as well. Asked by name they do answer, which is how a root checks that an override of its own still overrides anything: @@ -90,10 +99,12 @@ just --evaluate _bazel_excluded # what mine is now just --justfile /justfile --evaluate _bazel_excluded # what it would be ``` -A name that has gone says so rather than reporting an empty value. That is a diagnostic -to reach for once something looks wrong, though: it answers whether a name still exists, -not whether its meaning has changed, so it passes happily when the value here gains or -loses a pattern. Rename freely, but say so when handing the change over. +A name that has gone says so rather than reporting an empty value. Read as two values +rather than as two names, the same pair also shows a freeze: a pattern appearing only +under what it would be is one this repository added and the override never received. +Expect differences both ways, since a root that overrode a value usually added something +of its own, and only the missing half is a bug. Rename freely, but say so when handing +the change over. A directory that only makes sense when named explicitly can opt out of being found from above: From 4e31df4e4e464505fb1ac7421764406d722d5fa7 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Wed, 16 Sep 2026 14:06:11 +0200 Subject: [PATCH 13/15] Just: state the version these recipes need `set lists` is what makes argument forwarding work and it did not exist before 1.58, but nothing here said so. The error an older `just` gives is clear and points at the line, yet names no version to move to, which is the gap worth closing in prose rather than with a check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- misc/just/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/misc/just/README.md b/misc/just/README.md index a90213544cf1..454cc3ae3ba9 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -4,6 +4,11 @@ have common verbs (`build`, `test`, `format`, `lint`, `generate`) that individua of the project can implement, and some common functionality that can be used to that effect. +`just` 1.58 or newer is required: recipes forward argument lists using `set lists`, which +is still unstable and did not exist before then. An older one stops with an +`Unknown setting` error pointing at that line, which is clear enough but does not say +which version to move to. + # Forwarding The core of the functionality is given by forwarding. The idea is that: From 6c5350a4fdbdd6a286b41e88b65dc5ab5386a493 Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:51:55 +0200 Subject: [PATCH 14/15] 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> --- actions/ql/test/justfile | 2 +- cpp/ql/test/justfile | 2 +- csharp/ql/test/justfile | 2 +- go/ql/test/justfile | 2 +- java/ql/test-kotlin1/justfile | 2 +- java/ql/test-kotlin2/justfile | 2 +- java/ql/test/justfile | 2 +- javascript/ql/test/justfile | 2 +- misc/just/README.md | 3 +- misc/just/codeql_test_run.py | 189 +++++++++++++------------- misc/just/language_tests.py | 11 +- misc/just/lib.just | 6 +- misc/just/test_codeql_test_run.py | 214 ++++++++++++++++++++++-------- misc/just/test_language_tests.py | 7 - python/ql/test/justfile | 2 +- ruby/ql/test/justfile | 2 +- rust/ql/test/justfile | 2 +- swift/ql/test/justfile | 2 +- unified/ql/test/justfile | 2 +- 19 files changed, 281 insertions(+), 175 deletions(-) diff --git a/actions/ql/test/justfile b/actions/ql/test/justfile index a824f3029972..0c6841ebb405 100644 --- a/actions/ql/test/justfile +++ b/actions/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] all_checks := default_db_checks [no-cd] -test *ARGS=".": (_codeql_test "actions" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "actions" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/cpp/ql/test/justfile b/cpp/ql/test/justfile index 7ccd81541018..bc32c4f8d970 100644 --- a/cpp/ql/test/justfile +++ b/cpp/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := ['--include-location-in-star'] all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-unused-labels', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "cpp" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "cpp" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/csharp/ql/test/justfile b/csharp/ql/test/justfile index ba3e238580c5..5c25af2e049e 100644 --- a/csharp/ql/test/justfile +++ b/csharp/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--additional-packs=ql', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "csharp" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "csharp" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/go/ql/test/justfile b/go/ql/test/justfile index e4f9665c1773..c79e9a543f55 100644 --- a/go/ql/test/justfile +++ b/go/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] 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] [no-cd] -test *ARGS=".": (_codeql_test "go" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "go" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/java/ql/test-kotlin1/justfile b/java/ql/test-kotlin1/justfile index a9815627d15e..4edd755f19fe 100644 --- a/java/ql/test-kotlin1/justfile +++ b/java/ql/test-kotlin1/justfile @@ -10,4 +10,4 @@ base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT='] all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/java/ql/test-kotlin2/justfile b/java/ql/test-kotlin2/justfile index bda00ff0ca75..d3609681ad27 100644 --- a/java/ql/test-kotlin2/justfile +++ b/java/ql/test-kotlin2/justfile @@ -10,4 +10,4 @@ base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT=', 'CODEQL_KOTLIN_LEGAC all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/java/ql/test/justfile b/java/ql/test/justfile index aedf78381a05..f2b6774fa41c 100644 --- a/java/ql/test/justfile +++ b/java/ql/test/justfile @@ -9,4 +9,4 @@ base_flags := ['CODEQL_EXTRACTOR_KOTLIN_DIAGNOSTIC_LIMIT='] all_checks := default_db_checks ++ ['--check-undefined-labels', '--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "java" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/javascript/ql/test/justfile b/javascript/ql/test/justfile index 366b4e1e43dd..37ea1df3c257 100644 --- a/javascript/ql/test/justfile +++ b/javascript/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] all_checks := default_db_checks [no-cd] -test *ARGS=".": (_codeql_test "javascript" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "javascript" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/misc/just/README.md b/misc/just/README.md index 454cc3ae3ba9..ecd8b4de19ef 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -158,7 +158,8 @@ Another point is how launching QL tests can be tweaked: - you can add the additional checks that CI does with `--all-checks` or the `+` abbreviation. These additional checks are configured in justfiles per language, and correspond to all the additional checks that CI adds (but that a dev might not want to - run by default). + run by default). Checks a root passes unconditionally are not offers: what it offers + is what `--all-checks` can enable Test arguments are passed around as `just` lists (`set lists`), so they reach the underlying runner already split and arguments containing spaces survive intact. diff --git a/misc/just/codeql_test_run.py b/misc/just/codeql_test_run.py index 5bda2d5b86eb..a4d70b5a260f 100755 --- a/misc/just/codeql_test_run.py +++ b/misc/just/codeql_test_run.py @@ -5,14 +5,17 @@ python3 codeql_test_run.py LANGUAGE [ARG...] Arguments are already split by `just` (see `set lists`), so each one is taken verbatim. -`--all-checks=FLAG` contributes FLAG to the set of extra checks that `--all-checks` (or -its `+` abbreviation) turns on. +`--extra-check=FLAG` offers FLAG as a check to run, and `--all-checks` (or its `+` +abbreviation) turns the offered ones on. Per-language justfiles supply the offers and +the caller supplies the switch, so the two are separate options rather than one. """ +import argparse import os import re import subprocess import sys +import shutil from pathlib import Path JUST = os.environ.get("JUST_EXECUTABLE", "just") @@ -21,13 +24,12 @@ CMD_END = os.environ.get("CMD_END", "") SEMMLE_CODE = os.environ.get("SEMMLE_CODE") -ALL_CHECKS_PREFIX = "--all-checks=" -ENV_RE = re.compile(r"^[A-Z_][A-Z_0-9]*=.*$") +ENV_RE = re.compile(r"(^[A-Z_][A-Z_0-9]*)=(.*)$") def invoke(invocation, *, cwd=None, log_prefix=""): prefix = f"{log_prefix} " if log_prefix else "" - print(f"{CMD_BEGIN}{prefix}{' '.join(invocation)}{CMD_END}") + print(f"{CMD_BEGIN}{prefix}{' '.join(map(str, invocation))}{CMD_END}") try: subprocess.run(invocation, check=True, cwd=cwd) except subprocess.CalledProcessError as e: @@ -37,114 +39,121 @@ def invoke(invocation, *, cwd=None, log_prefix=""): def error(message): print(f"{ERROR}{message}", file=sys.stderr) + raise SystemExit(1) -def parse_args(args, argv): - """Sort arguments into tests, flags and environment assignments.""" - for arg in argv: - if not arg: - # an empty argument can come from a caller interpolating an unset variable - continue - if arg.startswith(ALL_CHECKS_PREFIX): - args["all_checks"].append(arg[len(ALL_CHECKS_PREFIX) :]) - elif arg.startswith("--codeql="): - args["codeql"] = arg.split("=", 1)[1] - elif arg in ("+", "--all-checks"): - args["all"] = True - elif arg.startswith("-"): - args["flags"].append(arg) - elif ENV_RE.match(arg): - args["env"].append(arg) - else: - args["tests"].append(arg) +class _Parser(argparse.ArgumentParser): + """An `argparse` parser that fails the way the rest of this script does. + The default reports to `stderr` in its own format and exits 2, which would arrive + in a `just` banner unprefixed and alongside a usage line naming this script rather + than the recipe the caller actually typed. + """ -def env_value(args, name, default): - """Resolve a setting from test arguments, then the environment, then a default.""" - for assignment in reversed(args["env"]): - key, _, value = assignment.partition("=") - if key == name and value: - return value - return os.environ.get(name) or default + def error(self, message): + error(message) -def main(): - argv = sys.argv[1:] - if not argv: - error("Usage: codeql_test_run.py LANGUAGE [ARG...]") - return 1 - - language, *rest = argv - - args = { - "tests": [], - "flags": [], - "env": [], - "all_checks": [], - "codeql": "build" if SEMMLE_CODE else "host", - "all": False, - } - parse_args(args, rest) - if args["all"]: - parse_args(args, args["all_checks"]) - - if not SEMMLE_CODE and args["codeql"] in ("build", "built"): - error( +def build_parser(): + # `+` can be an option string only because it is also a prefix character. `-h` and + # `--help` are left unclaimed so that they reach `codeql test run`. + parser = _Parser(add_help=False, allow_abbrev=False, prefix_chars="-+") + parser.add_argument("language") + parser.add_argument("--codeql", default="build" if SEMMLE_CODE else "host") + parser.add_argument("--extra-check", action="append", dest="extra_checks") + parser.add_argument("--all-checks", "+", action="store_true", dest="all") + parser.set_defaults(extra_checks=[], tests=[], flags=[], env={}) + return parser + + +def parse_arguments(): + """Sort a command line into the kinds that are handled differently. + + `argparse` owns the options this script acts on itself. Everything else belongs to + `codeql test run` and has to survive untouched, which is what `parse_known_args` + hands back, and what is sorted by shape here: a test path and a `CPUS=4` are both + positionals, told apart only by how they look. + """ + p = build_parser() + args, rest = p.parse_known_args() + if args.codeql in ("build", "built") and not SEMMLE_CODE: + p.error( "Using `--codeql=build` or `--codeql=built` requires working " "with the internal repository" ) - return 1 - if not args["tests"]: - args["tests"].append(".") + for arg in rest: + if arg.startswith("-"): + args.flags.append(arg) + elif m := ENV_RE.match(arg): + k, v = m.groups() + args.env[k] = v + else: + args.tests.append(arg) + return args + + +def resolve_codeql(args: argparse.Namespace) -> Path: + suffix = ".exe" if sys.platform == "win32" else "" + match args.codeql: + case "built" | "build": + return Path( + SEMMLE_CODE, + "target", + "intree", + f"codeql-{args.language}", + "codeql" + suffix, + ) + case "host": + codeql = shutil.which("codeql" + suffix) + if not codeql: + error("CodeQL executable not found in PATH") + return Path(codeql) + case _: + codeql = Path(args.codeql) + if codeql.is_dir(): + codeql /= "codeql" + suffix + return codeql + + +def main(): + args = parse_arguments() + + if args.all: + # Apply what the language offered by parsing it alongside everything else, so an + # offered check lands exactly where the same flag typed by hand would. + sys.argv[1:1] = args.extra_checks + args = parse_arguments() + + if not args.tests: + args.tests.append(".") + + os.environ.update(args.env) # Resolve these only once all arguments are known, so that a `RAM_PER_THREAD=` test # argument can lower the default on memory-heavy suites. default_ram = 3000 if sys.platform == "linux" else 2048 - ram_per_thread = int(env_value(args, "RAM_PER_THREAD", default_ram)) - cpus = int(env_value(args, "CPUS", os.cpu_count() or 1)) - args["flags"][:0] = [f"--ram={ram_per_thread * cpus}", f"-j{cpus}"] + ram_per_thread = int(os.environ.get("RAM_PER_THREAD") or default_ram) + cpus = int(os.environ.get("CPUS") or os.cpu_count() or 1) + args.flags[:0] = [f"--ram={ram_per_thread * cpus}", f"-j{cpus}"] - if args["codeql"] == "build": - if invoke([JUST, language, "build"], cwd=SEMMLE_CODE) != 0: - return 1 + if args.codeql == "build": + if ret := invoke([JUST, args.language, "build"], cwd=SEMMLE_CODE): + return ret - if args["codeql"] != "host": + if args.codeql != "host": # Disable the default implicit config file, but keep an explicit one. # Same behavior wrt --codeql as the integration test runner. os.environ.setdefault("CODEQL_CONFIG_FILE", ".") - for env_var in args["env"]: - key, _, value = env_var.partition("=") - if not key: - error(f"Invalid environment variable assignment: {env_var}") - return 1 - os.environ[key] = value - - # Resolve codeql executable - if args["codeql"] in ("built", "build"): - codeql = Path(SEMMLE_CODE, "target", "intree", f"codeql-{language}", "codeql") - elif args["codeql"] == "host": - codeql = Path("codeql") - else: - codeql = Path(args["codeql"]) - - if codeql.is_dir(): - codeql = codeql / "codeql" - - # On Windows, prefer codeql.exe over the Unix shell wrapper - if sys.platform == "win32" and codeql.suffix != ".exe": - exe = codeql.with_suffix(".exe") - if exe.exists(): - codeql = exe - - if args["codeql"] != "host" and not codeql.exists(): + codeql = resolve_codeql(args) + + if not codeql.exists(): error(f"CodeQL executable not found: {codeql}") - return 1 return invoke( - [str(codeql), "test", "run", *args["flags"], "--", *args["tests"]], - log_prefix=" ".join(args["env"]), + [codeql, "test", "run", *args.flags, "--", *args.tests], + log_prefix=" ".join(f"{k}={v}" for k, v in args.env.items()), ) diff --git a/misc/just/language_tests.py b/misc/just/language_tests.py index efd5e3f244f0..ab6d4bd5a148 100755 --- a/misc/just/language_tests.py +++ b/misc/just/language_tests.py @@ -4,8 +4,10 @@ Called from just recipes as: python3 language_tests.py ROOT [ARG...] -Arguments are already split by `just` (see `set lists`). The first one must be a test -root, which is used to locate the justfile implementing `test` for that suite. +Arguments are already split by `just` (see `set lists`). Only the first locates a +justfile: its `test` recipe is run once, and every argument after it is handed to that +one recipe rather than visited in turn. Roots wanting different `test` recipes therefore +cannot be run together. """ import os @@ -15,10 +17,7 @@ def main(): - # Blank arguments are dropped before the count is taken: one comes of a caller - # interpolating a variable that was never set, and a list of nothing but those is no - # arguments at all rather than a root to find a justfile above. - argv = [arg for arg in sys.argv[1:] if arg] + argv = sys.argv[1:] if not argv: print("Usage: language_tests.py ROOT [ARG...]", file=sys.stderr) return 1 diff --git a/misc/just/lib.just b/misc/just/lib.just index 6a254b4e99b8..84389e84b1d3 100644 --- a/misc/just/lib.just +++ b/misc/just/lib.just @@ -5,9 +5,9 @@ import "format.just" # Run language tests for LANGUAGE. # -# Arguments tagged with `--all-checks=` are held back and only applied when `--all-checks` -# or `+` is passed along, which is how per-language justfiles express the extra checks CI -# runs on top of the default ones. +# `--extra-check=` offers a check without running it, and `--all-checks` or `+` runs the +# offered ones. Per-language justfiles supply the offers, which is how the extra checks +# CI runs stay next to the language they belong to. [no-cd] [no-exit-message] [positional-arguments] diff --git a/misc/just/test_codeql_test_run.py b/misc/just/test_codeql_test_run.py index 28e8cfffd8bc..acb88ac11db6 100644 --- a/misc/just/test_codeql_test_run.py +++ b/misc/just/test_codeql_test_run.py @@ -5,70 +5,110 @@ at each one: a word is a test, a `-` is a flag, `NAME=value` is an environment assignment. Several of these pin down that an argument arrives whole, spaces and all, which is what taking them as a list rather than re-splitting a string bought. + +Sorting and resolving are tested at different levels because they happen at different +levels. `parse_arguments` only sorts; a setting is not resolved until `main` has merged +the assignments into the environment, so anything about precedence is observed there. """ import os +import sys import unittest from unittest import mock import codeql_test_run -def empty_args(): - """What `main` builds before sorting, limited to what `parse_args` fills in.""" - return { - "tests": [], - "flags": [], - "env": [], - "all_checks": [], - "codeql": "host", - "all": False, - } +def sorted_args(*argv, semmle_code=None): + """Sort a command line, supplying the language that always precedes it.""" + with ( + mock.patch.object(codeql_test_run, "SEMMLE_CODE", semmle_code), + mock.patch.object(sys, "argv", ["codeql_test_run.py", "alanguage", *argv]), + ): + return codeql_test_run.parse_arguments() + + +def run_main(*argv, environ=None): + """Run `main` with the executable and the child process stubbed out. + + Returns the flags and tests handed to `codeql test run`, and the environment the + child would have been given. + """ + codeql = mock.MagicMock() + codeql.exists.return_value = True + with ( + mock.patch.object(codeql_test_run, "SEMMLE_CODE", None), + mock.patch.object(sys, "argv", ["codeql_test_run.py", "alanguage", *argv]), + mock.patch.dict(os.environ, environ or {}, clear=True), + mock.patch.object(codeql_test_run, "resolve_codeql", return_value=codeql), + mock.patch.object(codeql_test_run, "invoke", return_value=0) as invoke, + ): + codeql_test_run.main() + (invocation,) = invoke.call_args.args + separator = invocation.index("--") + # Past the executable and `test run`, up to the separator `main` adds itself. + return invocation[3:separator], invocation[separator + 1 :], dict(os.environ) + +def flags(*argv, environ=None): + return run_main(*argv, environ=environ)[0] -def sorted_args(*argv): - args = empty_args() - codeql_test_run.parse_args(args, list(argv)) - return args + +def paths(*argv, environ=None): + return run_main(*argv, environ=environ)[1] + + +def child_environ(*argv, environ=None): + return run_main(*argv, environ=environ)[2] class TestParseArgs(unittest.TestCase): def test_a_plain_word_is_a_test(self): - self.assertEqual(sorted_args("ql/test/Foo")["tests"], ["ql/test/Foo"]) + self.assertEqual(sorted_args("ql/test/Foo").tests, ["ql/test/Foo"]) def test_a_dash_is_a_flag(self): - self.assertEqual(sorted_args("--fail-on-trap-errors")["flags"], ["--fail-on-trap-errors"]) + self.assertEqual( + sorted_args("--fail-on-trap-errors").flags, ["--fail-on-trap-errors"] + ) def test_an_uppercase_assignment_is_an_environment_variable(self): - self.assertEqual(sorted_args("CPUS=4")["env"], ["CPUS=4"]) + self.assertEqual(sorted_args("CPUS=4").env, {"CPUS": "4"}) def test_a_lowercase_assignment_is_a_test(self): # Only shouting counts, so a path that happens to contain `=` stays a path. - self.assertEqual(sorted_args("dir/a=b")["tests"], ["dir/a=b"]) + self.assertEqual(sorted_args("dir/a=b").tests, ["dir/a=b"]) def test_codeql_selects_the_executable(self): - self.assertEqual(sorted_args("--codeql=built")["codeql"], "built") + # `built` is rejected outright without an internal checkout to build in, so + # this says what it means to sort the option, not to act on it. + args = sorted_args("--codeql=built", semmle_code="/somewhere") + self.assertEqual(args.codeql, "built") def test_the_last_codeql_wins(self): - self.assertEqual(sorted_args("--codeql=host", "--codeql=built")["codeql"], "built") + args = sorted_args("--codeql=host", "--codeql=built", semmle_code="/somewhere") + self.assertEqual(args.codeql, "built") def test_all_checks_is_asked_for_by_either_spelling(self): - self.assertTrue(sorted_args("--all-checks")["all"]) - self.assertTrue(sorted_args("+")["all"]) + self.assertTrue(sorted_args("--all-checks").all) + self.assertTrue(sorted_args("+").all) def test_an_extra_check_is_held_back_until_it_is_asked_for(self): - held = sorted_args("--all-checks=--check-databases") - self.assertEqual(held["all_checks"], ["--check-databases"]) + held = sorted_args("--extra-check=--check-databases") + self.assertEqual(held.extra_checks, ["--check-databases"]) # Held back means held back: it is not a flag until `--all-checks` arrives. - self.assertEqual(held["flags"], []) - self.assertFalse(held["all"]) + self.assertEqual(held.flags, []) + self.assertFalse(held.all) - def test_an_empty_argument_is_ignored(self): - # One of these comes of a caller interpolating a variable that was never set. - self.assertEqual(sorted_args("", "test")["tests"], ["test"]) + def test_a_double_dash_hands_everything_after_it_to_codeql(self): + # Standard `--`: past it, an option is the caller's business and not ours. The + # separator itself is ours, though, so it is not passed on as well. + args = sorted_args("--", "--codeql=built") + self.assertEqual(args.codeql, "host") + self.assertIn("--codeql=built", args.flags) + self.assertNotIn("--", args.flags) def test_a_test_path_containing_a_space_stays_one_argument(self): - self.assertEqual(sorted_args("some dir/test")["tests"], ["some dir/test"]) + self.assertEqual(sorted_args("some dir/test").tests, ["some dir/test"]) def test_an_assignment_whose_value_contains_a_space_stays_whole(self): """The value is the point, and a split one used to arrive as three characters. @@ -76,43 +116,107 @@ def test_an_assignment_whose_value_contains_a_space_stays_whole(self): An argument list carries this; a whitespace-separated string cannot, as there is nothing left in it to tell a separator from part of a value. """ - self.assertEqual(sorted_args("EXTRA=a b")["env"], ["EXTRA=a b"]) + self.assertEqual(sorted_args("EXTRA=a b").env, {"EXTRA": "a b"}) def test_sorts_a_whole_command_line_at_once(self): - args = sorted_args("-j2", "CPUS=4", "ql/test", "+", "--all-checks=--check-diff") - self.assertEqual(args["flags"], ["-j2"]) - self.assertEqual(args["env"], ["CPUS=4"]) - self.assertEqual(args["tests"], ["ql/test"]) - self.assertEqual(args["all_checks"], ["--check-diff"]) - self.assertTrue(args["all"]) + args = sorted_args( + "-j2", "CPUS=4", "ql/test", "+", "--extra-check=--check-diff" + ) + self.assertEqual(args.flags, ["-j2"]) + self.assertEqual(args.env, {"CPUS": "4"}) + self.assertEqual(args.tests, ["ql/test"]) + self.assertEqual(args.extra_checks, ["--check-diff"]) + self.assertTrue(args.all) + + +class TestOfferedChecks(unittest.TestCase): + """What a root offers and what `--all-checks` enables are separate things. + + These go through `main` because that is where the two meet. `--all-checks` is + injected on every language test run rather than typed, so it means "enable whatever + this root offers" and not "I want more coverage": a root offering nothing has to + stay runnable through it. + """ + + def test_an_offered_check_is_applied_when_asked_for(self): + applied = flags("--extra-check=--check-databases", "--all-checks") + self.assertIn("--check-databases", applied) + + def test_an_offered_check_stays_held_back_until_it_is(self): + self.assertNotIn("--check-databases", flags("--extra-check=--check-databases")) + + def test_asking_for_checks_a_root_offers_none_of_enables_nothing(self): + self.assertEqual(paths("--all-checks", "some/test"), ["some/test"]) + self.assertEqual(flags("--all-checks", "some/test"), flags("some/test")) + + def test_a_check_passed_unconditionally_is_not_an_offer(self): + # A root can mean to run a check always rather than put it behind the flag. That + # is a flag like any other here, so it neither becomes an offer nor is withheld + # until the offers are asked for. + always = flags("--check-databases", "some/test") + self.assertIn("--check-databases", always) + self.assertEqual( + flags("--check-databases", "--all-checks", "some/test"), always + ) + + +class TestSettings(unittest.TestCase): + """`RAM_PER_THREAD` and `CPUS` are read back after assignments are applied. + + Resolution is what these pin down, so they go through `main`: an assignment and an + inherited variable only meet once `main` has merged them. + """ - -class TestEnvValue(unittest.TestCase): def test_prefers_a_test_argument(self): - args = sorted_args("CPUS=4") - with mock.patch.dict(os.environ, {"CPUS": "8"}): - self.assertEqual(codeql_test_run.env_value(args, "CPUS", "1"), "4") + self.assertIn("-j4", flags("CPUS=4", environ={"CPUS": "8"})) def test_falls_back_to_the_environment(self): - with mock.patch.dict(os.environ, {"CPUS": "8"}): - self.assertEqual(codeql_test_run.env_value(empty_args(), "CPUS", "1"), "8") + self.assertIn("-j8", flags(environ={"CPUS": "8"})) def test_falls_back_to_the_default(self): - with mock.patch.dict(os.environ, {}, clear=True): - self.assertEqual(codeql_test_run.env_value(empty_args(), "CPUS", "1"), "1") + self.assertIn(f"-j{os.cpu_count()}", flags()) def test_the_last_assignment_wins(self): - args = sorted_args("CPUS=4", "CPUS=2") - self.assertEqual(codeql_test_run.env_value(args, "CPUS", "1"), "2") + self.assertIn("-j2", flags("CPUS=4", "CPUS=2")) + + def test_an_empty_value_falls_back_to_the_default(self): + """An empty value does not override, so a later one erases an earlier setting. + + Compared against a run that never mentions the setting, so this pins the + behaviour without restating what the default happens to be. It is about + resolution only: see below for what the child is given. + """ + self.assertEqual(flags("CPUS=4", "CPUS="), flags()) + + def test_an_empty_assignment_still_reaches_the_child(self): + """Falling back to the default is not the same as the assignment being dropped. + + `RAM_PER_THREAD` and `CPUS` are read back out of the environment, so an empty + one reads as unset and the default stands. Every assignment is exported either + way, so the child sees the variable set and empty rather than absent, and a + variable this script does not read has no other behaviour to fall back to. + """ + self.assertEqual(child_environ("CPUS=")["CPUS"], "") + self.assertNotIn("CPUS", child_environ()) + + def test_an_assignment_reaches_the_child(self): + self.assertEqual(child_environ("EXTRA=a b")["EXTRA"], "a b") + + def test_ram_is_per_thread(self): + self.assertIn("--ram=200", flags("CPUS=2", "RAM_PER_THREAD=100")) + + +class TestDefaults(unittest.TestCase): + def test_the_current_directory_is_the_default_test(self): + self.assertEqual(paths(), ["."]) - def test_an_empty_value_does_not_count_as_a_setting(self): - args = sorted_args("CPUS=") - with mock.patch.dict(os.environ, {"CPUS": "8"}): - self.assertEqual(codeql_test_run.env_value(args, "CPUS", "1"), "8") + def test_a_named_test_replaces_the_default(self): + self.assertEqual(paths("ql/test/Foo"), ["ql/test/Foo"]) - def test_a_value_containing_a_space_survives(self): - args = sorted_args("EXTRA=a b") - self.assertEqual(codeql_test_run.env_value(args, "EXTRA", "none"), "a b") + def test_an_offered_check_becomes_a_flag_once_asked_for(self): + self.assertIn( + "--check-databases", flags("--extra-check=--check-databases", "+") + ) if __name__ == "__main__": diff --git a/misc/just/test_language_tests.py b/misc/just/test_language_tests.py index d1865599f9f4..b2e93a28306c 100644 --- a/misc/just/test_language_tests.py +++ b/misc/just/test_language_tests.py @@ -109,13 +109,6 @@ def test_needs_a_root(self): self.assertEqual(status, 1) run.assert_not_called() - def test_nothing_but_blank_arguments_is_no_arguments(self): - # An unset variable interpolated by a caller arrives as one of these. Counting - # it as an argument and then dropping it left nothing to take a root from. - status, run, _ = self.run_main("", "") - self.assertEqual(status, 1) - run.assert_not_called() - def test_reports_a_root_with_no_justfile_above_it(self): orphan = self.semmle_code / "elsewhere" orphan.mkdir() diff --git a/python/ql/test/justfile b/python/ql/test/justfile index 0f44a489e82f..c02c78ba8241 100644 --- a/python/ql/test/justfile +++ b/python/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := _python_env all_checks := default_db_checks ++ ['--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "python" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "python" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/ruby/ql/test/justfile b/ruby/ql/test/justfile index 9673cebe0370..ab79be8d890e 100644 --- a/ruby/ql/test/justfile +++ b/ruby/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] 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] [no-cd] -test *ARGS=".": (_codeql_test "ruby" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "ruby" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/rust/ql/test/justfile b/rust/ql/test/justfile index 8d5d6c05da2d..8b6133008e8a 100644 --- a/rust/ql/test/justfile +++ b/rust/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] all_checks := default_db_checks ++ ['--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "rust" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "rust" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/swift/ql/test/justfile b/swift/ql/test/justfile index 6f15ac6d0723..f396305ed282 100644 --- a/swift/ql/test/justfile +++ b/swift/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] all_checks := default_db_checks ++ ['--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "swift" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "swift" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) diff --git a/unified/ql/test/justfile b/unified/ql/test/justfile index 1ca509465bd8..4a097e0b1d65 100644 --- a/unified/ql/test/justfile +++ b/unified/ql/test/justfile @@ -8,4 +8,4 @@ base_flags := [] all_checks := default_db_checks ++ ['--check-repeated-labels', '--check-redefined-labels', '--check-use-before-definition', '--consistency-queries=' + consistency_queries] [no-cd] -test *ARGS=".": (_codeql_test "unified" (base_flags ++ prepend('--all-checks=', all_checks) ++ ARGS)) +test *ARGS=".": (_codeql_test "unified" (base_flags ++ prepend('--extra-check=', all_checks) ++ ARGS)) From 66752e54bee69068315139e6f71811643208877b Mon Sep 17 00:00:00 2001 From: Paolo Tranquilli Date: Fri, 18 Sep 2026 15:52:04 +0200 Subject: [PATCH 15/15] Just: assorted follow-up fixes - restate which `just` version these recipes actually need, correcting the earlier guess - three recipes (Kotlin extractor generation and codegen) do what their name promised, which they had stopped doing since being folded into the shared verbs - a coding-standards reference in `cpp/justfile` follows the fragment to where it now lives - `format` reaches the rest of the checked Python, including the models-as-data scripts, which had no justfile of their own Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- cpp/justfile | 2 +- java/ql/test-kotlin1/justfile | 5 +++-- java/ql/test-kotlin2/justfile | 5 +++-- misc/codegen/justfile | 2 +- misc/just/README.md | 12 ++++++++---- misc/scripts/models-as-data/justfile | 3 +++ python/justfile | 2 +- 7 files changed, 20 insertions(+), 11 deletions(-) create mode 100644 misc/scripts/models-as-data/justfile diff --git a/cpp/justfile b/cpp/justfile index 0fc87260ebec..a0a07f94da3a 100644 --- a/cpp/justfile +++ b/cpp/justfile @@ -1,5 +1,5 @@ import '../lib.just' -import? '../../cpp-coding-standards.just' +import? '../../buildutils-internal/just/cpp-coding-standards.just' [group('build')] build: (_build_dist "cpp") diff --git a/java/ql/test-kotlin1/justfile b/java/ql/test-kotlin1/justfile index 4edd755f19fe..fc29900a05c7 100644 --- a/java/ql/test-kotlin1/justfile +++ b/java/ql/test-kotlin1/justfile @@ -1,7 +1,8 @@ import "../justfile" -# These are CI shards of the Kotlin language tests, run as `just java -# kotlin-language-tests-1`, so they only run when asked for by name. +# These are CI shards of the Kotlin language tests, too long to run by accident, +# so a verb coming from above passes over them and they run only when this +# directory is named. explicit_verbs := ['test'] # Kotlin tests may fail the diags.ql consistency test if the diagnostic limit is set. diff --git a/java/ql/test-kotlin2/justfile b/java/ql/test-kotlin2/justfile index d3609681ad27..4ef773c27ecd 100644 --- a/java/ql/test-kotlin2/justfile +++ b/java/ql/test-kotlin2/justfile @@ -1,7 +1,8 @@ import "../justfile" -# These are CI shards of the Kotlin language tests, run as `just java -# kotlin-language-tests-2`, so they only run when asked for by name. +# These are CI shards of the Kotlin language tests, too long to run by accident, +# so a verb coming from above passes over them and they run only when this +# directory is named. explicit_verbs := ['test'] # Kotlin tests may fail the diags.ql consistency test if the diagnostic limit is set. diff --git a/misc/codegen/justfile b/misc/codegen/justfile index a65fa16e5679..bd632b74817b 100644 --- a/misc/codegen/justfile +++ b/misc/codegen/justfile @@ -1,5 +1,5 @@ import "../just/lib.just" -test *ARGS="": (_bazel ['test', '@codeql//misc/codegen/...']) +test *ARGS: (_bazel (['test', '@codeql//misc/codegen/...'] ++ ARGS)) format *ARGS=".": (_format_py ARGS) diff --git a/misc/just/README.md b/misc/just/README.md index ecd8b4de19ef..9dc2693e36b1 100644 --- a/misc/just/README.md +++ b/misc/just/README.md @@ -4,10 +4,14 @@ have common verbs (`build`, `test`, `format`, `lint`, `generate`) that individua of the project can implement, and some common functionality that can be used to that effect. -`just` 1.58 or newer is required: recipes forward argument lists using `set lists`, which -is still unstable and did not exist before then. An older one stops with an -`Unknown setting` error pointing at that line, which is clear enough but does not say -which version to move to. +`just` 1.53 or newer is required. Recipes forward argument lists rather than encoding +them as whitespace separated strings, and every piece of that arrived in that one +release: `set lists`, list literals, the `++` operator, and variadic parameters that pass +their elements on one at a time rather than space-joined. Lists are still unstable, at +1.58 as much as at 1.53, which is what `set unstable` in `defs.just` is for. An older +`just` stops at the first list literal it parses, complaining about an unexpected `[` and +saying nothing about versions; only `defs.just` read on its own gives the clearer +`unknown setting` naming `lists`. # Forwarding diff --git a/misc/scripts/models-as-data/justfile b/misc/scripts/models-as-data/justfile new file mode 100644 index 000000000000..bb5cb98694f2 --- /dev/null +++ b/misc/scripts/models-as-data/justfile @@ -0,0 +1,3 @@ +import "../../just/lib.just" + +format *ARGS=".": (_format_py ARGS) diff --git a/python/justfile b/python/justfile index 33580aa1e05c..dec897cdf672 100644 --- a/python/justfile +++ b/python/justfile @@ -6,7 +6,7 @@ build: (_build_dist "python") # Long filename needed for extractor tests (too long for Git on Windows) [no-cd] -@_ensure_long_filename: +@_ensure_long_filename: _require_semmle_code #!/usr/bin/env bash longfile="$SEMMLE_CODE/ql/python/ql/test/extractor-tests/long_path/really_rather_too_long_for_windows_path_length/with_unecessarily_longwinded_and_verbose_sub_folder/extremely_long_module_name_with_lots_of_digits_at_the_end_000000000000000000000000000000000000000000000000000000000000000000/test0000000000000000000000000000000000000000000000000000000.py" mkdir -p "$(dirname "$longfile")"