From 36a52af9fe81c0b1c7c168188b66b0ef600ff641 Mon Sep 17 00:00:00 2001 From: David Date: Sun, 11 Oct 2026 11:05:23 -0700 Subject: [PATCH] fix(lint): skip build trees and git-ignored files in check_displayxr_app After a local Gradle build, AGP's externalNativeBuild output (.cxx/) holds the FetchContent'd displayxr-common sources under _deps/, including its tests/. The app linter walked into it and reported false INV-3.1 errors against those test files (cube_handle_vk_android, 2026-10-11). All ten directory walks now share one is_excluded() predicate: the fixed EXCLUDE_DIRS list (extended with .cxx, .externalNativeBuild, .gradle, _deps) plus untracked git-ignored paths from one cached `git ls-files --others --ignored --exclude-standard --directory` per root. Tracked files are never hidden; outside a git work tree (or with no git) the fixed list is the only filter. New hermetic test scripts/tests/test_check_displayxr_app_skip_build_trees.py (wired into lint.yml) covers each build-tree name, a build tree not masking a real error, the vk walker, git-ignored paths, and a tracked file in an ignored dir. Mutation-checked against both halves of the filter. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/lint.yml | 8 + scripts/check_displayxr_app.py | 63 ++++++- ...st_check_displayxr_app_skip_build_trees.py | 155 ++++++++++++++++++ 3 files changed, 217 insertions(+), 9 deletions(-) create mode 100644 scripts/tests/test_check_displayxr_app_skip_build_trees.py diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index d72141e893..e684bef2d0 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -253,3 +253,11 @@ jobs: # submit -> wait loop parks Adreno at ~1/3 clock), and that one-off # uploads, VkFrameRing-style rings and the exemption comment stay quiet. run: python3 scripts/tests/test_check_displayxr_app_vk_frame.py + + - name: App-linter build-tree exclusion unit tests + # Hermetic (synthetic app trees + a throwaway git repo). Pins that every + # directory walk skips build output (.cxx/, .externalNativeBuild/, .gradle/, + # _deps/, build/) and git-ignored untracked files: a local Gradle build's + # .cxx/ holds FetchContent'd displayxr-common tests/ that tripped false + # INV-3.1 errors. + run: python3 scripts/tests/test_check_displayxr_app_skip_build_trees.py diff --git a/scripts/check_displayxr_app.py b/scripts/check_displayxr_app.py index b83704b9db..9703968748 100755 --- a/scripts/check_displayxr_app.py +++ b/scripts/check_displayxr_app.py @@ -33,6 +33,7 @@ import argparse import json import re +import subprocess import sys import xml.etree.ElementTree as ET from pathlib import Path @@ -40,9 +41,13 @@ RULES_DOC = "docs/guides/displayxr-app-rules.md" # Directories never linted: shared reference code, vendored headers, build output. +# The Android entries matter after a local Gradle build: AGP's externalNativeBuild +# tree (.cxx/, older AGP .externalNativeBuild/) holds FetchContent'd displayxr-common +# sources under _deps/, incl. its tests/, which tripped false INV-3.1 errors. EXCLUDE_DIRS = { "build", ".git", "third_party", "openxr_includes", "common", "_package", "__pycache__", "node_modules", ".vs", "out", ".claude", + ".cxx", ".externalNativeBuild", ".gradle", "_deps", } SOURCE_EXTS = {".c", ".cc", ".cpp", ".cxx", ".h", ".hh", ".hpp", ".m", ".mm"} # JVM sources are linted for the Android rules only (§11) — the C/C++ checks @@ -250,11 +255,51 @@ def strip_comments(text: str) -> str: return re.sub(r"//[^\n]*", "", text) +_GIT_IGNORED_CACHE: dict = {} + + +def git_ignored_prefixes(root: Path) -> tuple: + """Untracked-and-ignored paths under `root`, relative to it (directories end + in "/"), per the enclosing git repo's ignore rules. One `git ls-files` call + per root, cached. Empty when `root` is not in a git work tree or git is + unavailable — EXCLUDE_DIRS is then the only filter. Tracked files are never + reported, so a committed file inside an ignored directory is still linted.""" + key = str(root) + if key not in _GIT_IGNORED_CACHE: + prefixes: tuple = () + try: + out = subprocess.run( + ["git", "-C", key, "ls-files", "-z", "--others", "--ignored", + "--exclude-standard", "--directory"], + capture_output=True, timeout=30, check=True).stdout + prefixes = tuple(e for e in out.decode("utf-8", "replace").split("\0") if e) + except (OSError, subprocess.SubprocessError): + pass + _GIT_IGNORED_CACHE[key] = prefixes + return _GIT_IGNORED_CACHE[key] + + +def is_excluded(path: Path, root: Path) -> bool: + """True for files the linter must skip: anything under an EXCLUDE_DIRS + directory, or git-ignored (build trees, generated sources).""" + try: + relp = path.relative_to(root) + except ValueError: + return False + if any(part in EXCLUDE_DIRS for part in relp.parts[:-1]): + return True + ignored = git_ignored_prefixes(root) + if not ignored: + return False + posix = relp.as_posix() + return any(posix == e or (e.endswith("/") and posix.startswith(e)) for e in ignored) + + def iter_source_files(root: Path): for p in sorted(root.rglob("*")): if not p.is_file() or p.suffix.lower() not in SOURCE_EXTS: continue - if any(part in EXCLUDE_DIRS for part in p.relative_to(root).parts[:-1]): + if is_excluded(p, root): continue yield p @@ -567,7 +612,7 @@ def check_mcp_pairing(root: Path, findings: list): # Manifest ids. manifest_ids = {} for m in root.rglob("*.displayxr.json"): - if any(part in EXCLUDE_DIRS for part in m.relative_to(root).parts[:-1]): + if is_excluded(m, root): continue try: data = json.loads(m.read_text(encoding="utf-8")) @@ -590,7 +635,7 @@ def check_mcp_pairing(root: Path, findings: list): for p in sorted(root.rglob("*")): if p.suffix.lower() not in SOURCE_EXTS or not p.is_file(): continue - if any(part in EXCLUDE_DIRS for part in p.relative_to(root).parts[:-1]): + if is_excluded(p, root): continue try: text = p.read_text(encoding="utf-8", errors="replace") @@ -635,7 +680,7 @@ def check_manual_tracking_event(root: Path, findings: list): for p in sorted(root.rglob("*")): if p.suffix.lower() not in SOURCE_EXTS or not p.is_file(): continue - if any(part in EXCLUDE_DIRS for part in p.relative_to(root).parts[:-1]): + if is_excluded(p, root): continue try: text = strip_comments(p.read_text(encoding="utf-8", errors="replace")) @@ -749,7 +794,7 @@ def check_views_change_realloc(root: Path, findings: list): for p in sorted(root.rglob("*")): if p.suffix.lower() not in SOURCE_EXTS or not p.is_file(): continue - if any(part in EXCLUDE_DIRS for part in p.relative_to(root).parts[:-1]): + if is_excluded(p, root): continue try: text = strip_comments(p.read_text(encoding="utf-8", errors="replace")) @@ -929,7 +974,7 @@ def check_vk_frame_discipline(root: Path, findings: list): for path in sorted(root.rglob("*")): if not path.is_file() or path.suffix.lower() not in VK_DISCIPLINE_EXTS: continue - if any(part in EXCLUDE_DIRS for part in path.relative_to(root).parts[:-1]): + if is_excluded(path, root): continue try: raw = path.read_text(encoding="utf-8", errors="replace") @@ -1119,7 +1164,7 @@ def iter_android_sources(root: Path): for p in sorted(root.rglob("*")): if not p.is_file() or p.suffix.lower() not in exts: continue - if any(part in EXCLUDE_DIRS for part in p.relative_to(root).parts[:-1]): + if is_excluded(p, root): continue yield p @@ -1279,7 +1324,7 @@ def check_android_manifest(mpath: Path, root: Path, findings: list) -> None: def check_android(root: Path, findings: list) -> None: """§11 — Android pixel-exactness. No-ops entirely for non-Android apps.""" manifests = [m for m in sorted(root.rglob("AndroidManifest.xml")) - if not any(part in EXCLUDE_DIRS for part in m.relative_to(root).parts[:-1])] + if not is_excluded(m, root)] if not manifests: return # Not an Android app — every check below is Android-only. @@ -1332,7 +1377,7 @@ def check_android(root: Path, findings: list) -> None: def scan_manifests(root: Path, findings: list): manifests = [p for p in root.rglob("*.displayxr.json") - if not any(part in EXCLUDE_DIRS for part in p.relative_to(root).parts[:-1])] + if not is_excluded(p, root)] if not manifests: findings.append(Finding(WARN, "INV-9.1", str(root), 1, "No *.displayxr.json found — the app won't appear in any workspace launcher.", diff --git a/scripts/tests/test_check_displayxr_app_skip_build_trees.py b/scripts/tests/test_check_displayxr_app_skip_build_trees.py new file mode 100644 index 0000000000..b50f255733 --- /dev/null +++ b/scripts/tests/test_check_displayxr_app_skip_build_trees.py @@ -0,0 +1,155 @@ +#!/usr/bin/env python3 +"""Unit tests for the build-tree / git-ignore exclusion in +scripts/check_displayxr_app.py. + +After a local Gradle build, AGP's externalNativeBuild output (.cxx/) holds the +FetchContent'd displayxr-common sources under _deps/, incl. its tests/. The +linter used to walk into it and report false INV-3.1 errors against those test +files (seen on test_apps/handle/cube_handle_vk_android, 2026-10-11). Every +directory walk now goes through is_excluded(): a fixed list of build-output +directory names, plus anything the enclosing git repo ignores. + +Hermetic: each case writes a tiny synthetic app tree to a temp dir and lints it. + + python3 scripts/tests/test_check_displayxr_app_skip_build_trees.py +""" +from __future__ import annotations + +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) +import check_displayxr_app as lint # noqa: E402 + +# Reads the active mode's view count but never opts into PRIMARY_MULTIVIEW_DXR: +# the shape INV-3.1 flags as an ERROR. +OFFENDING = """ +void pick_mode(XrSession s) +{ + uint32_t renderingModeCount = 0; + xrEnumerateDisplayRenderingModesDXR(s, 0, &renderingModeCount, nullptr); +} +""" + +# A clean N-view app source (opts in, so INV-3.1 is silent for it). +CLEAN = """ +void begin(XrSession s) +{ + XrViewConfigurationType t = XR_VIEW_CONFIGURATION_TYPE_PRIMARY_MULTIVIEW_DXR; +} +""" + +# The vk-discipline walker is a separate loop; pin it too. +VK_DRAIN = """ +void RenderFrame() +{ + vkCmdDraw(cmd, 3, 1, 0, 0); + vkQueueSubmit(q, 1, &si, VK_NULL_HANDLE); + vkQueueWaitIdle(q); +} +""" + +BUILD_TREE_PATHS = [ + ".cxx/Debug/4x5y6z/arm64-v8a/_deps/displayxr_common-src/tests/test_views.cpp", + ".externalNativeBuild/cmake/debug/arm64-v8a/offending.cpp", + ".gradle/8.7/offending.cpp", + "_deps/displayxr_common-src/tests/offending.cpp", + "build/intermediates/offending.cpp", +] + + +def write_tree(root: Path, files: dict[str, str]) -> None: + for rel, text in files.items(): + p = root / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + + +def inv31(root: Path) -> list: + findings: list = [] + lint.scan_sources(root, findings) + return [f for f in findings if f.rule == "INV-3.1"] + + +def git(root: Path, *args: str) -> None: + subprocess.run(["git", "-C", str(root), *args], check=True, capture_output=True) + + +class SkipBuildTrees(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = Path(self._tmp.name).resolve() + + def tearDown(self): + self._tmp.cleanup() + + def test_fixture_trips_inv31_when_in_source_tree(self): + # Control: the offending source IS flagged where a real app source lives, + # so the exclusion tests below are not passing vacuously. + write_tree(self.root, {"src/main/cpp/offending.cpp": OFFENDING}) + got = inv31(self.root) + self.assertEqual(len(got), 1, got) + self.assertEqual(got[0].level, lint.ERROR) + + def test_build_trees_not_scanned(self): + for path in BUILD_TREE_PATHS: + with self.subTest(path=path): + with tempfile.TemporaryDirectory() as d: + root = Path(d).resolve() + write_tree(root, {"src/main/cpp/main.cpp": "int main() { return 0; }\n", path: OFFENDING}) + self.assertEqual(inv31(root), []) + + def test_build_tree_does_not_mask_real_opt_in(self): + # A build tree's sources must not count for the app either way: a stray + # PRIMARY_MULTIVIEW_DXR in .cxx/ must not silence a real app-side error. + write_tree(self.root, { + "src/main/cpp/main.cpp": OFFENDING, + ".cxx/Debug/x/_deps/displayxr_common-src/common/views.cpp": CLEAN, + }) + self.assertEqual(len(inv31(self.root)), 1) + + def test_vk_walker_skips_build_trees(self): + write_tree(self.root, {".cxx/Debug/x/_deps/foo-src/render.cpp": VK_DRAIN}) + findings: list = [] + lint.check_vk_frame_discipline(self.root, findings) + self.assertEqual(findings, []) + + def test_no_git_repo_falls_back_to_fixed_list(self): + if shutil.which("git"): + probe = subprocess.run(["git", "-C", str(self.root), "rev-parse", "--is-inside-work-tree"], + capture_output=True) + if probe.returncode == 0: + self.skipTest("temp dir is inside a git work tree on this machine") + self.assertEqual(lint.git_ignored_prefixes(self.root), ()) + + @unittest.skipUnless(shutil.which("git"), "git not installed") + def test_git_ignored_paths_not_scanned(self): + git(self.root, "init", "-q") + write_tree(self.root, { + ".gitignore": "generated/\n*.gen.cpp\n", + # Neutral, NOT CLEAN: an opt-in anywhere silences INV-3.1 app-wide. + "src/main/cpp/main.cpp": "int main() { return 0; }\n", + "generated/nested/offending.cpp": OFFENDING, + "src/main/cpp/stub.gen.cpp": OFFENDING, + }) + self.assertEqual(inv31(self.root), []) + + @unittest.skipUnless(shutil.which("git"), "git not installed") + def test_tracked_file_in_ignored_dir_still_scanned(self): + # git-ignore only hides UNTRACKED files: a committed source inside an + # ignored directory is part of the app and must stay linted. + git(self.root, "init", "-q") + write_tree(self.root, { + ".gitignore": "generated/\n", + "generated/offending.cpp": OFFENDING, + }) + git(self.root, "add", "-f", "generated/offending.cpp") + self.assertEqual(len(inv31(self.root)), 1) + + +if __name__ == "__main__": + unittest.main(verbosity=2)