Skip to content

Add missing find_dependency calls.#203

Merged
orange-cpp merged 1 commit into
orange-cpp:mainfrom
BillyONeal:find-dependency
Jul 15, 2026
Merged

Add missing find_dependency calls.#203
orange-cpp merged 1 commit into
orange-cpp:mainfrom
BillyONeal:find-dependency

Conversation

@BillyONeal

Copy link
Copy Markdown
Contributor

This was detected in reviewing microsoft/vcpkg#52901 by GPT 5.6 Sol.

(Also the file VERSION in this repo still claims 5.4.0 rather than 5.5.0 but I have not tried to fix that in this PR as I'm not sure what you want your next version to be)

This was detected in reviewing microsoft/vcpkg#52901 by GPT 5.6 Sol.

(Also the file VERSION in this repo still claims 5.4.0 rather than 5.5.0 but I have not tried to fix that in this PR as I'm not sure what you want your next version to be)
endif()

if (@OMATH_ENABLE_LUA@)
find_dependency(Lua)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Matching

find_package(Lua REQUIRED)

endif()

if (@OMATH_ENABLE_HOOKING@)
find_dependency(safetyhook CONFIG)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Matching

omath/CMakeLists.txt

Lines 128 to 134 in d4c86da

find_package(safetyhook CONFIG REQUIRED)
target_link_libraries(${PROJECT_NAME} PRIVATE safetyhook::safetyhook)
if (WIN32)
target_link_libraries(${PROJECT_NAME} PRIVATE d3d9 d3d11 d3d12 dxgi opengl32 gdi32)
elseif (UNIX AND NOT APPLE)
find_package(OpenGL REQUIRED)

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request updates the CMake configuration template to find the Lua, safetyhook, and OpenGL dependencies when their respective features are enabled. The reviewer points out that because these are private dependencies, they should only be required by consumers when building as a static library. It is recommended to wrap these dependency checks in a conditional block to avoid forcing shared library consumers to install them.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +9 to +18
if (@OMATH_ENABLE_LUA@)
find_dependency(Lua)
endif()

if (@OMATH_ENABLE_HOOKING@)
find_dependency(safetyhook CONFIG)
if (NOT WIN32 AND (UNIX AND NOT APPLE))
find_dependency(OpenGL)
endif()
endif()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since Lua, safetyhook, and OpenGL are linked as PRIVATE dependencies in CMakeLists.txt, they are only required by consumers when omath is built as a static library. If omath is built as a shared library (OMATH_BUILD_AS_SHARED_LIBRARY is ON), these dependencies are already linked into the shared library binary, and forcing consumers to find them via find_dependency is unnecessary and can cause configuration failures if those packages are not installed on the consumer's system.

We should wrap these find_dependency calls in a check for NOT @OMATH_BUILD_AS_SHARED_LIBRARY@ to avoid forcing shared library consumers to install these private dependencies.

if (NOT @OMATH_BUILD_AS_SHARED_LIBRARY@)
    if (@OMATH_ENABLE_LUA@)
        find_dependency(Lua)
    endif()

    if (@OMATH_ENABLE_HOOKING@)
        find_dependency(safetyhook CONFIG)
        if (NOT WIN32 AND (UNIX AND NOT APPLE))
            find_dependency(OpenGL)
        endif()
    endif()
endif()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there can be header dependencies and similar still present even if PRIVATE?

@orange-cpp
orange-cpp merged commit eac3adb into orange-cpp:main Jul 15, 2026
37 of 39 checks passed
@orange-cpp

Copy link
Copy Markdown
Owner

thx for fix!

@BillyONeal
BillyONeal deleted the find-dependency branch July 15, 2026 19:08
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