Skip to content

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Jun 1, 2025

Don't try to create targets if they already exist. (Not just theory: Multiple calls to find_package occur with transitive dependencies.)

Add missing link libs for CUDA and OpenCL, mirroring the setup during the build.

Don't link OpenMP when it wasn't used at build time. Fixes static linkage with AppleClang.

dg0yt added 2 commits June 1, 2025 11:42
Fix error on repeated find_package(ggml).
For simplicity, check only for the top-level ggml::ggml.
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 1, 2025

Tested with the standalone example in vcpkg.

@dg0yt dg0yt marked this pull request as draft June 1, 2025 10:05
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 1, 2025

Let me also adopt find_dependency. This is useful even when eventually switching to exported CMake config.

dg0yt added 2 commits June 1, 2025 16:55
Use set and append to control link lib variables.
Apply more $<LINK_ONLY...>.
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 2, 2025

The point of find_dependency, and of the changes to find_library here, is to allow for the CONFIG search mode of find_package: If a config package returns not-found, the search is to continue in other locations. It the original find_package wasn't REQUIRED, the config package must not raise an immediate fatal configuration failure.

That's why a config package must not use find_package or find_library with REQUIRED. All checks must take place before all non-reversible effects such as add_library, or even setting cache variables.

CMake can provide quite useful diagnostics for why it didn't consider a particular config package.

@dg0yt dg0yt marked this pull request as ready for review June 2, 2025 05:54
set_and_check(GGML_LIB_DIR "@PACKAGE_GGML_LIB_INSTALL_DIR@")
#set_and_check(GGML_BIN_DIR "@PACKAGE_GGML_BIN_INSTALL_DIR@")

if(NOT TARGET ggml::ggml)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the if, but I didn't add indent to the inner code. If desired, I can change the inner lines, too.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's indent.

Comment on lines 37 to 38
list(APPEND GGML_CPU_INTERFACE_LINK_LIBRARIES ${BLAS_LIBRARIES})
list(APPEND GGML_CPU_INTERFACE_LINK_OPTIONS ${BLAS_LINKER_FLAGS})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this go to GGML_BLAS_INTERFACE_LINK_LIBRARIES/OPTIONS instead?

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 21, 2025

Gentle ping.

Comment on lines +80 to +82
find_dependency(hip)
find_dependency(hipblas)
find_dependency(rocblas)
Copy link
Member

Choose a reason for hiding this comment

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

These are no longer "required" - isn't this a problem? My understanding is if we can't find a required dependency, CMake should stop immediately and report the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are no longer "required" - isn't this a problem?

No.

My understanding is if we can't find a required dependency, CMake should stop immediately and report the error.

No. CMake config files are used via find_package().

  • Nothing is required unless the consumer said REQUIRED.
  • A single failing package in CONFIG mode is the end for the current config, but not the end of the search.

And find_dependency (without REQUIRED!) is the missing link.

  • It knows how to deal with the consumer's REQUIRED.
  • It logs why it didn't consider a particular config (with regard to transitive dependencies).

This is standard CMake.
https://cmake.org/cmake/help/latest/module/CMakeFindDependencyMacro.html

Copy link
Member

@ggerganov ggerganov left a comment

Choose a reason for hiding this comment

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

Ok, let's give this a try - will merge after the indentation.

I don't understand most of the changes here, but you seem to be fluent in this stuff. If something breaks will ping you.

set_and_check(GGML_LIB_DIR "@PACKAGE_GGML_LIB_INSTALL_DIR@")
#set_and_check(GGML_BIN_DIR "@PACKAGE_GGML_BIN_INSTALL_DIR@")

if(NOT TARGET ggml::ggml)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's indent.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jul 22, 2025

I don't understand most of the changes here, but you seem to be fluent in this stuff. If something breaks will ping you.

Okay. FTR the experience is from packaging in vcpkg (static and dynamic, multi-config/MSVC, multi-platform, pkgconfig and cmake...). The ultimate goal is to use proper export of CMake config.

And to have the fixes and the testing here. The changes to test static libs are waiting (https://github.com/dg0yt/ggml/pull/1/files), but it won't be green until some problems are resolved.

@ggerganov ggerganov merged commit 0d274bd into ggml-org:master Jul 22, 2025
4 checks passed
This was referenced Jul 24, 2025
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