Repository navigation
Conversation
Co-authored-by: Junie <junie@jetbrains.com>
Julian-MW
left a comment
There was a problem hiding this comment.
Automated review. Final decision is with the human reviewer.
| 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 |
There was a problem hiding this comment.
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]@{ |
There was a problem hiding this comment.
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.
| set(env win32) | ||
| endif() | ||
|
|
||
| set(header "${arg_OUTPUT_DIR}/${name}_i.h") |
There was a problem hiding this comment.
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.
| set(tlb "${arg_OUTPUT_DIR}/${name}.tlb") | ||
|
|
||
| add_custom_command( | ||
| OUTPUT "${header}" "${iid}" "${tlb}" |
There was a problem hiding this comment.
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.
|
|
||
| add_custom_target(${target} DEPENDS "${header}" "${iid}" "${tlb}" SOURCES "${idl}") | ||
| set_target_properties(${target} PROPERTIES FOLDER "src") | ||
|
|
There was a problem hiding this comment.
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.
| @@ -70,3 +70,4 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON) | |||
| mwg_add_runtime_data_target() | |||
|
|
|||
| add_subdirectory(src/MapWinGIS.Core) | |||
There was a problem hiding this comment.
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.
| @@ -70,3 +70,4 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON) | |||
| mwg_add_runtime_data_target() | |||
|
|
|||
| add_subdirectory(src/MapWinGIS.Core) | |||
There was a problem hiding this comment.
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.
Phase 2 of the CMake migration plan: the ActiveX control (
MapWinGIS.ocx, static ATL, apartment threaded). The MSBuild.slnxbuild is unchanged.Changes
cmake/MwgMidl.cmake:mwg_add_midl_target()runsmidl.exethrough one explicit custom command for every generator (Ninja has no.idlsupport), 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 whenmidl.exeis missing, or whencl.exeis not onPATHunder Ninja.src/MapWinGIS.ActiveX/CMakeLists.txt:SHAREDtarget,OUTPUT_NAME MapWinGIS,SUFFIX .ocx. The.def,.rcand.rgsare sources. The.rcgets anOBJECT_DEPENDSon the generated.tlb, plus a target dependency on the MIDL target._ATL_APARTMENT_THREADEDis set on C/C++ only, RC gets/l0x0409, and the Windows libs from the audit are linked explicitly.DEFINE_SYMBOL ""keeps CMake'sMapWinGIS_ActiveX_EXPORTSout. The six shared ActiveX sources are exported asMWG_ACTIVEX_SHARED_SOURCESfor the tests (D4, Phase 3).cmake/MwgCompilerOptions.cmake:/TCfor C sources (CMake only emits/TPfor C++)._i.cis matched by file name..rcentries incompile_commands.jsonare skipped.BUILDING.md(ATL/MIDL prerequisites, per-user registration) and the OCX parity status inCMAKE-MIGRATION.md.Verification
dumpbinshows identical dependents, the 5 COM exports (DllCanUnloadNow,DllGetClassObject,DllRegisterServer,DllUnregisterServer,DllInstall), and the same subsystem and DLL characteristics.MapWinGIS.ActiveX_i.his identical below its header comment.-j32Ninja builds passed (no MIDL/RC race). The VS generator builds x64 Debug/Release and x86 Debug with 0 warnings.regsvr32 /n /i:user→New-Object -ComObject MapWinGIS6.Control→ unregister works for the x64 and the Win32 OCX (32-bit PowerShell).git statusis 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
.resname differs. The Core import library comes fromtarget_link_librariesinstead of the project reference.Known limitations
Verify-ActiveXActivation.ps1run moves to Phase 3, because it needs the test executable..slnxbuild 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.