diff --git a/CMakeLists.txt b/CMakeLists.txt index 94ebb8def..a1daffc0e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -266,9 +266,8 @@ add_test(NAME copyright_gate COMMAND ${CMAKE_SOURCE_DIR}/tests/copyright_gate_te # engine#1701: the NPU hold's queue/token follow ONEBIT_HOLD_DIR, and its queue settings never reach the payload. # Shell only (a stub gpu-hold plus the tree's own), no device, ~1 s. add_test(NAME npu_hold COMMAND bash ${CMAKE_SOURCE_DIR}/tests/npu_hold_test.sh) -# engine#1696: the gpu_hold_* registrations live in their own file, so adding one does not conflict with every -# other test-adding PR through the one contiguous add_test block this used to be part of (see that file's header). -include(cmake/tests-gpu-hold.cmake) +# engine#1696: the gpu_hold_* registrations live in cmake/tests-gpu-hold.cmake. It is no longer named here: every +# cmake/tests-*.cmake is included by the glob near the end of this file (engine#1979). # engine#1485: the two port read-backs the route tests use (tests/serve_port.sh). Script-based, no device, no model -- # it feeds the parsers the exact lines `1bit serve` and `llama-server` print, because a read-back that silently returns # empty turns into "the server did not come up" and is blamed on whatever the row under test checks. @@ -1723,6 +1722,42 @@ if(ONEBIT_NPU_PRIVATE) endif() endif() +# ── Test registrations that live in their own file (engine#1696, engine#1979) ─ +# Every cmake/tests-*.cmake is included here, in name order. A test added as a NEW file there touches no line any +# other pull request touches, so it cannot conflict and its registration cannot go stale while the PR waits: 37 commits +# changed this file in the 24 h before engine#1979 was filed, and three open PRs had a registration hunk that no longer +# applied. The rows above stay where they are; nothing has to move. +# Here, after every target and after find_package(Python3), so a file may use if(TARGET ...), add_python_test() and +# ${Python3_EXECUTABLE}; and before the RESOURCE_LOCK loop below, which asks if(TEST ...). +# CONFIGURE_DEPENDS re-runs the glob at build time, so a file added after the first configure is picked up. +# Readers that follow this shape: tools/ci/ci_targets_check.py (the glob), tools/ci/hrx_build_scope.py and +# .github/workflows/hrx-build.yml (the cmake/tests-*.cmake path), tools/ci/tests_referenced_check.py (all of cmake/). +# The lines between the two markers are run as they stand by tests/registration_glob_test.sh, in a scratch project. +# >>> registration glob +file(GLOB ONEBIT_TEST_REGISTRATION_FILES CONFIGURE_DEPENDS ${CMAKE_SOURCE_DIR}/cmake/tests-*.cmake) +list(SORT ONEBIT_TEST_REGISTRATION_FILES) +# A file this CMakeLists.txt already include()s by name is skipped: that include fixes the file's position (some rows +# need one), and including it twice stops the configure with `add_test given test NAME ... which already exists`. So a +# family file moved out with its own include() line and this glob can land in either order. +# "By name" means a line CMake would execute -- include( at the start of a line, the same rule +# tools/ci/ci_targets_check.py reads by -- and not the same text in a comment: a substring search took +# `# was include(cmake/tests-gpu-hold.cmake)` for an include, and the seven gpu_hold_* tests left the configuration with +# every gate green (review of engine#1983). An include this pattern does not recognise is the loud miss: both includes +# run and the configure stops. +file(READ ${CMAKE_CURRENT_LIST_FILE} _onebit_top_cmake_text) +set(_onebit_include_prefix "(^|\n)[ \t]*include\\([ \t]*\"?(\\$\\{CMAKE_(CURRENT_)?SOURCE_DIR\\}/)?cmake/") +foreach(_onebit_test_registration_file IN LISTS ONEBIT_TEST_REGISTRATION_FILES) + get_filename_component(_onebit_test_registration_name ${_onebit_test_registration_file} NAME) + string(REGEX REPLACE "([.+])" "\\\\\\1" _onebit_test_registration_re "${_onebit_test_registration_name}") + string(REGEX MATCH "${_onebit_include_prefix}${_onebit_test_registration_re}\"?[ \t]*\\)" _onebit_named + "${_onebit_top_cmake_text}") + if(_onebit_named STREQUAL "") + include(${_onebit_test_registration_file}) + endif() +endforeach() +unset(_onebit_top_cmake_text) +# <<< registration glob + # ── Loopback ports: route tests run one at a time (engine#1070) ────────────── # Every test named below starts a server on 127.0.0.1. Most pick a port by binding :0, closing the # socket, then letting the child bind that number; three use fixed 43xxx literals. Under `ctest -j` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7d5f58f6d..b86b8a268 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -87,6 +87,17 @@ an RFC or issue. hands the script to the interpreter instead of relying on the file's exec bit; `tests/python_test_order_check.py --check` enforces that it follows `find_package(Python3)`. Two rows may not share a name: CMake stops at configure with `add_test given test NAME "…" which already exists in this directory`. +- **A new test's registration goes in its own file.** Put the `add_test(...)` (or `add_python_test(...)`) row, its + comment, and its `set_tests_properties` in a new `cmake/tests-.cmake` with the Apache notice, and do not touch + `CMakeLists.txt` for it: every `cmake/tests-*.cmake` is included by a glob near the end of `CMakeLists.txt`, after all + targets exist. A new file cannot conflict with another pull request, and a row appended to `CMakeLists.txt` does — + that file changed 37 times in one day and three open pull requests had a registration hunk that no longer applied + (engine [#1979](https://github.com/1bit-MONSTER/engine/issues/1979)). A row that belongs to an existing family goes in + that family's file (`cmake/tests-gpu-hold.cmake`). An `add_executable` for a test binary still goes in + `CMakeLists.txt` next to the sources it builds; only the registration moves. Existing rows stay where they are. + A registration file that needs a fixed position may still be `include()`d by name from `CMakeLists.txt`; the glob + then leaves it alone. `tests/registration_glob_test.sh` (itself registered from `cmake/tests-registration-glob.cmake`) + runs the glob block and holds both rules. - **Pins only move forward.** CI fails a pull request whose submodule pin is behind, or has diverged from, the pin on `main` (`tools/check_pins.py`), because that drops commits the engine already shipped. A rollback on purpose carries the `pin rollback` label. diff --git a/cmake/tests-registration-glob.cmake b/cmake/tests-registration-glob.cmake new file mode 100644 index 000000000..2e4e1b43b --- /dev/null +++ b/cmake/tests-registration-glob.cmake @@ -0,0 +1,23 @@ +# Copyright 2026 bong-water-water-bong +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# The registration glob's own test (engine#1979), registered the way it asks new tests to be: this file is named by +# nothing and is picked up by the cmake/tests-*.cmake glob near the end of CMakeLists.txt. +# +# It configures the glob block of the real CMakeLists.txt in scratch projects: a new file is registered, a file named +# only in a comment is still registered, and a file CMakeLists.txt include()s by name is registered once. Configure +# only (project(... NONE)): no compiler, no device, about a second. +add_test(NAME registration_glob + COMMAND bash ${CMAKE_SOURCE_DIR}/tests/registration_glob_test.sh ${CMAKE_COMMAND} ${CMAKE_SOURCE_DIR}/CMakeLists.txt) diff --git a/tests/registration_glob_test.sh b/tests/registration_glob_test.sh new file mode 100755 index 000000000..5b83cec19 --- /dev/null +++ b/tests/registration_glob_test.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# Copyright 2026 bong-water-water-bong +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# The cmake/tests-*.cmake glob in CMakeLists.txt, run as it stands (engine#1979). +# +# The block between `# >>> registration glob` and `# <<< registration glob` is cut out of the real CMakeLists.txt and +# configured in a scratch project, so what is tested is the text main configures with, not a copy of it. Each case +# says which tests ctest must then list: +# - a new file nothing names -> its test is registered (the point of the glob), +# - the same file named only in a COMMENT -> still registered. A substring search skipped it +# here: the family's tests left the configuration and no gate noticed (review of engine#1983), +# - a file CMakeLists.txt include()s by name, four spellings -> registered once; a second include would stop the +# configure with `add_test given test NAME ... which already exists`, +# - tests-a.b.cmake beside an include of tests-axb.cmake -> both registered: the dot is not a wildcard. +# +# usage: tests/registration_glob_test.sh (configure only: no compiler, no device) +set -uo pipefail +CMAKE=${1:?usage: registration_glob_test.sh } +TOP=${2:?usage: registration_glob_test.sh } +CTEST="$(dirname "$CMAKE")/ctest" +[ -x "$CTEST" ] || CTEST=ctest +T=$(mktemp -d); trap 'rm -rf "$T"' EXIT +BLOCK=$(sed -n '/^# >>> registration glob$/,/^# <<< registration glob$/p' "$TOP") +if [ -z "$BLOCK" ] || ! printf '%s\n' "$BLOCK" | grep -q 'file(GLOB'; then + echo "FAIL: no '# >>> registration glob' block with a file(GLOB in $TOP"; exit 1 +fi +fails=0 +# case ... +case_() { + local what=$1 want=$2 prelude=$3; shift 3 + local d="$T/p$((++n))"; mkdir -p "$d/cmake" + local f + for f in "$@"; do + printf 'add_test(NAME %s COMMAND true)\n' "$(printf '%s' "${f%.cmake}" | tr -c 'A-Za-z0-9_\n' '_')" > "$d/cmake/$f" + done + { printf 'cmake_minimum_required(VERSION 3.20)\nproject(regglob NONE)\nenable_testing()\n%s\n%s\n' "$prelude" "$BLOCK" + } > "$d/CMakeLists.txt" + local got + if ! "$CMAKE" -S "$d" -B "$d/b" > "$d/log" 2>&1; then + got="CONFIGURE FAILED: $(grep -m1 -A1 'CMake Error' "$d/log" | tr '\n' ' ')" + else + got=$(cd "$d/b" && "$CTEST" -N | sed -n 's/^ *Test *#[0-9]*: //p' | sort | tr '\n' ' ' | sed 's/ $//') + fi + if [ "$got" = "$want" ]; then echo " ok $what"; else echo " FAIL $what: got '$got', wanted '$want'"; fails=$((fails + 1)); fi +} +n=0 +case_ "a new file nothing names is registered" "tests_one" "" tests-one.cmake +case_ "a file named only in a comment is still registered" "tests_one" \ + "# history: this used to be include(cmake/tests-one.cmake) right here" tests-one.cmake +case_ "a file named in a trailing comment is still registered" "tests_one" \ + "set(X 1) # was include(cmake/tests-one.cmake)" tests-one.cmake +case_ "a file included by name is registered once" "tests_one tests_two" \ + "include(cmake/tests-one.cmake)" tests-one.cmake tests-two.cmake +case_ "an indented include under if() counts as an include" "tests_one" \ + "$(printf 'if(TRUE)\n include(cmake/tests-one.cmake)\nendif()')" tests-one.cmake +case_ 'a ${CMAKE_SOURCE_DIR}/ include counts as an include' "tests_one" \ + 'include(${CMAKE_SOURCE_DIR}/cmake/tests-one.cmake)' tests-one.cmake +case_ "a quoted include counts as an include" "tests_one" \ + 'include("cmake/tests-one.cmake")' tests-one.cmake +case_ "the dot in a file name is not a wildcard" "tests_a_b tests_axb" \ + "include(cmake/tests-axb.cmake)" tests-a.b.cmake tests-axb.cmake +case_ "a file the glob does not match is not registered" "tests_one" "" tests-one.cmake other.cmake +echo "registration_glob_test: $fails failure(s)" +[ "$fails" -eq 0 ] diff --git a/tools/ci/ci_targets_check.py b/tools/ci/ci_targets_check.py index 4ac56efec..4c103ef67 100755 --- a/tools/ci/ci_targets_check.py +++ b/tools/ci/ci_targets_check.py @@ -65,7 +65,9 @@ # shape engine#1696 landed. The include argument is a VARIABLE, so INCLUDE above cannot match it, and a # reader that only followed literal includes would stop seeing every registration at once - the count # would fall to zero and read as "the gap closed". -GLOB = re.compile(r'file\s*\(\s*GLOB\s+(\w+)[^)]*?([^\s")]+/\*\.cmake)', re.IGNORECASE) +# engine#1979: the pattern main uses is `cmake/tests-*.cmake` (a prefix before the star), not `/*.cmake`; a +# reader that wanted the star right after the slash matched nothing there, and every such file went unread. +GLOB = re.compile(r'file\s*\(\s*GLOB\s+(\w+)[^)]*?([^\s")]+/[^\s"/)]*\*[^\s"/)]*\.cmake)', re.IGNORECASE) INCLUDE_VAR = re.compile(r'include\(\s*\$\{(\w+)\}\s*\)') FOREACH_LISTS = re.compile(r'foreach\s*\(\s*(\w+)\s+IN\s+LISTS\s+(\w+)', re.IGNORECASE) DEFINE = re.compile(r"-D([A-Za-z0-9_]+)=([^\s\"]*)") @@ -352,6 +354,7 @@ def family_text(root, cmake_text): tracks a guard stack across the text this returns. """ out = [] + seen = set() # a glob the include loop iterates: every file matching it is configured, so every one is read for gvar, pattern in GLOB.findall(cmake_text): # the loop variable differs from the glob variable: `foreach(v IN LISTS GLOBVAR) include(${v})` @@ -360,12 +363,16 @@ def family_text(root, cmake_text): continue pat = pattern.replace("${CMAKE_SOURCE_DIR}/", "").replace("${PROJECT_SOURCE_DIR}/", "") for path in sorted(glob.glob(os.path.join(root, pat))): - if os.path.isfile(path): + if os.path.isfile(path) and os.path.realpath(path) not in seen: + seen.add(os.path.realpath(path)) out.append(open(path, encoding="utf-8").read()) for m in INCLUDE.finditer(cmake_text): rel = m.group(1).replace("${CMAKE_SOURCE_DIR}/", "").replace("${PROJECT_SOURCE_DIR}/", "") path = os.path.join(root, rel) - if os.path.isfile(path): + # engine#1979: a file both named by a literal include() and matched by the glob is configured once + # (CMakeLists.txt skips it in the glob loop), so it is read once. + if os.path.isfile(path) and os.path.realpath(path) not in seen: + seen.add(os.path.realpath(path)) out.append(open(path, encoding="utf-8").read()) return "\n".join(out) @@ -516,16 +523,16 @@ def self_test(): import tempfile fails = 0 - def case(what, ci, cmake, want, baseline="auto", family=None): + def case(what, ci, cmake, want, baseline="auto", family=None, family_rel="cmake/tests/zz_family.cmake"): nonlocal fails with tempfile.TemporaryDirectory() as d: os.makedirs(os.path.join(d, ".github", "workflows")) open(os.path.join(d, CI), "w", encoding="utf-8").write(ci) open(os.path.join(d, CMAKE), "w", encoding="utf-8").write(cmake) if family is not None: - fam_dir = os.path.join(d, "cmake", "tests") - os.makedirs(fam_dir, exist_ok=True) - open(os.path.join(fam_dir, "zz_family.cmake"), "w", encoding="utf-8").write(family) + fam_path = os.path.join(d, family_rel) + os.makedirs(os.path.dirname(fam_path), exist_ok=True) + open(fam_path, "w", encoding="utf-8").write(family) if baseline == "auto": # The direction-A fixtures below are not about direction B, so the baseline records # whatever they drift - a direction-B case must then say what its baseline is. @@ -600,6 +607,18 @@ def case(what, ci, cmake, want, baseline="auto", family=None): case("an add_test in a glob-included family file is still read", B_CI, GLOB_CMAKE, 1, baseline="", family="add_executable(glob_fam g.cpp)\nadd_test(NAME glob_suite COMMAND glob_fam)\n") + # engine#1979: the shape main has, a prefix before the star (`cmake/tests-*.cmake`). With the GLOB pattern that + # wanted `/*.cmake` this arm returned 0: the file was configured by CMake and read by nobody here. + PREFIX_GLOB_CMAKE = GLOB_CMAKE.replace("cmake/tests/*.cmake", "cmake/tests-*.cmake") + case("an add_test in a file included by a cmake/tests-*.cmake glob is still read", + B_CI, PREFIX_GLOB_CMAKE, 1, baseline="", + family="add_executable(glob_fam g.cpp)\nadd_test(NAME glob_suite COMMAND glob_fam)\n", + family_rel="cmake/tests-zz.cmake") + # ... and a file the glob does not match (one directory down) is not configured, so it is not read. + case("a file the cmake/tests-*.cmake glob does not match is not read", + B_CI, PREFIX_GLOB_CMAKE, 0, baseline="", + family="add_executable(glob_fam g.cpp)\nadd_test(NAME glob_suite COMMAND glob_fam)\n", + family_rel="cmake/tests-zz/inner.cmake") # ... and a baseline name nothing names any more is a FAILURE, not "progress": the two reasons a # count can fall are "now built" (good) and "no longer seen" (the reader narrowed). case("a baseline name no add_test names any more fails instead of reading as progress",