Skip to content

CMake Introduce CMake as the Primary Build System for MapWinGIS6 - #41

Open
pmeems wants to merge 1 commit into
cmake-phase-1-corefrom
cmake-phase-2-activex
Open

pmeems wants to merge 1 commit into
cmake-phase-1-corefrom
cmake-phase-2-activex

Conversation

@pmeems

@pmeems pmeems commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Phase 2 of the CMake migration plan: the ActiveX control (MapWinGIS.ocx, static ATL, apartment threaded). The MSBuild .slnx build is unchanged.

Changes

  • cmake/MwgMidl.cmake: mwg_add_midl_target() runs midl.exe through one explicit custom command for every generator (Ninja has no .idl support), with the MSBuild options /W1 /char signed /env x64|win32 /target NT60. Output goes to <build>/src/MapWinGIS.ActiveX/midl, and an INTERFACE target exposes that directory (D3). It stops configure with a clear message when midl.exe is missing, or when cl.exe is not on PATH under Ninja.
  • src/MapWinGIS.ActiveX/CMakeLists.txt: SHARED target, OUTPUT_NAME MapWinGIS, SUFFIX .ocx. The .def, .rc and .rgs are sources. The .rc gets an OBJECT_DEPENDS on the generated .tlb, plus a target dependency on the MIDL target. _ATL_APARTMENT_THREADED is set on C/C++ only, RC gets /l0x0409, and the Windows libs from the audit are linked explicitly. DEFINE_SYMBOL "" keeps CMake's MapWinGIS_ActiveX_EXPORTS out. The six shared ActiveX sources are exported as MWG_ACTIVEX_SHARED_SOURCES for the tests (D4, Phase 3).
  • cmake/MwgCompilerOptions.cmake: /TC for C sources (CMake only emits /TP for C++).
  • Parity script: deliberate compile differences are explained as well, and the generated _i.c is matched by file name. .rc entries in compile_commands.json are skipped.
  • Docs: BUILDING.md (ATL/MIDL prerequisites, per-user registration) and the OCX parity status in CMAKE-MIGRATION.md.

Verification

  • Parity script, Debug/Release × x64/Win32 (Ninja): 0 lost options, 0 warnings for Core and the OCX. dumpbin shows identical dependents, the 5 COM exports (DllCanUnloadNow, DllGetClassObject, DllRegisterServer, DllUnregisterServer, DllInstall), and the same subsystem and DLL characteristics.
  • The generated type library is byte-identical to the MSBuild one. MapWinGIS.ActiveX_i.h is identical below its header comment.
  • 3 consecutive clean -j32 Ninja builds passed (no MIDL/RC race). The VS generator builds x64 Debug/Release and x86 Debug with 0 warnings.
  • COM: regsvr32 /n /i:user → New-Object -ComObject MapWinGIS6.Control → unregister works for the x64 and the Win32 OCX (32-bit PowerShell).
  • git status is clean after the builds; nothing is generated in the source tree.

Explained differences

The MIDL include directory moved per D3. There is no vcpkg include dir, because the OCX doesn't use GDAL directly. The .res name differs. The Core import library comes from target_link_libraries instead of the project reference.

Known limitations

  • The full Verify-ActiveXActivation.ps1 run moves to Phase 3, because it needs the test executable.
  • The VS generator x86 Release build and a VS-generator parity run were not done.
  • The .slnx build was not rebuilt in this worktree; no MSBuild file changed.

Dependencies

Stacked on #40 (Phase 1, cmake-phase-1-core), which is stacked on #39.

Co-authored-by: Junie <junie@jetbrains.com>
@pmeems
pmeems added this pull request to stack #42 October 31, 2026 11:45

@Julian-MW Julian-MW left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review. Final decision is with the human reviewer.

Comment thread docs/BUILDING.md
tools** component for each architecture you build, and `midl.exe` needs `cl.exe`
on `PATH`, so configure and build from a Visual Studio toolchain (CLion) or a
Developer PowerShell. CMake 3.28 or later and Ninja are required; both ship
with Visual Studio 2026 and CLion. Run the bootstrap script first, then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Minor | Confidence: Low

Issue: This sentence still says "CMake 3.28 or later and Ninja are required", but this PR's own verification (and docs/CMAKE-MIGRATION.md) covers the Visual Studio generator as well — Ninja is not required.
Impact: A reader on CLion/VS with the VS generator skips CMake believing it cannot work here.
Fix: Reword to "CMake 3.28 or later, with either Ninja or the Visual Studio generator".

}

# Compile differences that are deliberate (docs/CMAKE-MIGRATION.md).
$explainedCompileDifferences = [ordered]@{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Minor | Confidence: Medium

Issue: $explainedCompileDifferences is applied to every target indiscriminately. For example, '^/I<vcpkg_installed>/[^/]+/include$' would also reclassify a genuinely lost vcpkg include for a target that should have it (Core, or the OCX if it ever gains a direct vcpkg dependency) as "explained", and the same applies to /DGDAL_DEBUG.
Impact: The parity script is the quality gate of the whole migration; a too-broad pattern can silently downgrade a real regression from "lost" to "explained", which is exactly where regressions would show up once targets or dependencies change.
Fix: Key the explained compile patterns per binary/target (like a $explainedCompileDifferencesByTarget map), or at least add a per-pattern target allow-list, so an option is only explained for the targets where the difference is expected.

Comment thread cmake/MwgMidl.cmake
set(env win32)
endif()

set(header "${arg_OUTPUT_DIR}/${name}_i.h")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Minor | Confidence: Medium

Issue: The MSBuild Midl task writes the type library to $(OutDir) (<TypeLibraryName>$(OutDir)MapWinGIS.ActiveX.tlb</TypeLibraryName> in MapWinGIS.ActiveX.vcxproj), i.e. next to the OCX; the CMake build writes it only to <build>/src/MapWinGIS.ActiveX/midl. This is not listed among the "explained differences" in the PR description or docs/CMAKE-MIGRATION.md.
Impact: Anything that packages, registers or redistributes the OCX together with its .tlb from the output directory (installers, Verify-ActiveXActivation.ps1-style scripts, the existing CI artifact upload of bin/Release/<platform>/) will behave differently between the two builds.
Fix: Either copy the .tlb next to the OCX after the MIDL step (or via a post-build cmake -E copy), or record this as an intentional difference in docs/CMAKE-MIGRATION.md.

Comment thread cmake/MwgMidl.cmake
set(tlb "${arg_OUTPUT_DIR}/${name}.tlb")

add_custom_command(
OUTPUT "${header}" "${iid}" "${tlb}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Minor | Confidence: Medium

Issue: MapWinGIS.ActiveX_p.c and dlldata.c are redirected into the output directory but are neither declared as OUTPUT/BYPRODUCTS nor consumed by any target. The header comment correctly notes that today's IDL declares no non-automation interfaces, but nothing guards that invariant.
Impact: The day the IDL gains a non-automation (non-oleautomation) interface, MIDL will emit a proxy file and the OCX will silently be built without the proxy/stub code and without dlldata.c, producing registration/marshalling failures that are hard to trace back to the build system.
Fix: Either add a configure-time check that fails when the IDL contains non-automation interfaces, or declare the two files as BYPRODUCTS and add a comment at the mwg_add_midl_target call site that they must be wired into consumers when they appear.

Comment thread cmake/MwgMidl.cmake

add_custom_target(${target} DEPENDS "${header}" "${iid}" "${tlb}" SOURCES "${idl}")
set_target_properties(${target} PROPERTIES FOLDER "src")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Suggestion | Confidence: Low

Issue: The build-directory path ${arg_OUTPUT_DIR} is exposed as an INTERFACE include directory without a BUILD_INTERFACE guard.
Impact: Fine for this phase, but the moment any target linking MapWinGIS.ActiveX.Interface.Generated is part of an install(EXPORT)/export() set, generation fails with "target ... not in any export set" / non-exportable interface errors. This will bite in Phase 3 when the tests link this target if an export is ever added alongside.
Fix: When exports appear, wrap it in $<BUILD_INTERFACE:...>; no change needed now — noting it so it doesn't surface as a surprise later.

Comment thread CMakeLists.txt
@@ -70,3 +70,4 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON)
mwg_add_runtime_data_target()

add_subdirectory(src/MapWinGIS.Core)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Suggestion | Confidence: Medium

Issue: The ActiveX subdirectory is added unconditionally, and mwg_add_midl_target hard-FATAL_ERRORs configure when midl.exe or a cl.exe on PATH is missing. There is no way to configure a Core-only CMake build.
Impact: Consumers who only want MapWinGIS.Core.dll (headless Core CI, documentation or packaging jobs, contributors without the ATL/MIDL components installed) cannot configure the project at all, even though the Core target has no MIDL dependency.
Fix: Add an option(MWG_BUILD_ACTIVEX "Build the ActiveX control (requires the Windows SDK MIDL and ATL)" ON) and gate this add_subdirectory (and document the option in docs/BUILDING.md) on it.

Comment thread CMakeLists.txt
@@ -70,3 +70,4 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON)
mwg_add_runtime_data_target()

add_subdirectory(src/MapWinGIS.Core)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Severity: Suggestion | Confidence: High

Issue: Nothing in CI exercises the new CMake build. .github/workflows/build.yml runs only the MSBuild .slnx build (plus tests); all the CMake verification in this PR (parity script, Ninja/VS-generator builds, regsvr32 round-trip) was run manually.
Impact: The entire value of this migration stack is "no lost flags / no new warnings", which is currently only guaranteed by a script a human has to remember to run. A future PR can quietly break the CMake build or the parity guarantee and CI stays green.
Fix: Add a cmake job to build.yml (e.g. Ninja x64 Debug configure + build + run Compare-CompileCommands.ps1 once the baseline is committed), so the parity guarantee is enforced on every PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants