Add HarfBuzz:: targets within the FindHarfBuzz module.
Created attachment 385221 [details] WIP Patch Just the CMake module changes
Created attachment 385222 [details] WIP Patch
Created attachment 385225 [details] WIP Patch
Created attachment 385303 [details] Patch
Comment on attachment 385303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385303&action=review > Source/cmake/OptionsGTK.cmake:27 > +find_package(HarfBuzz 0.9.18 REQUIRED COMPONENTS ICU) Igalia folks is this ok? Latest release is 2.6.4 and 0.9.18 is from May of 2013. WPE sets it as the minimum. If not then I have to add back in the version check to see if the ICU library is split off from the main harfbuzz library.
Comment on attachment 385303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385303&action=review Actually, modern versions of HarfBuzz provide cmake config files with harfbuzz::harfbuzz and harfbuzz::icu target, though I didn't research in which version they were added > Source/cmake/FindHarfBuzz.cmake:60 > +``HarfBuzz_INCLUDE_DIRS`` While code below defines HarfBuzz_INCLUDE_DIR, not DIRS. FWIW, modern cmake use "*DIRS" naming. > Source/cmake/FindHarfBuzz.cmake:94 > + message(FATAL_ERROR "Required version (" ${Harfbuzz_FIND_VERSION} ") is higher than found version (" ${Harfbuzz_VERSION} ")") Nice catch, but it should be spelled as HarfBuzz_VERSION
Comment on attachment 385303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385303&action=review >> Source/cmake/OptionsGTK.cmake:27 >> +find_package(HarfBuzz 0.9.18 REQUIRED COMPONENTS ICU) > > Igalia folks is this ok? Latest release is 2.6.4 and 0.9.18 is from May of 2013. WPE sets it as the minimum. > > If not then I have to add back in the version check to see if the ICU library is split off from the main harfbuzz library. I think it is OK. Our policy for bumping versions of dependencies is this one: https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy Ubuntu 18.04 LTS has harfbuzz 1.7.2 and Debian 10 Stable version 2.3.1 (and Debian 9 oldstable 1.4.2)
(In reply to Konstantin Tokarev from comment #6) > Comment on attachment 385303 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385303&action=review > > Actually, modern versions of HarfBuzz provide cmake config files with > harfbuzz::harfbuzz and harfbuzz::icu target, though I didn't research in > which version they were added It seems like they want to move over to meson at some point in the future. So we'll probably have to have our own or push one to CMake's repo which would take a long time to trickle down to linux distros. > > Source/cmake/FindHarfBuzz.cmake:60 > > +``HarfBuzz_INCLUDE_DIRS`` > > While code below defines HarfBuzz_INCLUDE_DIR, not DIRS. > > FWIW, modern cmake use "*DIRS" naming. I was looking at the following modules which seemed to be more recently updates. https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindFontconfig.cmake https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindTIFF.cmake They both use _LIBRARY, marking it as advanced, and then create a _LIBRARIES variable off of it. Seems like an ok pattern there. > > Source/cmake/FindHarfBuzz.cmake:94 > > + message(FATAL_ERROR "Required version (" ${Harfbuzz_FIND_VERSION} ") is higher than found version (" ${Harfbuzz_VERSION} ")") > > Nice catch, but it should be spelled as HarfBuzz_VERSION Good catch there!
(In reply to Carlos Alberto Lopez Perez from comment #7) > Comment on attachment 385303 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385303&action=review > > >> Source/cmake/OptionsGTK.cmake:27 > >> +find_package(HarfBuzz 0.9.18 REQUIRED COMPONENTS ICU) > > > > Igalia folks is this ok? Latest release is 2.6.4 and 0.9.18 is from May of 2013. WPE sets it as the minimum. > > > > If not then I have to add back in the version check to see if the ICU library is split off from the main harfbuzz library. > > I think it is OK. > Our policy for bumping versions of dependencies is this one: > https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy > Ubuntu 18.04 LTS has harfbuzz 1.7.2 and Debian 10 Stable version 2.3.1 (and > Debian 9 oldstable 1.4.2) Great thanks for confirming! For those of us who don't use Linux is there a good way to see what the distribution is bundling and what specific version you all are supporting?
Created attachment 385316 [details] Patch Fix review comments
> It seems like they want to move over to meson at some point in the future. > So we'll probably have to have our own or push one to CMake's repo which > would take a long time to trickle down to linux distros. > Inclusion of modules unto cmake itself is long since discouraged, projects are expected to install their own cmake config files as a part of make install routine
Comment on attachment 385316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385316&action=review > Source/cmake/FindHarfBuzz.cmake:31 > +# HarfBuzz_INCLUDE_DIRS - containg the HarfBuzz headers You still have DIRS here and DIR in code, it needs to be consistent
(In reply to Don Olmstead from comment #9) > (In reply to Carlos Alberto Lopez Perez from comment #7) > > Comment on attachment 385303 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=385303&action=review > > > > >> Source/cmake/OptionsGTK.cmake:27 > > >> +find_package(HarfBuzz 0.9.18 REQUIRED COMPONENTS ICU) > > > > > > Igalia folks is this ok? Latest release is 2.6.4 and 0.9.18 is from May of 2013. WPE sets it as the minimum. > > > > > > If not then I have to add back in the version check to see if the ICU library is split off from the main harfbuzz library. > > > > I think it is OK. > > Our policy for bumping versions of dependencies is this one: > > https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy > > Ubuntu 18.04 LTS has harfbuzz 1.7.2 and Debian 10 Stable version 2.3.1 (and > > Debian 9 oldstable 1.4.2) > > Great thanks for confirming! For those of us who don't use Linux is there a > good way to see what the distribution is bundling and what specific version > you all are supporting? Yes, package info and versions for Debian and Ubuntu can be queried at https://packages.debian.org and https://packages.ubuntu.com respectively.
Comment on attachment 385316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385316&action=review > Source/cmake/FindHarfBuzz.cmake:179 > + set(HarfBuzz_INCLUDE_DIRS ${HarfBuzz_INCLUDE_DIR}) HarfBuzz_INCLUDE_DIRS is set here. See https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindTIFF.cmake#L75-76 and https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindFontconfig.cmake#L98-101 for CMake bundled find modules that use this pattern. CMake itself isn't exactly internally consistent so I don't care if we go with this or not. Fine with whatever the preference is since we're moving to targets here.
Comment on attachment 385316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385316&action=review >> Source/cmake/FindHarfBuzz.cmake:179 >> + set(HarfBuzz_INCLUDE_DIRS ${HarfBuzz_INCLUDE_DIR}) > > HarfBuzz_INCLUDE_DIRS is set here. > > See https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindTIFF.cmake#L75-76 and https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindFontconfig.cmake#L98-101 for CMake bundled find modules that use this pattern. CMake itself isn't exactly internally consistent so I don't care if we go with this or not. Fine with whatever the preference is since we're moving to targets here. Oops, I've just overlooked this line
Comment on attachment 385316 [details] Patch Clearing flags on attachment: 385316 Committed r253388: <https://trac.webkit.org/changeset/253388>
All reviewed patches have been landed. Closing bug.