Conversion to vcpkg - #138
Open
alexanderbock wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current exported/installed CMake package setup has correctness issues (manifest-sync script logic and missing propagated link dependencies for a static library) that can break consumers and CI validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR converts Ghoul’s dependency management and build workflow to vcpkg manifest mode, adding overlay ports for forks/pinned versions and making Ghoul consumable as an installed CMake package (find_package(ghoul CONFIG REQUIRED)).
Changes:
- Add root
vcpkg.jsonmanifest and overlay ports (ghoul, glbinding, websocketpp, tiny-process-library) plus a CMake script to keep manifest/port deps in sync. - Rework CMake build to source all third-party dependencies from vcpkg, add install/export rules, and add a consumer regression project.
- Update CI/docs to build via CMake presets + vcpkg; remove legacy vendored/submodule dependency setup.
File summaries
| File | Description |
|---|---|
| vcpkg.json | Adds vcpkg manifest with dependencies, features, overrides, and overlay port configuration |
| tests/CMakeLists.txt | Switches unit tests to vcpkg-provided Catch2 v3 and adds ctest registrations |
| support/vcpkg/ports/websocketpp/vcpkg.json | Defines overlay port metadata for websocketpp fork |
| support/vcpkg/ports/websocketpp/usage | Documents how consumers should locate header-only websocketpp |
| support/vcpkg/ports/websocketpp/portfile.cmake | Implements header-only installation from OpenSpace websocketpp fork |
| support/vcpkg/ports/tiny-process-library/vcpkg.json | Defines overlay port metadata and host-tool deps |
| support/vcpkg/ports/tiny-process-library/portfile.cmake | Builds and installs tiny-process-library via vcpkg CMake helpers |
| support/vcpkg/ports/glbinding/vcpkg.json | Defines overlay port pinning glbinding 3.5.0 with rationale |
| support/vcpkg/ports/glbinding/portfile.cmake | Custom install/fixup logic for glbinding’s nonstandard CMake layout |
| support/vcpkg/ports/ghoul/vcpkg.json | Adds ghoul overlay port mirroring runtime deps/features (minus tests) |
| support/vcpkg/ports/ghoul/usage | Documents consumer usage for find_package(ghoul) and overrides |
| support/vcpkg/ports/ghoul/portfile.cmake | Ports Ghoul build/install via vcpkg and wires feature options |
| support/vcpkg/check-manifest-sync.cmake | Adds script to enforce manifest/port dependency parity |
| support/consumer-test/vcpkg.json | Adds a minimal consuming manifest that depends on the ghoul port |
| support/consumer-test/main.cpp | Adds compile/link regression program to validate exported config/headers |
| support/consumer-test/CMakeLists.txt | Adds minimal external-consumer style build using vcpkg toolchain + find_package |
| support/cmake/ghoulConfig.cmake.in | Adds installed package config that finds dependencies and patches websocketpp include path |
| support/cmake/ghoul_install.cmake | Adds install/export rules and generates package config/version files |
| support/cmake/FindGLM.cmake | Removes custom FindGLM module (replaced by vcpkg-provided config) |
| support/cmake/FindGLEW.cmake | Removes custom FindGLEW module (no longer used with vcpkg flow) |
| src/CMakeLists.txt | Adds exported alias target, install-friendly include dirs, and vcpkg-based dependency wiring |
| README.md | Documents vcpkg-based dependency/build/consume workflow and presets |
| Jenkinsfile | Updates CI pipeline to build/test via CMake presets and vcpkg manifest mode |
| ext/stb_image/stb_image.c | Switches stb includes to angle-bracket form for vcpkg-provided headers |
| ext/stb_image/CMakeLists.txt | Removes standalone stb_image CMake project (now built via object lib in src/CMakeLists.txt) |
| ext/renderdoc/CMakeLists.txt | Removes standalone renderdoc CMake project (header is installed directly) |
| ext/lz4/src/xxhash.c | Removes vendored LZ4 source (replaced by vcpkg lz4) |
| ext/lz4/src/lz4hc.c | Removes vendored LZ4 source (replaced by vcpkg lz4) |
| ext/lz4/src/lz4.c | Removes vendored LZ4 source (replaced by vcpkg lz4) |
| ext/lz4/README.md | Removes vendored LZ4 docs |
| ext/lz4/LICENSE | Removes vendored LZ4 license file |
| ext/lz4/include/lz4/xxhash.h | Removes vendored LZ4 headers |
| ext/lz4/include/lz4/lz4hc.h | Removes vendored LZ4 headers |
| ext/lz4/include/lz4/lz4frame.h | Removes vendored LZ4 headers |
| ext/lz4/include/lz4/lz4.h | Removes vendored LZ4 headers |
| ext/CMakeLists.txt | Removes legacy “ext/” dependency build orchestration (submodules/ExternalProject) |
| CMakePresets.json | Adds configure/build/test presets wired to vcpkg toolchain and triplets |
| CMakeLists.txt | Reworks top-level configure flow for vcpkg manifest features and optional install/tests |
| .gitmodules | Removes dependency submodules, leaving only common-compile-settings |
Review details
Suppressed comments (5)
support/vcpkg/check-manifest-sync.cmake:88
- default-features handling in flatten_dependencies is incorrect: it treats the mere presence of "default-features" as ON (even when set to false) and leaves the value empty when the key is omitted, so the sync check won't catch default-feature mismatches.
src/CMakeLists.txt:307 - Ghoul is built as a static library; dependencies used in its object files (like lz4 and tiny-process-library) must be part of the exported target's link interface, otherwise consumers linking Ghoul::Ghoul will get unresolved symbols.
src/CMakeLists.txt:347 - assimp is linked PRIVATE, but Ghoul's static library objects reference assimp symbols (e.g., modelreaderassimp.cpp). To make the installed/exported target usable, assimp needs to be in the public link interface so consumers link it automatically.
src/CMakeLists.txt:293 - On FreeBSD, inotify is linked PRIVATE. Because Ghoul is a static library, consumers may need this link dependency propagated via the exported target to avoid unresolved symbols.
support/vcpkg/check-manifest-sync.cmake:128 - feature_dependencies has its error check inverted: when the feature has a dependencies array, this currently replaces it with "[]", so feature dependency drift will never be detected.
- Files reviewed: 51/52 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+285
to
+287
| if (WIN32) | ||
| target_link_libraries(Ghoul PRIVATE "shlwapi" "WbemUuid.lib" "Ws2_32.lib") | ||
| endif () |
Comment on lines
+25
to
+38
| find_path(GHOUL_WEBSOCKETPP_INCLUDE_DIR "websocketpp/version.hpp") | ||
| if (NOT GHOUL_WEBSOCKETPP_INCLUDE_DIR) | ||
| set(Ghoul_FOUND FALSE) | ||
| set(Ghoul_NOT_FOUND_MESSAGE "Could not find the WebSocket++ headers required by Ghoul") | ||
| return () | ||
| endif () | ||
|
|
||
| include("${CMAKE_CURRENT_LIST_DIR}/GhoulTargets.cmake") | ||
|
|
||
| set_property(TARGET Ghoul::Ghoul APPEND PROPERTY | ||
| INTERFACE_INCLUDE_DIRECTORIES "${GHOUL_WEBSOCKETPP_INCLUDE_DIR}" | ||
| ) | ||
|
|
||
| check_required_components(Ghoul) |
Comment on lines
+68
to
+71
| string(JSON features ERROR_VARIABLE featuresError GET "${entry}" "features") | ||
| set(featureList "") | ||
| if (featuresError STREQUAL "NOTFOUND") | ||
| string(JSON featureCount LENGTH "${features}") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.