Bug 205042 – [CMake] Add HarfBuzz targets

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

Add HarfBuzz:: targets within the FindHarfBuzz module.

[Don Olmstead]

Created attachment 385221 [details]
WIP Patch

Just the CMake module changes

[Don Olmstead]

Created attachment 385222 [details]
WIP Patch

[Don Olmstead]

Created attachment 385225 [details]
WIP Patch

[Don Olmstead]

Created attachment 385303 [details]
Patch

[Don Olmstead]

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.

[Konstantin Tokarev]

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

[Carlos Alberto Lopez Perez]

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)

[Don Olmstead]

(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!

[Don Olmstead]

(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?

[Don Olmstead]

Created attachment 385316 [details]
Patch

Fix review comments

[Konstantin Tokarev]

> 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

[Konstantin Tokarev]

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

[Carlos Alberto Lopez Perez]

(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.

[Don Olmstead]

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.

[Konstantin Tokarev]

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

[WebKit Commit Bot]

Comment on attachment 385316 [details]
Patch

Clearing flags on attachment: 385316

Committed r253388: <https://trac.webkit.org/changeset/253388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.