Enable unity build repo-wide via WSL_UNITY_BATCH_SIZE - #41441
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables repo-wide CMake unity (jumbo) builds gated by the existing WSL_UNITY_BATCH_SIZE option, replacing prior per-target opt-ins. It also adds targeted unity opt-outs and resolves a unity-induced type-name collision in wslcsession.
Changes:
- Enable unity build globally (for targets created after dependency
FetchContent), controlled byWSL_UNITY_BATCH_SIZE. - Add opt-outs for generated MIDL proxy/stub targets and specific per-source-flag/generated sources that are incompatible with unity builds.
- Fix
WSLCContainer.cppreturn-type lookup ambiguity by removing a globalusingand fully qualifying affected return types; add delay-load linker settings consistently across several Windows targets.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Enables global unity build configuration when WSL_UNITY_BATCH_SIZE > 0, after FetchContent dependencies are instantiated. |
| src/windows/common/CMakeLists.txt | Propagates common’s link dependencies via PUBLIC and excludes WSLCUserSettings.cpp from unity build due to per-source settings. |
| src/windows/libwsl/CMakeLists.txt | Adds delay-load support (delayimp + /DELAYLOAD) for MSI/WinTrust. |
| src/windows/service/stub/CMakeLists.txt | Disables unity build for the MIDL proxy/stub target to avoid generated-symbol collisions. |
| src/windows/wsldevicehoststub/CMakeLists.txt | Disables unity build for the MIDL proxy/stub target to avoid generated-symbol collisions. |
| src/windows/wslinstaller/stub/CMakeLists.txt | Disables unity build for the MIDL proxy/stub target to avoid generated-symbol collisions. |
| src/windows/wslc/CMakeLists.txt | Removes per-target unity opt-in; adds delay-load linker settings to wslc.exe. |
| src/windows/WslcSDK/winrt/CMakeLists.txt | Excludes generated module.g.cpp from unity build due to COM/C++/WinRT header interaction. |
| src/windows/wslcsession/CMakeLists.txt | Adds delay-load linker settings to wslcsession. |
| src/windows/wslcsession/WSLCContainer.cpp | Resolves unity-build-induced WSLCPortMapping name collision by removing global using and fully qualifying return types. |
| src/windows/wslhost/CMakeLists.txt | Adds delay-load linker settings to wslhost. |
| src/windows/wslrelay/CMakeLists.txt | Adds delay-load linker settings to wslrelay. |
| test/windows/CMakeLists.txt | Removes per-target unity opt-in for wsltests (now governed by global setting). |
| test/windows/wslc/CMakeLists.txt | Excludes WSLC test sources from unity build because they rely on per-source PCH compile flags. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
a8b8ff1 to
ab9c4c7
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/windows/wslc/CMakeLists.txt:22
SKIP_UNITY_BUILD_INCLUSION ONis being applied to${WSLC_TEST_SOURCES}(all *.cpp under test/windows/wslc), which opts the entire WSLC test suite out of unity builds. That undermines the repo-wide unity build goal; if only specific files need special per-source flags, restrict the skip to just those files (or rely ontarget_precompile_headers(wsltests REUSE_FROM common)at the parent level).
set_source_files_properties(${WSLC_TEST_SOURCES}
PROPERTIES
COMPILE_FLAGS "/Yuprecomp.h"
SKIP_UNITY_BUILD_INCLUSION ON
)
…build # Conflicts: # src/windows/common/string.hpp # src/windows/wslc/tasks/ImageTasks.cpp # src/windows/wslc/tasks/VolumeTasks.cpp
ranm-msft
left a comment
There was a problem hiding this comment.
The unity wiring itself looks right to me - setting the CMAKE_UNITY_BUILD* variables after FetchContent_MakeAvailable is the correct place, and the collision fixes (qualifying socket::CancellableAccept / string::IsEqual, ::socket, hoisting the duplicate InitializeFileOffset and the containerd timeouts into headers) all read as genuine merged-TU problems rather than workarounds. Two questions before I sign off.
1. The unconditional link changes aren't covered by the description. common gains a PUBLIC link line for HCS/MSI/VirtDisk/configfile, and five targets gain delayimp.lib + /DELAYLOAD:msi.dll /DELAYLOAD:WINTRUST.dll. Neither is gated on WSL_UNITY_BATCH_SIZE.
I assume this is unity fallout: batching common coarsens static-library object granularity, so pulling one symbol now drags in MSI/WinTrust references that previously lived in unselected archive members, and the delay-load keeps those from becoming eager startup imports. If that's the reasoning, could it go in the description? As written it reads as unrelated scope.
Worth caveating the "local developer builds are unchanged" line too - delay-loading WINTRUST.dll moves a load-time failure to a first-call exception in the default (non-unity) build as well.
2. Coverage inverts. .pipelines/build-job.yml passes -DWSL_UNITY_BATCH_SIZE=4, so after this PR every first-party target is built by CI only in unity mode, while the checked-in default of 0 that local devs get has no CI leg. Previously most targets were still compiled per-file in CI. Is a non-unity leg worth keeping, or is manual validation of the default path considered enough?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
ranm-msft
left a comment
There was a problem hiding this comment.
Verified the cleanup - this is a genuine no-op. wsltests is created in test/windows/CMakeLists.txt, and source file properties are only visible to targets created in the same directory scope, so the /Yuprecomp.h and SKIP_UNITY_BUILD_INCLUSION properties set in the child directory never reached it. Calling target_sources() from the child doesn't change that. PCH is unaffected either way, since target_precompile_headers(wsltests REUSE_FROM common) is target-level. Worth noting for future readers that this doesn't newly opt those files into batching - they were already eligible. If per-source flags are ever genuinely needed there, set_source_files_properties(... TARGET_DIRECTORY wsltests ...) is the escape hatch.
Thanks for the description rewrite on the link changes - the archive-member granularity explanation plus the dumpbin /imports check fully answers my first point.
Two small follow-ups:
-
The "Opt-outs" bullet still lists "the
/Yuprecomp.hsource undertest/windows/wslc" as markedSKIP_UNITY_BUILD_INCLUSION. That half is stale as of this commit. -
On the non-unity leg: there is a counterexample in the other direction. A
.cppmissing a direct include can compile inside a batch because an earlier neighbour in the same translation unit already included the header, then fail when compiled on its own;usingdirectives and macro state can leak the same way. So unity success doesn't imply non-unity success, and the checked-in default of0is the configuration CI never builds. Happy to treat a second leg as a cost tradeoff - just flagging that the failure direction does exist.
Approving - nothing here blocks.
|
There was a problem hiding this comment.
🔵 Needs a closer look
common now links configfile before the configfile target is defined (subdir order), which can break targeted/incremental builds due to missing CMake target dependencies.
Review details
Suppressed comments (1)
src/windows/common/CMakeLists.txt:165
commonis now linked PUBLIC toconfigfile, butconfigfileis added as a subproject aftersrc/windows/commonin the rootCMakeLists.txt(seeadd_subdirectory(src/windows/common)beforeadd_subdirectory(src/shared/configfile)). In that ordering,configfilewill be treated as a plain library name rather than a CMake target, so consumers may not get a proper target dependency and targeted builds (e.g., building onlywslcsession) can fail becauseconfigfile.libisn't built first.
# common calls into HCS, HNS, the IP helper API, the VHD APIs, MSI, WinTrust and
# the config file parser, so anything linking common needs those libraries too.
# PUBLIC propagates them instead of making each consumer restate them.
target_link_libraries(common PUBLIC ${HCS_LINK_LIBRARIES} ${MSI_LINK_LIBRARIES} VirtDisk.lib configfile)
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary of the Pull Request
Enables CMake unity (jumbo) builds across the entire repo, driven by the existing
WSL_UNITY_BATCH_SIZEknob. Previously only two targets (wslclibandwsltests) opted in individually; this replaces those per-target opt-ins with a single global setting and fixes the fallout that surfaced once every target was batched.The checked-in default remains
0(unity build disabled), so local developer builds are unchanged unless the knob is set explicitly.PR Checklist
Detailed Description of the Pull Request / Additional comments
Enabling unity build
The root
CMakeLists.txtnow setsCMAKE_UNITY_BUILD,CMAKE_UNITY_BUILD_MODEandCMAKE_UNITY_BUILD_BATCH_SIZEwhenWSL_UNITY_BATCH_SIZEis greater than zero. The block is placed after theFetchContent_MakeAvailablecalls, since these variables only initialize theUNITY_BUILDproperty of targets created afterwards — this keeps GSL, nlohmann, yaml-cpp and boost building exactly as they do today.The per-target opt-in blocks in
src/windows/wslc/CMakeLists.txtandtest/windows/CMakeLists.txtare removed in favor of the global setting.test/windows/wslc/CMakeLists.txtalso carried aset_source_files_propertiescall applying/Yuprecomp.handSKIP_UNITY_BUILD_INCLUSIONto the WSLC test sources. Both were no-ops: source file properties are scoped to the directory that sets them, andwsltestsis defined in the parent directory, so neither ever reached the target. Those sources have therefore always been batched along with the rest ofwsltests, and removing the call changes no build behavior — the precompiled header comes fromtarget_precompile_headers(wsltests REUSE_FROM common).Opt-outs
Three categories of source needed to stay out of the batched translation units:
wslserviceproxystub,wsldevicehostproxystub,wslinstallerproxystub) are markedUNITY_BUILD OFF. Each generated_p.cdeclares its ownMIDL_TYPE_FORMAT_STRINGstruct with a different size, so merging them produces redefinition errors.WSLCUserSettings.cpp, which needs a yaml-cpp include directory andYAML_CPP_STATIC_DEFINE— are markedSKIP_UNITY_BUILD_INCLUSION. CMake does not exclude these automatically: the original source is marked excluded from build and its flags are silently dropped.module.g.cppin the C++/WinRT projection is markedSKIP_UNITY_BUILD_INCLUSION. It mixes classic COM and C++/WinRT headers, so sharing a translation unit with a projection source that hasusing namespace winrt::Windows::Foundation;at global scope makesIUnknownambiguous. The remaining 22 projection sources still batch normally.Name collision in
wslcsessionOne genuine unity-build conflict was found.
WSLCContainer.cpphad a globalusing wsl::windows::service::wslc::WSLCPortMapping;that collided with the IDL-generated global::WSLCPortMappingfromwslc.h. OnceWSLCContainer.cppandWSLCVirtualMachine.cpplanded in the same translation unit,::WSLCPortMappingsilently changed meaning andVMPortMapping::FromWSLCPortMappingno longer matched its header declaration.The fix removes the global using-declaration and fully qualifies the two affected return types. Only return types needed qualification: for out-of-line member definitions the return type is looked up at namespace scope, whereas parameter types are looked up in class scope.
Validation Steps Performed
cmake . -DWSL_UNITY_BATCH_SIZE=4followed bycmake --build . --clean-first -- -m) on x64 Debug, matching the batch size used in CI. Zero errors; all 18 binaries produced, includingwsl.exe,wslservice.exe,wslc.exe,wslcsdk.dll,wslinstaller.exeandwsltests.dll.WSL_UNITY_BATCH_SIZEleft at its default of0, no unity sources are generated and every target compiles per-file as before.Why the link changes are here
Enabling unity batching changes static-library granularity, which pulls in link
dependencies that per-file builds never selected.
commonis a static library, so at the default ofWSL_UNITY_BATCH_SIZE=0the linkerpicks archive members one source file at a time. Only
install.cpp,WslClient.cppandwslutil.cppcall MSI, and onlyinstall.cppcalls WinTrust, so targets such aswslrelayandwslhostnever pull those members in —dumpbin /importson a defaultbuild shows no
msi.dllorWINTRUST.dllimports for them.Batching merges those files into an object that also holds members these targets do need,
so the references come along:
commongains aPUBLIClink line for HCS, MSI, VirtDisk andconfigfile.COMMON_LINK_LIBRARIEScarries neither MSI nor HCS, so propagating fromcommoniswhat keeps every consumer from restating them.
delayimp.liband/DELAYLOAD:msi.dll /DELAYLOAD:WINTRUST.dll, sothe newly reachable references do not become eager startup imports of DLLs those
binaries never call into. This matches
wsl,wslgandwslcsdk, which already usethe same pair.
Neither change is gated on
WSL_UNITY_BATCH_SIZEbecause the delay-load is inert atbatch 0 — there are no imports from those DLLs to defer — and the
PUBLICline onlyrestates dependencies
commongenuinely has.