dasVulkan is off by default on Apple builds - #4075
Merged
Merged
Conversation
On a Mac, Vulkan is MoltenVK: its 32 KB workgroup memory cap declines the kernel classes the module's consumers serve, and what it proves does not carry to a native Vulkan device, so every Mac build paid the module's build time and every dasLLAMA test child its engine-module compile for two permanently red suite files. The root option DAS_VULKAN_DISABLED now defaults to ON on Apple and stays OFF elsewhere; -DDAS_VULKAN_DISABLED=OFF opts in, which the nightly Vulkan lane's macOS job does explicitly since proving the MoltenVK loader path is its purpose. The dasVulkan README, ARCHITECTURE sec.9 and ROADMAP say macOS is an opt-in. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…sVulkan artifact Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new sweep gating currently only considers .shared_module presence in DLL builds, which can still cause compile-sweep failures in static builds where dasVulkan is disabled.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
This PR makes dasVulkan opt-in on Apple builds by defaulting DAS_VULKAN_DISABLED to ON under APPLE, while keeping non-Apple behavior unchanged. It also adjusts CI and preflight behavior/docs so Vulkan-dependent trees are only built/swept when the module is actually present.
Changes:
- Default
DAS_VULKAN_DISABLEDtoONon Apple in the rootCMakeLists.txt, preservingOFFelsewhere. - Update the nightly macOS Vulkan workflow job to explicitly opt in with
-DDAS_VULKAN_DISABLED=OFF. - Teach preflight’s compile sweep to skip Vulkan-gated trees when the dasVulkan shared artifact is absent, and document the opt-in in dasVulkan docs.
| File | Description |
|---|---|
CMakeLists.txt |
Makes dasVulkan disabled-by-default on Apple via an APPLE-conditional default. |
.github/workflows/nightly_vulkan.yml |
Explicitly opts the macOS nightly Vulkan lane into building dasVulkan. |
utils/internal/preflight/config.das |
Adds SWEEP_NEEDS_MODULE and gates sweep roots on module artifact presence. |
utils/internal/preflight/tests/test_changed_set.das |
Extends sweep-exclusion tests to cover module-gated trees. |
utils/internal/preflight/README.md |
Documents the new split between SWEEP_EXCLUDED and SWEEP_NEEDS_MODULE. |
modules/dasVulkan/README.md |
Clarifies macOS usage as opt-in via -DDAS_VULKAN_DISABLED=OFF. |
modules/dasVulkan/ARCHITECTURE.md |
Updates architecture docs to reflect Apple opt-in and rationale. |
modules/dasVulkan/ROADMAP.md |
Updates macOS section to reflect opt-in behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…atic build asks the binary Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Behavior change: an Apple build leaves dasVulkan out unless configured with
-DDAS_VULKAN_DISABLED=OFF; a Mac build directory that had the module keeps it until the cached option is unset (cmake -B build -UDAS_VULKAN_DISABLED).Why. On a Mac, Vulkan is MoltenVK, whose 32 KB workgroup memory cap declines the kernel classes the module's consumers serve, and what it proves does not carry to a native Vulkan device. Every Mac build paid the module's build time and every dasLLAMA test child its engine-module compile for two permanently red suite files.
What changes.
DAS_VULKAN_DISABLEDdefaults toONon Apple and staysOFFelsewhere; the flag remains explicit for anyone who wants MoltenVK.-DDAS_VULKAN_DISABLED=OFF, since proving the MoltenVK loader path is its purpose.examples/vulkan/,examples/vulkan_imgui_cube/,examples/gltf/mesh_viewer_vulkan/,modules/dasVulkan/) while the module's shared artifact is absent, and sweeps them as before while it exists (SWEEP_NEEDS_MODULEinutils/internal/preflight/config.das).Observable behavior.
require ?vulkan), andrun.das -- --suite model-freeon the M5 Max goes from two red files to none.Where to look. The option block in the root
CMakeLists.txt, and thebuild_macosjob of.github/workflows/nightly_vulkan.yml.Validation, claims, ledger
Validation
DAS_VULKAN_DISABLED:BOOL=ON, the tree builds, and the two formerly red files readtest_vulkan_tier36 passed andtest_vulkan_dec_tail2 passed, 3 skipped (34/2 and 2/3 before, the MoltenVK cap declining the class rails). The full preflight ran once: the fast tier was red on the compile sweep (three Vulkan example roots with no module to require), which the sweep rule fixes; every lane the red tier skipped then ran alone and green - docs (vulkan2rst skips without the module), tests-cpp, tests-interp, tests-jit, tests-aot, utils-tests - and the sweep gate reads 726 roots compiling with the four Vulkan trees held back.Not done
OFFand its builtdasModuleVulkan.shared_moduleuntil the option is unset and the artifact removed; CMake does not delete either.