Package manager compatibility & CMake changes

[Chlumsky]

The CMake build script was originally created by other people and I didn't try to maintain it as there are even parts that I don't really understand, especially those having to do with installation and msdfgenConfig. However, now that the CMake files are used by many people through package managers - vcpkg and Conan, I'd like to try and improve the compatibility with these package managers and I'd like the opinions of the authors of the msdfgen packages in the respective package managers, @Haeri and @SpaceIm.

Because I didn't write the CMake files, there were some oddities that I didn't care to resolve, for example the target of the binary was named msdfgen-standalone but the executable then renamed to msdfgen, the same name as the core library target. Also, the include directory for dependent projects was incorrect, which I didn't notice.

I made the branch cmake-update with some changes to address the issues. I realize that these changes will be breaking, but I believe it is better to make these changes now and make them final, rather than having to keep the bad configuration for eternity because of compatibility. Besides, people can keep using the old version if they want to. So the changes I've made so far:

• I renamed the option MSDFGEN_BUILD_MSDFGEN_STANDALONE to just MSDFGEN_BUILD_STANDALONE which I think is more sensible.
• I renamed the core library target from msdfgen to msdfgen-core. Since the library was already divided into two targets, I don't see why the partial library should have the name of the whole project.
• I renamed the msdfgen-standalone target to just msdfgen so that the executable doesn't have to be renamed and doesn't cause problems in vcpkg.
• I fixed some of the settings and dependencies to be transitively imposed on dependent targets so that they don't have to be set multiple times. For example, it is not needed to link both msdfgen-core and msdfgen-ext because the latter will link the former.
• The interface include directories were changed so that the project is included as #include <msdfgen.h> rather than <msdfgen/msdfgen.h> as has been discussed in #126 but there was still an unresolved issue with this according to @SpaceIm. I believe this should be now fixed by changing the install interface part to $<INSTALL_INTERFACE:include/msdfgen>?
• I removed the libraries being linked by #pragma comment(lib... along with MSDFGEN_CMAKE_BUILD, which slightly complicates disabling Skia in the non-CMake solution, but it is better for the CMake build.

What I'd like to ask @Haeri and @SpaceIm is for the vcpkg and Conan packages to actually depend on and link Skia (enable the MSDFGEN_USE_SKIA option). The full version of the library should use it, and the only reason that it is disabled by default in the CMake is because it's super hard to just make it work with CMake alone, so people can enable it once they've managed to somehow configure it, but I believe this is exactly the problem that the package managers solve. Of course, if changes must be made to the CMakeLists to make this work, feel free to suggest them.

[Haeri]

Hi @Chlumsky. Great to hear that you are improving the library to be easier to maintain for the package managers. From the cmake standpoint, I am afraid I am not going to be a big help since I myself also just barely understand how cmake works. The port I made was mostly trial and error. I can provide you the link to the pull request where I had some discussions with the vcpkg people about improving the port https://github.com/microsoft/vcpkg/pull/15427. But I think you have already covered most of the points that were discussed there. Regarding breaking changes, I think you should be good to go with the changes regarding the vcpkg port, since the port only depends on a specific commit 9af250c7d6780a41dcaf536c05e3e1987a1bdcd7 of mdfgen. So even if the master branch changes, it won't affect the users from vcpkg. The port can be then updated at a later point in time.

[Chlumsky]

@Haeri Thanks for the reply. What do you think about the last part? Do you think you'd be willing to update the package to add the Skia dependency? I'd rather not attempt it myself since don't have much experience with vcpkg in general.

[Haeri]

Sure I could take another look at it when the changes are merged. I'm not sure about the Skia dependency thought. Even though vcpkg has a port for Skia and it shouldn't be a big problem to integrate it, Skia is a rather large library. In my case, for example, I have created a rendering library and I depend on msdfgen for font rendering, but I wouldn't want to import an entire massive second rendering library just to help out with the font rendering. I think it should be kept as an optional dependency if possible.

[SpaceIm]

I renamed the core library target from msdfgen to msdfgen-core. Since the library was already divided into two targets, I don't see why the partial library should have the name of the whole project.

It's a breaking change (libname changes also, is it consistent with other build files?), but it makes sense. No problem for conan.

I fixed some of the settings and dependencies to be transitively imposed on dependent targets so that they don't have to be set multiple times. For example, it is not needed to link both msdfgen-core and msdfgen-ext because the latter will link the former.

• I advice to link targets which are direct dependencies of your target, without trying to be too smart. So if msdfgen executable doesn't depend on msdfgen-core directly, you can link msdfgen-ext only, it's fine. But if it really directly depends on msdfgen-core features (and it's likely the case since you include msdfgen.h in main.cpp), I would link both targets, even if it would work with msdfgen-ext only. When someone read a CMakeLists and all the target_link_libraries, it should be able to see in one second what are the direct dependencies of a target and if they are private, public or interface.

• I see also that you link optional dependencies of FreeType. It's fine because FindFreetype.cmake shipped in CMake is not able to resolve transitive dependencies. But your discovery of optional harfbuzz is too fragile (harfbuzz may depend on GLib and ICU). Actually harbuzz provides an official CMake config files, so you should call find_package(harbuzz REQUIRED CONFIG), if found it will define harbuzz::harbuzz target.

The interface include directories were changed so that the project is included as #include <msdfgen.h> rather than <msdfgen/msdfgen.h> as has been discussed in Update cmake include directories #126 but there was still an unresolved issue with this according to @SpaceIm. I believe this should be now fixed by changing the install interface part to $<INSTALL_INTERFACE:include/msdfgen>?

👍

I removed the libraries being linked by #pragma comment(lib... along with MSDFGEN_CMAKE_BUILD, which slightly complicates disabling Skia in the non-CMake solution, but it is better for the CMake build.

👍

What I'd like to ask @Haeri and @SpaceIm is for the vcpkg and Conan packages to actually depend on and link Skia (enable the MSDFGEN_USE_SKIA option). The full version of the library should use it, and the only reason that it is disabled by default in the CMake is because it's super hard to just make it work with CMake alone, so people can enable it once they've managed to somehow configure it, but I believe this is exactly the problem that the package managers solve. Of course, if changes must be made to the CMakeLists to make this work, feel free to suggest them.

Skia is not available yet in conan-center, but when available we will add this option in msdfgen recipe and enable it by default since we follow the same default options than libraries.

=========== And an other comment: instead of hardcoded bin/lib/include folders use GNUInstallDirs: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html

    include(GNUInstallDirs)
    install(TARGETS msdfgen-core EXPORT msdfgenTargets
        RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
        LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
        ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
        PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/msdfgen/core
    )

[Chlumsky]

@SpaceIm I agree with the suggestions and have incorporated most of them, but I didn't really understand how to properly deal with the HarfBuzz issue. As mentioned in the comment, just adding find_package(harbuzz... throws an error on my machine, so unless you clarify how to make this work, I will leave it as it is for now.

[Chlumsky]

@Haeri I understand your argument, but unfortunately, Skia's path simplification algorithm is required in order for MSDFgen to work with arbitrary font files. This means that even your rendering library will render text incorrectly if certain fonts are used, specifically ones that don't have their glyph geometry normalized to eliminate self-intersections.

Believe me that I also don't like how large the dependency is, and that's why I didn't want to add it for a long time, but in the end, I have to admit that it simply is required for compatibility with font files. If there was a more compact alternative, I would ditch Skia immediately, but I haven't been able to find anything.

[kblaschke]

If HarfBuzz support is required, you can reliably detect if Freetype2 was built with it by using CMake's CheckSymbolExists module. Find Freetype2 first, then include freetype.h and test if FT_CONFIG_OPTION_USE_HARFBUZZ is defined:

find_package(Freetype REQUIRED)
include(CheckSymbolExists)
set(CMAKE_REQUIRED_INCLUDES "${FREETYPE_INCLUDE_DIR_freetype2}")
check_symbol_exists(FT_CONFIG_OPTION_USE_HARFBUZZ "freetype/freetype.h" HAVE_FREETYPE_WITH_HARFBUZZ)
unset(CMAKE_REQUIRED_INCLUDES)

if(NOT HAVE_FREETYPE_WITH_HARFBUZZ)
    message(FATAL_ERROR "Freetype2 was found, but has no HarfBuzz support, which is required.")
endif()

Since Freetype will be linked dynamically in most cases, there's no need to also link HarfBuzz.

Another possibility would be to disable HarfBuzz-specific features in MSDFgen if Freetype doesn't support them and warn the dev using message(AUTHOR_WARNING ...) that this can lead to broken rendering for some fonts. This might still be a good enough solution for people only using a fixed selection of well-tested fonts to create the glyphs in an application.

[Chlumsky]

@kblaschke Actually, HarfBuzz is not required at all, I only added it to the CMake because I was getting a link error when building it with a version of FreeType library that had HarfBuzz support enabled.