New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FreeTypeFont: link to Freetype's dependencies if it was built static #13

Merged
merged 2 commits into from Aug 1, 2016

Conversation

Projects
None yet
2 participants
@LB--
Contributor

LB-- commented Jul 21, 2016

This allows users to build the FreeTypeFont plugin as a shared library even if they are using a static Freetype installation. CMake's Freetype find module doesn't provide any info about whether Freetype was built as shared or static, nor which dependencies it was built with, so instead I check the library name for dll, so, or dylib to find out - falsely assuming it is static is mostly harmless. Unfortunately there's no easy way to check which dependencies it needs, so I just pull in all of them as optional and leave it to the user to configure them properly if needed.

Part of the motivation for this is that using Visual Studio to build Freetype as a shared library is a pain to do - Freetype's CMakeLists.txt actually has a check that ensures only MinGW can be used for creating a shared library on Windows, so instead you have to work around it by hand. It would be nice if instead we could ignore all that and just build it statically on Windows.

@mosra

This comment has been minimized.

Owner

mosra commented Jul 21, 2016

Cool!

One thing that I'm not really sure about -- Visual Studio (as opposed to MinGW) doesn't link to DLLs but to *.lib files, so FREETYPE_LIBRARIES doesn't contain any *.dll files, if I remember correctly. And personally I have no idea how one does check for that properly, because from compiler PoV linking to static and shared libs looks completely the same.

@LB--

This comment has been minimized.

Contributor

LB-- commented Jul 21, 2016

Actually, we are really lucky here. On my system:

  • MinGW shared results in lib/libfreetype.dll.a and bin/libfreetype.dll - both have dll in their name so it doesn't matter which ends up in FREETYPE_LIBRARIES
  • MinGW static results in just lib/libfreetype.a
  • Visual Studio shared would be whatever you name it when you follow the workaround
  • Visual Studio static results in just lib/freetyped.lib

So it all works out in the end. Though I did just realize I am checking the entire path and not just the filename, so let me go fix that real quick...

@LB--

This comment has been minimized.

Contributor

LB-- commented Jul 21, 2016

Alright, it should be fixed. But reading your comment again, I realize you're talking about when Visual Studio builds Freetype as shared. Since this is technically an unsupported way to build Freetype on Windows, I wouldn't worry too much about it - the whole point of this PR was to make it so Visual Studio users didn't have to do that.

@LB--

This comment has been minimized.

Contributor

LB-- commented Jul 29, 2016

So I looked around, and it turns out ftoption.h does report which dependencies were used to build FreeType, but to get that information in CMake the only way I can think of is with try_compile for each dependency, which is rather messy, but it would allow us to tell the user which dependencies they need if one can't be found rather than having them run into confusing linker errors later and wondering where they went wrong. What would you suggest I do?

@mosra

This comment has been minimized.

Owner

mosra commented Jul 29, 2016

@LB-- parse ftoption.h with some regexes? i'm doing that for Corrade/configure.h and it isn't as horrible as it seems

@LB--

This comment has been minimized.

Contributor

LB-- commented Jul 29, 2016

@mosra Maybe - when an option is not defined, the entire #define is still there, just in a block comment, e.g. /* #define FT_CONFIG_OPTION_USE_HARFBUZZ */ versus #define FT_CONFIG_OPTION_USE_HARFBUZZ. Freetype also supports letting the user do whatever they want in the file, so they could even have both the commented out and the not-commented-out version if they wanted. But if we're willing to ignore that scenario...

@mosra

This comment has been minimized.

Owner

mosra commented Jul 29, 2016

Ideally this should also go in the upstream FindFreetype.cmake so other people can make use of it .. but then we need to upstream also FindHarfBuzz.cmake...

@LB--

This comment has been minimized.

Contributor

LB-- commented Jul 29, 2016

Alright, let me know what you think of the current implementation.

Ideally this should also go in the upstream FindFreetype.cmake so other people can make use of it .. but then we need to upstream also FindHarfBuzz.cmake...

Do you want me to do that after this is merged?

# Require the dependency only if Freetype was built with it
if(_require_dep)
find_package(${_dep} QUIET REQUIRED)
list(APPEND FREETYPE_LIBRARIES ${${_DEP}_LIBRARY} ${${_DEP}_LIBRARIES})

This comment has been minimized.

@mosra

mosra Jul 29, 2016

Owner

I would create a new variable (and then link to it also) instead of modifying FREETYPE_LIBRARIES. Not sure about the interactions when using FreeType in more than one directory.

This comment has been minimized.

@LB--

LB-- Jul 29, 2016

Contributor

I changed it to FREETYPE_DEPENDENCIES, is that fine?

This comment has been minimized.

@mosra

mosra Jul 31, 2016

Owner

Maybe FREETYPE_DEPENDENCY_LIBRARIES to follow the naming conventions?

@mosra

This comment has been minimized.

Owner

mosra commented Jul 29, 2016

Just thinking out loud at the moment -- this feels like it should be part of FindFreeType.cmake instead of a part of particular buildsystem in particular project. Having it directly in FindFreeType.cmake would cover also the case when having static FreeType and static FreeTypeFont plugin.

@LB--

This comment has been minimized.

Contributor

LB-- commented Jul 29, 2016

Currently we're using CMake's FindFreetype module, so we would either have to override CMake's module with our own, or I'd have to contribute this to CMake directly (which would only help people using the newest versions of CMake). I made this PR to work around the fact that CMake's FindFreetype module was inadequate but I guess overriding it with our own would make more sense. Of course now we have to talk about the licensing issues if we copy-paste-edit or rewrite from scratch, etc...

EDIT: Reread what you wrote. Yeah, it definitely should be in CMake's FindFreetype module, but as I said it would only benefit people with newer versions of CMake. I actually don't understand why Freetype doesn't provide its own find module, though. Also, I get the feeling that CMake people aren't too inclined to do this automatic dependency searching, I've read some posts where people suggested it or asked about it and it was generally met with a glare. Maybe it's different now, though.

@mosra

This comment has been minimized.

Owner

mosra commented Jul 31, 2016

What we could definitely do is to "fork" upstream FindFreetype.cmake and put it into our modules directory so it works at least in the case of building shared FreeTypeFont plugin with static FreeType. Then the people get an option to either don't care and use the upstream module or copy our forked module to their own repository in case they want to have static FreeType + static plugin working in their project too.

I wouldn't bother too much with the licensing -- just keeping the original CMake license so there are not many differences from upstream version.

@LB--

This comment has been minimized.

Contributor

LB-- commented Aug 1, 2016

Alright, let me know what you think. You'll want to diff against https://github.com/Kitware/CMake/blob/b213a7f6ab0d4aa18e7b704bf1cf4994fae77254/Modules/FindFreetype.cmake
Also, that's the last possible version we can fork from without backporting because after that they switch to using string(APPEND) which isn't recognized by older versions of CMake.

@mosra

This comment has been minimized.

Owner

mosra commented Aug 1, 2016

Great job once again :)

Could you perhaps put the original unchanged upstream module as a separate commit with info about which version it was taken from so it's easily diffable just by viewing file history?

Fork CMake's FindFreetype module
Forked from https://github.com/Kitware/CMake/blob/b213a7f6ab0d4aa18e7b704bf1cf4994fae77254/Modules/FindFreetype.cmake
This is the last possible version we can fork from without backporting to support older CMake versions.
@LB--

This comment has been minimized.

Contributor

LB-- commented Aug 1, 2016

@mosra done.

# handle the QUIETLY and REQUIRED arguments and set FREETYPE_FOUND to TRUE if
# all listed variables are TRUE
include(${CMAKE_CURRENT_LIST_DIR}/FindPackageHandleStandardArgs.cmake)
include(${CMAKE_ROOT}/Modules/FindPackageHandleStandardArgs.cmake)

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

I would do just include(FindPackageHandleStandardArgs), similarly for SelectLibraryConfigurations above. AFAIK this absolute inclusion is done by CMake developers to prevent accidental module overrides from user-provided module dirs.

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

Oops, I just made that change without testing and actually it doesn't work for SelectLibraryConfigurations.cmake so that one will have to remain absolute.

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

It should be just SelectLibraryConfigurations, without the extension, the same for the other.

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

Er, my mistake, I forgot to remove the .cmake at the end...

# We want to match the first and not the second
set(_match_before "\n[ \t]*\\#define[ \t]*FT_CONFIG_OPTION")
set(_match_after "[ \t]*\n")
string(REGEX MATCH "${_match_before}_USE_${_DEP}${_match_after}" _use_dep "${_ftoption_h}")

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

I would move this string(REGEX.. line to the else() clause so the handling of _USE_ and _SYSTEM_ looks symmetric.

actually, it's not possible, sorry .. or could be?

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

Unfortunately we need to know the result of both _USE_ZLIB and _SYSTEM_ZLIB - the latter may be set while the former is not.

# Require the dependency only if Freetype was built with it
if(_require_dep)
find_package(${_dep} QUIET REQUIRED)
list(APPEND FREETYPE_DEPENDENCY_LIBRARIES ${${_DEP}_LIBRARY} ${${_DEP}_LIBRARIES})

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

Doesn't the ${_DEP}_LIBRARIES already contain what's in ${_DEP}_LIBRARY?

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

Some of the find modules only use one or the other, some use both. I'm just bulletproofing so I don't have to special case them. E.g. There is only ZLIB_LIBRARIES and only HARFBUZZ_LIBRARY. We could change the latter case, though, and only need to use _LIBRARIES in all cases.

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

Yes, please -- only _LIBRARIES. Patch the FindHarfBuzz.cmake. It was there originally but then I removed it when I redid the module for imported targets.

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

I just added HARFBUZZ_LIBRARIES and made it mirror HARFBUZZ_LIBRARY.

# Require the dependency only if Freetype was built with it
if(_require_dep)
find_package(${_dep} QUIET REQUIRED)

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

Why QUIET?

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

I only did that because FreeType's CMake build does it. I could remove QUIET if you want.

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

I'm not sure how the errors will be propagated to the user if both QUIET and REQUIRED is set.

This comment has been minimized.

@LB--

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

Ah, great, let's keep it then :)

(note to self: add QUIET when finding dependencies in all my Find modules)

@@ -61,3 +62,5 @@ if(NOT TARGET HarfBuzz::HarfBuzz)
IMPORTED_LOCATION ${HARFBUZZ_LIBRARY}
INTERFACE_INCLUDE_DIRECTORIES ${HARFBUZZ_INCLUDE_DIR})
endif()
set(HARFBUZZ_LIBRARIES "${HARFBUZZ_LIBRARY}")

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

Just the following should be enough I think:

set(HARFBUZZ_LIBRARIES ${HARFBUZZ_LIBRARY})

And that's also my last comment, then I'm happy to merge :)

This comment has been minimized.

@LB--

LB-- Aug 1, 2016

Contributor

Ah, that's my paranoia about spaces in paths kicking in. I find it hard to remember how each command expands arguments and whether they convert spaces to lists etc. so I just surround everything with quotes overzealously. Fixed.

This comment has been minimized.

@mosra

mosra Aug 1, 2016

Owner

From my investigation: once you create a string with spaces in it it, it will stay so in all further variable expansions, unless you explicitly replace spaces with ;.

FreeTypeFont: link to Freetype's dependencies if it was built static
This involves forking CMake's FindFreetype module and modifying it to detect if Freetype was built as static or shared, and if so, detect which dependencies it was built with and require them with find_package, finally exposing them for linking through a new variable.

@mosra mosra merged commit b32e92b into mosra:master Aug 1, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@mosra

This comment has been minimized.

Owner

mosra commented Aug 1, 2016

Merged, thanks a lot!

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment