COMP: Propagate CMAKE_VS_GLOBALS to external projects - #1446
Conversation
There was a problem hiding this comment.
Thank you, the change looks mostly good, but the _ep_arguments"fix" is unrelated and probably not necessary. It is simpler to skip it, but if you want to keep it, then move it to a separate commit, fix the commit comment, and do the same in line 324, too.
Details from Claude:
Claim REFUTED: "silently corrupts CMAKE_JOB_POOLS"
I tested the old form (CMAKE_CACHE_ARGS -DFOO:STRING=a followed by a stray b) in the same experiment. ExternalProject's initial-cache writer (_ep_command_line_to_initial_cache) deliberately re-joins non--D stragglers onto the previous entry — its source even comments "Assume this is a list to append to the last var". The generated cache was:
set(FOO "a;b" CACHE STRING "Initial cache" FORCE) # old style — NOT corrupted
set(BAR "x;y" CACHE STRING "Initial cache" FORCE) # new style — identical resultSo on every CMake version CTK supports, the old code already delivered CMAKE_JOB_POOLS=compile=4;link=2 correctly, and the PR's before/after example is misleading (the ^^ in the "after" line is also replaced back to ; before the inner project sees it). The join change is a contract-compliance/robustness cleanup — relying on documented LIST_SEPARATOR behavior instead of an undocumented re-assembly heuristic (which would misbehave if a list element started with -D or matched an ExternalProject keyword) — not a bug fix. Worth asking the author to correct the commit message/description so the git history doesn't record a false rationale.
The same unquoted-expansion pattern still exists for mark_as_superbuild variables via _sb_cmakevar_to_cmakearg (line 324) — if consistency is the goal, that path relies on the same heuristic. Fine to leave; just noting the PR doesn't (and needn't) fix it.
CTK is a nested superbuild: the outer project only configures the inner CTK build and the third-party projects, so a CMake option that is not forwarded by _sb_get_external_project_arguments never reaches the projects that actually compile code. CMAKE_VS_GLOBALS was missing from that list, so a parent project could not set UseMultiToolTask and EnforceProcessCountAcrossBuilds for them and MSBuild compiled one file at a time. Ninja builds are unaffected, since CMAKE_JOB_POOLS is already propagated.
811db72 to
0206da1
Compare
|
You're right, and thanks for checking it rather than taking my word for it. I confirmed it too: set(CMAKE_JOB_POOLS "compile=4;link=2" CACHE STRING "Initial cache" FORCE)My "silently corrupts" claim was wrong. I had looked at the generated argument list, seen the Since the propagation works either way, I dropped the |
Disclaimer: We (MITK) are currently catching up on the latest CTK master branch
since we forked for Qt 6 back in 2023. We found a few bugs that are not caught by
your CI and mostly affect external users of CTK.
CTK is a nested superbuild: the outer project only configures the inner
CTK-build and the third-party projects, so any CMake option not explicitly
forwarded is lost.
_sb_get_external_project_argumentspropagatesCMAKE_EXPORT_COMPILE_COMMANDS,CMAKE_FIND_*_PACKAGE_REGISTRYandCMAKE_JOB_POOL*, but notCMAKE_VS_GLOBALS.A parent project that sets
UseMultiToolTask/EnforceProcessCountAcrossBuildstherefore never reaches the projects that actually compile code, and MSBuild
builds CTK with a single
cl.exe. Ninja is unaffected, sinceCMAKE_JOB_POOLSis propagated, so this is MSBuild-specific.
The same commit joins all automatically propagated options with
EP_LIST_SEPARATOR. Multi-value options were expanded into oneExternalProject argument per value, which silently corrupts
CMAKE_JOB_POOLS:Related: #1349 addresses the same slow-build symptom by adding
CTK_MSVC_ENABLE_MPwith/MP /FS. The two are complementary — this one letsa parent project's existing scheduling choice through, rather than imposing
/MP, which over-subscribes when combined with MultiToolTask.