[cgal] add eigen3/tbb/ceres/openmesh features, split qt-svg out of qt - #53396
[cgal] add eigen3/tbb/ceres/openmesh features, split qt-svg out of qt#53396N'yoma Diamond (nyoma-diamond) wants to merge 4 commits into
Conversation
ed63a09 to
22d93c1
Compare
1509c83 to
db59987
Compare
Billy O'Neal (BillyONeal)
left a comment
There was a problem hiding this comment.
GPT 5.6 Sol reports:
- The
licenseexpression omitsMIT, although the installedCGAL/Classification/ETHZheaders are covered by upstream's exact MIT text inInstallation/LICENSE.RFL.
Would you consider nyoma-diamond#2 ?
|
Gladly :) |
Adds standalone opt-in features for optional third-party libraries CGAL supports, following the same find_package()/include()/target_link_libraries() pattern CGAL's own upstream examples use for each: - eigen3: Eigen3 linear algebra support (was previously only obtainable via the "qt" feature, which had nothing to do with Eigen3) - qt: disentangled from Eigen3; also drops the unused "qtdeclarative" dependency, which CGAL's Qt6 GraphicsView support never references - qt-svg: SVG icon support for the "qt" feature, split out since CGAL treats Qt6::Svg as an OPTIONAL_COMPONENTS, not a hard requirement of Qt support - tbb: Intel TBB support for CGAL's parallel algorithms - ceres: Ceres Solver support, used in mesh processing optimization - openmesh: OpenMesh support, usable as an alternative mesh data structure Fixes microsoft#48042 Fixes microsoft#10736 usage documents the exact CMake incantation each feature requires, since none of CGAL's optional support targets are linked automatically -- this mirrors CGAL's own per-target opt-in design across all of its third-party integrations, not just the ones touched here.
qtbase:arm64-linux=fail and qtsvg:arm64-linux=cascade already confirm this cascade path; qt-svg depends on both cgal[qt] and qtsvg directly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
b6878b0 to
cc36543
Compare
|
CGAL is header-only, so all this does is pre-install the dependencies subject to features.
So basically it moves the port from "install the desired deps explicitly" to "install the features"? It would be useful if the user wouldn't need to know the deps. But OTOH users are still expected to manually do explicit (No changes requested, just taking notes.) |
|
I think Kai Pastor (@dg0yt) makes some good points; N'yoma Diamond (@nyoma-diamond) can you respond? If downstream customers need to do this surgery themselves that seems a strong argument to me to not add features that just pass through dependencies. |
|
Kai Pastor (@dg0yt) Billy O'Neal (@BillyONeal) Very fair point. In that case to be honest if we're following the thought of not wanting downstream consumers to need to manage the machinery manually, I'd actually suggest going the reverse direction and removing the existing If we kept the features, theoretically it may be possible to implement CMake scripts and/or apply patches to CGAL that would handle this surgery automatically, but I suspect that would be very clunky and potentially fragile. It also introduces the risk that a project with subprojects would end up forcibly including the optional features in all subprojects, even if only one needed them. E.g., suppose a project A which just needs base CGAL, with subprojects B and C, of which B also requires the Qt feature. If the port were to automatically configure all requested optional features to be enabled, then both A and C would also get them, even if they don't need it, which may introduce unnecessary bulk and build-time expense. The primary reason I proposed the new features was to clean up the existing Qt feature and pull out Eigen. I just added the additional features while I was at it since they're pretty simple. Given the existing Qt feature already required manual surgery, I thought I'd stick to that existing methodology rather than try to uproot things. The way I see it there are three potential approaches to take: (A) Remove the (B) Accept this proposal as-is with awareness that some manual intervention is still required for functional use. My hope is the updated (C) Inject explicit intervention to provide the optional third-party dependencies / features (or at least automatically include the corresponding helper scripts) alongside CGAL at all times if requested via vcpkg. This eliminates/reduces consumer labor, but requires circumnavigating CGAL's own suggestions for linking the third-party dependencies and is a much more complex methodology to implement and maintain (and also has the potential bulk issue mentioned above) Personally I'm fine with any of these directions so long as the port's existing feature design (for Let me know your thoughts :) |
This PR adds standalone opt-in features for several of CGAL's optional third-party libraries, and fixes the existing
qtfeature to only depend on what CGAL's Qt integration actually requires.Closes #48042 (requests an
eigen3feature and points outqtis overspecified).Closes #10736 (clarifies intent/expectation to properly enable CGAL's TBB support).
New/changed features:
eigen3eigen3qt, which has nothing to do with Eigen3qtqtbase[widgets]eigen3(see above) andqtdeclarative(never referenced by CGAL's Qt6 GraphicsView. Verified by readingCGAL_SetupCGAL_Qt6Dependencies.cmake, which only ever touchesOpenGL/OpenGLWidgets/Widgets, plus optionalSvg)qt-svgqtsvg, requiresqtSvgviaOPTIONAL_COMPONENTS, not as a hard requirement of Qt supporttbbtbbCGAL::TBB_supportfor CGAL's parallel algorithmsceresceresCGAL::Ceres_support, used in mesh-processing optimization (e.g. angle/area smoothing)openmeshopenmeshCGAL::OpenMesh_support, an alternative mesh data structure usable with CGAL's BGL-based algorithmsusagenow documents the exact CMake needed per feature, since CGAL never links any of these automatically. Every one of CGAL's optional third-party integrations is opt-in per-target by design, matching what CGAL's own example CMakeLists.txt files do.Design choices / caveats
Per-target opt-in, not auto-detection. [CGAL] Enable CGAL_LINKED_WITH_TBB #10736's original ask was for the port to auto-detect TBB and enable it if present. This wasn't implemented as literally requested: It would violate the maintainer guide's "ports must not be path dependent" rule. A port's installed output/behavior must not silently change based on what else happens to be installed. Explicit per-feature opt-in (as done here) keeps
cgal's behavior deterministic regardless of install order, and matches CGAL's own upstream design: Every one of its roughly 15CGAL_*_support.cmakemodules (Eigen3, TBB, Ceres, OpenMesh, OpenCV, METIS, and GLPK, among others) is opt-in per-target via an explicitinclude()+target_link_libraries(), not auto-enabled.find_package(... CONFIG REQUIRED)inusage, notQUIET. CGAL's own examples useQUIET(noREQUIRED) because they need to configure across arbitrary environments where the optional dependency may or may not exist at all. That's the wrong pattern forusage's audience: If you've enabledcgal[tbb](etc.) via vcpkg, the dependency is guaranteed to be installed, so a silentfind_packagefailure only masks real misconfiguration (wrong triplet, broken toolchain file).CONFIGalso avoids a real hazard, beyond simple correctness. CGAL bundles its own legacyFindTBB.cmake(a rawTBB_ROOT-based module predating oneTBB) onCMAKE_MODULE_PATH, and CMake tries Module mode before Config mode by default. WithoutCONFIG, a machine with strayTBB_ROOT/TBB_ARCH_PLATFORMenv vars from an unrelated install could silently hijack discovery away from vcpkg's package. Verified all five (Eigen3Config.cmake,TBBConfig.cmake,CeresConfig.cmake,OpenMeshConfig.cmake, andQt6Config.cmake) are actually installed by vcpkg's respective ports.CGAL_setup_CGAL_Qt6_dependencies()alone is not sufficient. It linksCGAL::CGAL,CGAL::Qt6_moc_and_resources, andQt6::OpenGLWidgetswith theINTERFACEkeyword, which (per CMake semantics) only propagates to further consumers of your target: It doesn't reach an executable's own link line.usagedocuments linking those three explicitly as well, confirmed by hittingLNK2001 unresolved external symbol ...GraphicsItem::metaObjectwithout it.openmesh,ceres,tbb,eigen3, andqt/qt-svgwere each individually built and run against real upstream CGAL examples (Polygon_mesh_processing/examplesfor TBB/Ceres,BGL/examples/BGL_OpenMeshfor OpenMesh,Solver_interface/examplesfor Eigen3, and aTriangulationGraphicsItem/QGraphicsViewprogram for Qt), confirming actual execution rather than a successful configure step alone. This caught both theINTERFACE-linkage issue above and confirmed vcpkg'scerespackage sets the legacy uppercaseCERES_FOUNDvariable thatCGAL_Ceres_support.cmakechecks for.METIS/GLPK/OSQP/OpenCV/ITK were investigated but not added in this PR. They follow the identical simple pattern, but I didn't build/run/verify them the same way, and some (ITK, OpenCV) are heavy dependencies. Happy to follow up separately if there's interest.
Checklist
cgal[qt]:arm64-linux=cascadeentry is unrelated to this change and left as-is.cgal[qt-svg]:arm64-linux=cascade.qtbase:arm64-linux=failandqtsvg:arm64-linux=cascadealready confirm the same cascade path, andqt-svgdepends on bothcgal[qt]andqtsvgdirectly../vcpkg x-add-version --alland committing the result.