Skip to content

Allow more granular selection of system dependencies (inc. SheenBidi) - #3723

Open
solbjorn wants to merge 1 commit into
SFML:masterfrom
solbjorn:deps-granularity
Open

solbjorn wants to merge 1 commit into
SFML:masterfrom
solbjorn:deps-granularity

Conversation

@solbjorn

Copy link
Copy Markdown
Contributor
  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?

Description

"System deps" might be used not only when the corresponding packages are installed by the OS package manager, but also when compiling them separately. In either case, it would be useful to allow selecting which system/external dependencies shoud be used and which should be built within SFML more granularly.

Add the corresponding options. Freetype and Harfbuzz are combined into one option due to their circular dependencies, same with FLAC and Vorbis, and MbedTLS and libssh. Allow using external SheenBidi. The default behaves just like before the change, and the global SFML_USE_SYSTEM_DEPS switch is still here.

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

How to test this PR?

Compile test on different OSes and different combinations of the system dependencies options.

"System deps" might be used not only when the corresponding
packages are installed by the OS package manager, but also when
compiling them separately. In either case, it would be useful to
allow selecting which system/external dependencies shoud be used
and which should be built within SFML more granularly.

Add the corresponding options. Freetype and Harfbuzz are combined
into one option due to their circular dependencies, same with FLAC
and Vorbis, and MbedTLS and libssh. Allow using external SheenBidi.
The default behaves just like before the change, and the global
`SFML_USE_SYSTEM_DEPS` switch is still here.

Signed-off-by: Alexander Lobakin <alobakin@mailbox.org>
@eXpl0it3r

eXpl0it3r commented Jun 25, 2026 •

Copy link
Copy Markdown
Member

I'm not in favor of adding so many new options.

There likely will only ever be very few users that really need this kind of granularity. In those cases it's much simpler to patch SFML's CMake files, than to have SFML cover every potential configuration option. I'd even argue that if someone does need to instrument the dependency discovery as much, they likely would also need to patch other SFML CMake code.

It's the route Debian and Vcpkg packages go. It's also the route we pick when building dependencies ourselves.

As for SheenBidi, we can treat it as system package, as soon as it lands in popular distro package managers.

Comment on lines +59 to 62
if(SFML_USE_SYSTEM_LIBSSH2_MBEDTLS)
find_package(MbedTLS REQUIRED)
find_package(Libssh2 REQUIRED)
else()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it make more sense to keep SFML_USE_SYSTEM_DEPS and instead do something like this?

sfml_set_option(SFML_USE_SYSTEM_MBEDTLS ${SFML_USE_SYSTEM_DEPS} BOOL "ON to use system MbedTLS, OFF to use the bundled ones.")
sfml_set_option(SFML_USE_SYSTEM_LIBSSH2 ${SFML_USE_SYSTEM_DEPS} BOOL "ON to use system Libssh2, OFF to use the bundled ones.")

include(FetchContent)

if(SFML_USE_SYSTEM_MBEDTLS)
    find_package(MbedTLS REQUIRED)
else()
    ...
endif()

if(SFML_USE_SYSTEM_LIBSSH2)
    find_package(Libssh2 REQUIRED)
else()
    ...
endif()

This would allow a top-level CMake to do something like this

set(SFML_USE_SYSTEM_DEPS OFF)
set(SFML_USE_SYSTEM_MBEDTLS ON)

set(CMAKE_DISABLE_FIND_PACKAGE_MBEDTLS TRUE)
set(MBEDTLS_FOUND TRUE)
set(MBEDTLS_LIBRARY MbedTLS::mbedtls)
set(MBEDX509_LIBRARY MbedTLS::mbedx509)
set(MBEDCRYPTO_LIBRARY MbedTLS::mbedcrypto)
get_target_property(MBEDTLS_INCLUDE_DIR ${MBEDTLS_LIBRARY} INTERFACE_INCLUDE_DIRECTORIES)

add_subdirectory(SFML)

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.

3 participants