Bug 197934 – [CMake] Use builtin FindICU

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Don Olmstead]

FindICU is present since CMake 3.7 https://cmake.org/cmake/help/v3.7/module/FindICU.html

Need to bump the version and it'd be nice to make it an icu target.

[Don Olmstead]

Created attachment 370016 [details]
WIP Patch

[Michael Catanzaro]

Comment on attachment 370016 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370016&action=review

> CMakeLists.txt:5
> -# NOTE: cmake_minimum_required() and project() *MUST* be the two fist commands
> +# NOTE: cmake_minimum_required() and project() *MUST* be the two first commands

✊

> Source/cmake/OptionsPlayStation.cmake:131
> -set(WTF_LIBRARY_TYPE STATIC)
> -set(JavaScriptCore_LIBRARY_TYPE STATIC)
> -set(WebCore_LIBRARY_TYPE STATIC)
> +set(WTF_LIBRARY_TYPE SHARED)
> +set(JavaScriptCore_LIBRARY_TYPE SHARED)
> +set(PAL_LIBRARY_TYPE SHARED)
> +set(WebCore_LIBRARY_TYPE SHARED)

You're going a little wild here.

I agree with the end goal, of course. Especially with your experiment to switch from ICU_INCLUDE_DIRS/ICU_LIBRARIES to the build targets. But surely "fundamentally change how the PlayStation port is linked" is rather a lot to bury in a "use the system FindICU" patch.

[Don Olmstead]

Comment on attachment 370016 [details]
WIP Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370016&action=review

Thank you for the unexpected review. I wasn't sure if this would break the build for any ports so I just threw it at the bots. Apple ports can't use the find_package(ICU) so for those I manually have to create the targets. Will fix that and it looks like we're good to go.

>> CMakeLists.txt:5
>> +# NOTE: cmake_minimum_required() and project() *MUST* be the two first commands
> 
> ✊

Actually ✊✊

>> Source/cmake/OptionsPlayStation.cmake:131
>> +set(WebCore_LIBRARY_TYPE SHARED)
> 
> You're going a little wild here.
> 
> I agree with the end goal, of course. Especially with your experiment to switch from ICU_INCLUDE_DIRS/ICU_LIBRARIES to the build targets. But surely "fundamentally change how the PlayStation port is linked" is rather a lot to bury in a "use the system FindICU" patch.

Yea I'm going to remove that. It slipped in when I was seeing how ICU::* was propagating. ${_target}_LIBRARIES are default PUBLIC which is something that should probably be changed.

[Don Olmstead]

Created attachment 370152 [details]
Patch

[Michael Catanzaro]

Comment on attachment 370152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370152&action=review

This is beginning to look a lot like a better future.

More targets, please!

> Source/cmake/target/icu.cmake:2
> +# Apple ports provide their own ICU that can't be found by find_package(ICU).
> +# This file will create targets that would be created by find_package(ICU).

Well, we removed a find module and grew... basically another find module? Anyway, it's fine.

[WebKit Commit Bot]

Comment on attachment 370152 [details]
Patch

Clearing flags on attachment: 370152

Committed r245492: <https://trac.webkit.org/changeset/245492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Caio Lima]

After this change, I'm not able to use `build-jsc --jsc-only` on macOS anymore. The output is:

```
CMake Error at Source/cmake/OptionsJSCOnly.cmake:102 (include):
  include could not find load file:

    target/icu.cmake
Call Stack (most recent call first):
  Source/cmake/WebKitCommon.cmake:56 (include)
  CMakeLists.txt:167 (include)
```

If I put the full path to include `target/icu.cmake`, the error is then:

```
+  cmake --build ~/webkit/WebKitBuild/Release --config Release -- jsc testb3 testair testapi testmasm testdfg -j12
ninja: error: 'libicucore.dylib', needed by 'bin/LLIntSettingsExtractor', missing and no known rule to make it
```

[Michael Catanzaro]

Hey Don, I think we're going to have to roll this out. Charlie discovered that CMake's FindICU.cmake isn't using pkg-config and is somehow not using the ICU from our jhbuild environment. So that's bad. We need it for consistent layout test results.

What we could do is bundle a modified copy of the upstream FindICU that still has the build targets?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9)
> Hey Don, I think we're going to have to roll this out. Charlie discovered
> that CMake's FindICU.cmake isn't using pkg-config and is somehow not using
> the ICU from our jhbuild environment. So that's bad. We need it for
> consistent layout test results.

We talked about setting some CMake variable that might fix this, but ultimately we should not be using any CMake find module that does not use pkg-config under the hood. That's silly and unsuitable for Linux. I know that CMake tries very hard in rolling its own custom dependency finding mechanism, but it has zero acceptance on Linux and I don't want to be using it.

But, I also don't want to roll this out, because the upstream FindICU does have real nice targets that simplified our build significantly.

Suggestions:
 
 - Just copy the upstream FindICU into our tree, and add back the pkg-config goodness? (easy)
 - Ban use of find modules altogether and just use pkg_check_modules directly everywhere. (harder but best for Linux IMO)

[Don Olmstead]

I’m not familiar with pkg-config since it doesn’t really work right for Windows. ICU doesn’t have any dependencies though. I’m also not sure why you all aren’t pointing things at the jhbuild directories to search for dependencies like Windows and PlayStation does.

I’d like to use targets throughout WebKit. The problem is that CMake introduced a lot of targets after the version of CMake required for GTK/WPE.

I’m not totally opposed to checking in a modified version of the Find modules within CMake BUT I would prefer we change things upstream before landing in WebKit. I don’t know if there’s any reason why ICU doesn’t do any pkg-config. Could be an oversight or it could be purposeful.

Just my 2 cents on this issue.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #11)
> I’m not familiar with pkg-config since it doesn’t really work right for
> Windows. ICU doesn’t have any dependencies though. I’m also not sure why you
> all aren’t pointing things at the jhbuild directories to search for
> dependencies like Windows and PlayStation does.

jhbuild sets PKG_CONFIG_PATH and LD_LIBRARY_PATH which suffices for everything we've ever tried to build, until now. FindICU.cmake seems to be the first find module we have that ignores these standard environment variables. Or, at least, now is the first time we've noticed the problem.

[Fujii Hironori]

(In reply to Don Olmstead from comment #11)
> BUT I would prefer we change things upstream before
> landing in WebKit. I don’t know if there’s any reason why ICU doesn’t do any
> pkg-config. Could be an oversight or it could be purposeful.

I think CMake wouldn't accept such change because pkg-config is not widely used among platforms CMake supports.
Instead, CMake take CMAKE_PREFIX_PATH into account.

jhbuild sets JHBUILD_PREFIX env var. What about the idea setting CMAKE_PREFIX_PATH based on JHBUILD_PREFIX?

file(TO_CMAKE_PATH $ENV{JHBUILD_PREFIX} CMAKE_PREFIX_PATH)

[Michael Catanzaro]

Hm. jhbuild should surely do that itself if required. (No problem: we can change jhbuild.)

I wonder if this is only a problem for build-webkit because this is the only situation where you would be compiling with jhbuild but not installing into the jhbuild prefix, right? It's kinda weird that build-webkit does not set any installation directories, but that's because the built product is not intended to ever be installed.

[Fujii Hironori]

(In reply to Michael Catanzaro from comment #14)
> Hm. jhbuild should surely do that itself if required. (No problem: we can
> change jhbuild.)

I don't think we should change jhbuild.
Indeed, jhbuild does right. jhbuild sets CMAKE_INSTALL_PREFIX for cmake modules.
https://github.com/GNOME/jhbuild/blob/dde410486ce3af6cb4cc7e7ffad21a6b2ef7f4c7/jhbuild/modtypes/cmake.py#L97

Setting CMAKE_INSTALL_PREFIX also suffices.
https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html

> I wonder if this is only a problem for build-webkit because this is the only
> situation where you would be compiling with jhbuild but not installing into
> the jhbuild prefix, right? It's kinda weird that build-webkit does not set
> any installation directories, but that's because the built product is not
> intended to ever be installed.

Right.
So, you mean build-webkit should invoke cmake with setting CMAKE_PREFIX_PATH or CMAKE_INSTALL_PREFIX if shouldUseJhbuild() is true, right?

[Charlie Turner]

FTR what I do to fix this issue is,

    jhbuild -f $JHBUILDRC -m $JHBUILD_MODULES run  \
	  env ICU_ROOT=$THE_JHBUILD_PREFIX cmake ...

And I get the ICU version built inside the jhbuild. The approach of extending the CMAKE_PREFIX_PATH with JHBUILD_PREFIX seems like a better idea..

[Michael Catanzaro]

(In reply to Fujii Hironori from comment #15)
> So, you mean build-webkit should invoke cmake with setting CMAKE_PREFIX_PATH
> or CMAKE_INSTALL_PREFIX if shouldUseJhbuild() is true, right?

Yes. I guess CMAKE_PREFIX_PATH, because the build result is not intended to be installed. And... I suppose that would do it. Charlie, willing to test a patch?

[Michael Catanzaro]

Hm, it's already set since r165452, but as an environment variable.

Can CMake variables be set as environment variables? I think it's not working because I don't see CMAKE_PREFIX_PATH set in my CMakeCache.txt.

[Michael Catanzaro]

Let's continue in bug #200124.

[Michael Catanzaro]

Well, reopening. I tried this little patch:

diff --git a/Tools/Scripts/webkitdirs.pm b/Tools/Scripts/webkitdirs.pm
index 1374b4b3f2d..dc5be704897 100755
--- a/Tools/Scripts/webkitdirs.pm
+++ b/Tools/Scripts/webkitdirs.pm
@@ -2220,6 +2220,11 @@ sub generateBuildSystemFromCMakeProject
     my @args;
     push @args, "-DPORT=\"$port\"";
     push @args, "-DCMAKE_INSTALL_PREFIX=\"$prefixPath\"" if $prefixPath;
+    if (shouldUseJhbuild()) {
+        my $jhbuildPath = getJhbuildPath();
+        push @args, "-DCMAKE_PREFIX_PATH=\"$jhbuildPath\"";
+        push @args, "-DCMAKE_LIBRARY_PATH=\"$jhbuildPath/lib\"";
+    }
     push @args, "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON";
     if ($config =~ /release/i) {
         push @args, "-DCMAKE_BUILD_TYPE=Release";
diff --git a/Tools/jhbuild/jhbuildrc_common.py b/Tools/jhbuild/jhbuildrc_common.py
index 61dfa7ec48e..dafd277623f 100644
--- a/Tools/jhbuild/jhbuildrc_common.py
+++ b/Tools/jhbuild/jhbuildrc_common.py
@@ -78,10 +78,6 @@ def init(jhbuildrc_globals, jhbuild_platform):
         addpath('PKG_CONFIG_PATH', os.path.join(libdir, 'pkgconfig'))
     addpath('PKG_CONFIG_PATH', os.path.join(os.sep, 'usr', 'share', 'pkgconfig'))
 
-    prefix = jhbuildrc_globals['prefix']
-    addpath('CMAKE_PREFIX_PATH', prefix)
-    addpath('CMAKE_LIBRARY_PATH', os.path.join(prefix, 'lib'))
-
     if 'JHBUILD_MIRROR' in os.environ:
         jhbuildrc_globals['dvcs_mirror_dir'] = os.environ['JHBUILD_MIRROR']
         jhbuildrc_globals['tarballdir'] = os.environ['JHBUILD_MIRROR']


But it's not enough:

//ICU data library (debug)
ICU_DATA_LIBRARY_DEBUG:FILEPATH=ICU_DATA_LIBRARY_DEBUG-NOTFOUND

//ICU data library (release)
ICU_DATA_LIBRARY_RELEASE:FILEPATH=/usr/lib64/libicudata.so

//ICU derb executable
ICU_DERB_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/derb

//ICU genbrk executable
ICU_GENBRK_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/genbrk

//ICU genccode executable
ICU_GENCCODE_EXECUTABLE:FILEPATH=ICU_GENCCODE_EXECUTABLE-NOTFOUND

//ICU gencfu executable
ICU_GENCFU_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/gencfu

//ICU gencmn executable
ICU_GENCMN_EXECUTABLE:FILEPATH=ICU_GENCMN_EXECUTABLE-NOTFOUND

//ICU gencnval executable
ICU_GENCNVAL_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/gencnval

//ICU gendict executable
ICU_GENDICT_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/gendict

//ICU gennorm2 executable
ICU_GENNORM2_EXECUTABLE:FILEPATH=ICU_GENNORM2_EXECUTABLE-NOTFOUND

//ICU genrb executable
ICU_GENRB_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/genrb

//ICU gensprep executable
ICU_GENSPREP_EXECUTABLE:FILEPATH=ICU_GENSPREP_EXECUTABLE-NOTFOUND

//ICU i18n library (debug)
ICU_I18N_LIBRARY_DEBUG:FILEPATH=ICU_I18N_LIBRARY_DEBUG-NOTFOUND

//ICU i18n library (release)
ICU_I18N_LIBRARY_RELEASE:FILEPATH=/usr/lib64/libicui18n.so

//ICU icu-config executable
ICU_ICU-CONFIG_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/icu-config

//ICU icuinfo executable
ICU_ICUINFO_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/icuinfo

//ICU icupkg executable
ICU_ICUPKG_EXECUTABLE:FILEPATH=ICU_ICUPKG_EXECUTABLE-NOTFOUND

//ICU include directory
ICU_INCLUDE_DIR:PATH=/usr/include

//ICU makeconv executable
ICU_MAKECONV_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/makeconv

//ICU Makefile.inc data file
ICU_MAKEFILE_INC:FILEPATH=/usr/lib64/icu/63.2/Makefile.inc

//ICU pkgdata executable
ICU_PKGDATA_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/pkgdata

//ICU pkgdata.inc data file
ICU_PKGDATA_INC:FILEPATH=/usr/lib64/icu/63.2/pkgdata.inc

//ICU uconv executable
ICU_UCONV_EXECUTABLE:FILEPATH=/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/uconv

//ICU uc library (debug)
ICU_UC_LIBRARY_DEBUG:FILEPATH=ICU_UC_LIBRARY_DEBUG-NOTFOUND

//ICU uc library (release)
ICU_UC_LIBRARY_RELEASE:FILEPATH=/usr/lib64/libicuuc.so


i.e. even with CMAKE_PREFIX_PATH set, FindICU is still improperly using system ICU. I think we just need to accept that CMake's FindICU is broken....

Talking to Charlie, he's not actually using build-webkit anyway: he has a custom JHBuild environment. That means when he runs 'jhbuild build', CMAKE_INSTALL_PREFIX must already be set by JHBuild (unlike when using build-webkit) and that should eliminate the need for CMAKE_PREFIX_PATH anyway.

[Michael Catanzaro]

OK, testing this myself, I see when using build-webkit everything already works fine *without* my patch in the other bug. My patch actually broke this. :)

(In reply to Fujii Hironori from comment #15)
> I don't think we should change jhbuild.
> Indeed, jhbuild does right. jhbuild sets CMAKE_INSTALL_PREFIX for cmake
> modules.
> https://github.com/GNOME/jhbuild/blob/
> dde410486ce3af6cb4cc7e7ffad21a6b2ef7f4c7/jhbuild/modtypes/cmake.py#L97
> 
> Setting CMAKE_INSTALL_PREFIX also suffices.
> https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html

I don't think so.

According to https://cmake.org/cmake/help/latest/command/find_library.html?highlight=find_library, that path is not searched. Looks like CMake treats the install prefix as separate from CMAKE_PREFIX_PATH.

So jhbuild probably *should* set CMAKE_PREFIX_PATH, to fix this sort of issue when used outside build-webkit, which is what Charlie is doing. But as build-webkit already does that (via our jhbuildrc) it is not a problem for WebKit's jhbuild environment.

So this problem only occurs when using a JHBuild environment other than WebKit's own. It so happens I do that all the time, so I care about this, but I don't have ICU in my custom JHBuild because why would I possibly build my own ICU there? Whereas Charlie has done so; that's why he's seeing this problem.

I think there's nothing to change in WebKit, we should just fix jhbuild. I'll need to decide how traumatic touching that repository again will be for me; I have no shortage of bad memories. :) Closing.

[Michael Catanzaro]

https://gitlab.gnome.org/GNOME/jhbuild/merge_requests/24, marked as WIP until Charlie tells me it works.

[Charlie Turner]

(In reply to Michael Catanzaro from comment #22)
> https://gitlab.gnome.org/GNOME/jhbuild/merge_requests/24, marked as WIP
> until Charlie tells me it works.

Nice investigation Michael, that does indeed fix the problem for me, thanks for taking the time to look into it!