Skip to content

COMP: Propagate CMAKE_VS_GLOBALS to external projects - #1446

Open
kislinsk wants to merge 1 commit into
commontk:masterfrom
MITK:propagate-cmake-vs-globals
Open

COMP: Propagate CMAKE_VS_GLOBALS to external projects#1446
kislinsk wants to merge 1 commit into
commontk:masterfrom
MITK:propagate-cmake-vs-globals

Conversation

@kislinsk

Copy link
Copy Markdown
Contributor

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_arguments propagates
CMAKE_EXPORT_COMPILE_COMMANDS, CMAKE_FIND_*_PACKAGE_REGISTRY and
CMAKE_JOB_POOL*, but not CMAKE_VS_GLOBALS.

A parent project that sets UseMultiToolTask / EnforceProcessCountAcrossBuilds
therefore never reaches the projects that actually compile code, and MSBuild
builds CTK with a single cl.exe. Ninja is unaffected, since CMAKE_JOB_POOLS
is propagated, so this is MSBuild-specific.

The same commit joins all automatically propagated options with
EP_LIST_SEPARATOR. Multi-value options were expanded into one
ExternalProject argument per value, which silently corrupts CMAKE_JOB_POOLS:

before:  -DCMAKE_JOB_POOLS:STRING=compile=4   +   stray argument "link=2"
after:   -DCMAKE_JOB_POOLS:STRING=compile=4^^link=2

Related: #1349 addresses the same slow-build symptom by adding
CTK_MSVC_ENABLE_MP with /MP /FS. The two are complementary — this one lets
a parent project's existing scheduling choice through, rather than imposing
/MP, which over-subscribes when combined with MultiToolTask.

@lassoan lassoan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 result

So 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.
@kislinsk
kislinsk force-pushed the propagate-cmake-vs-globals branch from 811db72 to 0206da1 Compare July 25, 2026 20:01
@kislinsk

Copy link
Copy Markdown
Contributor Author

You're right, and thanks for checking it rather than taking my word for it. I confirmed it too:
_ep_command_line_to_initial_cache appends any argument that doesn't start with -D to the
previous variable, so both forms produce the same entry:

    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
value split across two entries, and inferred the outcome instead of checking what
ExternalProject does with them.

Since the propagation works either way, I dropped the _ep_arguments change rather than
splitting it off. The PR is now just the one line adding CMAKE_VS_GLOBALS to the propagated
options, and the commit message no longer mentions CMAKE_JOB_POOLS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants