Add CI guard rejecting CMAKE_BINARY_DIR in first-party CMake files - #12793
Conversation
Slang's first-party CMake must use the project-scoped slang_BINARY_DIR rather than CMAKE_BINARY_DIR, so that build outputs land under Slang's own build tree when Slang is consumed via add_subdirectory. No existing CI job builds Slang as a subproject, so a reintroduced CMAKE_BINARY_DIR is otherwise invisible. Add extras/check-no-cmake-binary-dir.sh (git grep over tracked files, so vendored submodule CMake is excluded) and a path-filtered, non-required workflow that runs it on pull_request. Depends on shader-slang#12570, which removes the existing 22 first-party occurrences. Closes shader-slang#12790.
|
Automated notice (PR board sync) — do not reply to this comment. Auto-assigned @jvepsalainen-nv as shepherd for this Bot PR. FYI for maintainers: committer signal on the changed files is highest for jkiviluoto-nv among collaborators other than the assignee. They were not auto-requested; a human may optionally add them as a reviewer. |
|
@nv-slang-bot review this PR |
|
👋 @jvepsalainen-nv — thanks for merging #12570 and un-drafting this. Review complete. Review checklist:
Outcome — WOULD_APPROVE @
No blocking findings. This is a maintainer-owned merge decision — flagging that the change touches (Last updated: 2026-08-28T08:50Z)
|
2 participants
Motivation
Slang's first-party CMake should use the project-scoped
slang_BINARY_DIR(equivalent toPROJECT_BINARY_DIRwithin Slang's scope) for its own build outputs, notCMAKE_BINARY_DIR.CMAKE_BINARY_DIRresolves to the top-level build directory; when Slang is consumed viaadd_subdirectory(as a submodule/subproject of a larger build), that is the superproject's build tree, so Slang's outputs land outside its own tree and can collide with the consumer's (see #5871, #5896).PR #12570 migrated the existing first-party uses to
slang_BINARY_DIR. But nothing keeps the fix in place: no CI job builds Slang as a subproject, so a reintroduced${CMAKE_BINARY_DIR}is invisible to every existing check. This PR adds a cheap textual guard so the invariant is enforced at PR time.This is the "cheap grep guard" that #12790 requests. (The heavier
add_subdirectoryconsumer smoke test the issue also mentions is intentionally left as a separate follow-up, per the reporter.)Proposed solution
A lightweight lint job (intended to be configured as a non-required check) that asserts zero occurrences of
CMAKE_BINARY_DIRin first-party CMake files, with a message pointing at theslang_BINARY_DIRremedy.The check uses
git grep(tracked files only). This is load-bearing:git grepdoes not descend into submodule working trees, so vendored third-party CMake (mimalloc, glm, spirv-tools, …) that legitimately usesCMAKE_BINARY_DIRis excluded automatically — whileexternal/CMakeLists.txt, which is first-party (tracked, not a submodule), is still covered. A plaingrep -r/rgover a checkout would false-positive on the vendored trees, and--exclude-dir=externalwould wrongly skip a first-party file. Scope is therefore the tracked-vs-submodule boundary, not theexternal/path prefix.The match is whole-word (
git grep -w), so a larger identifier such asMY_CMAKE_BINARY_DIRwould not be flagged. (CMAKE_CURRENT_BINARY_DIR— the subproject-safe variable used legitimately elsewhere in the tree — is a different string and is not matched.)Change summary
extras/check-no-cmake-binary-dir.sh— new. Runsgit grep -wn 'CMAKE_BINARY_DIR' -- '*.cmake' '*.cmake.in' '*CMakeLists.txt', captures the exit status (0 = matches, 1 = clean, >1 = a real grep error that fails the guard loudly), and on any match exits non-zero listing the offending lines plus theslang_BINARY_DIRremedy..github/workflows/check-cmake-binary-dir.yml— new. Modeled oncheck-submodules.yml: path-filteredpull_request,if: draft != true,permissions: contents: read,submodules: false, intended as a non-required check; a lightweight job..github/workflows/README.md— adds the new workflow to the PR-gates table.Concepts and vocabulary
CMAKE_BINARY_DIR— the top-level (superproject) build directory. Differs from Slang's own build dir only when Slang is included viaadd_subdirectory.slang_BINARY_DIR/PROJECT_BINARY_DIR— CMake's per-project()binary dir; always points at Slang's own build directory regardless of how Slang is included. The correct variable for Slang's outputs.external/CMakeLists.txt), as opposed to CMake inside submodule working trees, which are third-party and out of scope.Process report
add_subdirectoryconsumer build, which no PR-time CI job performs (cmake-options.yml'sfind_packageconsumer test isworkflow_dispatch/weekly-cron only). Agit grepassertion is essentially free and catches the reintroduction at the source line. The full consumer smoke test covers a broader class but is heavier/brittle and is deliberately deferred to a separate follow-up, as the issue requests.git grepand-w, verified: atorigin/master(c1cffad)git grep -wn 'CMAKE_BINARY_DIR' -- '*.cmake' '*.cmake.in' '*CMakeLists.txt'finds 22 lines across 8 files; a rawrgover a populated checkout finds many more, all underbuild/_deps/(untracked artifacts) andexternal/<submodule>/working trees — exactly what must be excluded.!external/) is dictated by the submodule boundary and stated in the script header.⚠ Sequencing: depends on #12570 — held as draft until it merges
Today's
masterstill has the 22 first-party occurrences; PR #12570 (OPEN, non-draft, head1754931a0c, branchfeature/fix-build-output-directory) is what makes the count zero — its changed-file set is identical to the 8 files with hits. If this guard ran as a green-required check against today's master it would fail on those 22 lines. This branch fails the guard today, by design, until #12570 lands.This PR is therefore held as a draft: the workflow's
if: github.event.pull_request.draft != truemeans the check does not run while the PR is draft, so there is no false red. Do not mark ready / merge until #12570 has landed on master; after it lands, a rebase makes the check pass (verified: the script exits 0 against #12570's head1754931a0c, which has zero matches).Closes #12790.