Bug 164934

Summary: [Win] Apple build is using incorrect ICU library (if present)
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, don.olmstead, gyuyoung.kim, Hironori.Fujii, mcatanzaro, pvollan
Priority: P1 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Brent Fulgham 2016-11-18 10:10:59 PST
The Apple Windows port needs to link against "libicuuc.lib" and "libicuin.lib", which have the correct symbols and versioning. For historical reasons, the Apple build environment also contains an older "icuuc.lib" and "icuin.lib", that are not compatible.

The FindICU.cmake rules will preferentially find and use the "icuuc.lib" and "icuin.lib" if they are present.

While I modified the WebKit build system to delete instances of "icuuc.lib" and "icuin.lib", I cannot do this for the production build system (to avoid breaking other Apple software).

Consequently, the FindICU.cmake rules need to be modified to account for this quirk.
Comment 1 Brent Fulgham 2016-11-18 10:11:28 PST
<rdar://problem/29329654>
Comment 2 Brent Fulgham 2016-11-18 10:18:17 PST
Created attachment 295163 [details]
Patch
Comment 3 Alex Christensen 2016-11-18 11:20:25 PST
Comment on attachment 295163 [details]
Patch

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

> Source/cmake/FindICU.cmake:26
> +if (WIN32)

:(

> Source/cmake/FindICU.cmake:37
> +        NAMES icuuc libicuuc cygicuuc cygicuuc32

I think we should make this platform independent and just not look for icuuc anywhere, even on linux.
Comment 4 Michael Catanzaro 2016-11-18 12:36:58 PST
Comment on attachment 295163 [details]
Patch

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

>> Source/cmake/FindICU.cmake:37
>> +        NAMES icuuc libicuuc cygicuuc cygicuuc32
> 
> I think we should make this platform independent and just not look for icuuc anywhere, even on linux.

FWIW the only one of these that exists on Linux is libicuuc. I guess icuuc is probably Windows-specific, so we should go with Alex's suggestion if it doesn't seem to break anything.
Comment 5 Brent Fulgham 2016-11-18 13:31:43 PST
Created attachment 295185 [details]
Patch
Comment 6 Brent Fulgham 2016-11-18 13:32:12 PST
Revised patch removing 'icuin.lib' and 'icuuc.lib'.
Comment 7 Don Olmstead 2016-11-18 13:34:54 PST
Does it work if you put lib* in front of the other ones?
Comment 8 Alex Christensen 2016-11-18 13:36:20 PST
Don, do you use icuin.lib or icuuc.lib, or libicuin.lib and libicuuc.lib?
Comment 9 Don Olmstead 2016-11-18 13:39:00 PST
(In reply to comment #8)
> Don, do you use icuin.lib or icuuc.lib, or libicuin.lib and libicuuc.lib?

Ours are all lib* for icu
Comment 10 Alex Christensen 2016-11-18 13:40:54 PST
Comment on attachment 295185 [details]
Patch

It looks like EFL and only EFL needs icuuc and icuin.
Comment 11 Don Olmstead 2016-11-18 13:45:08 PST
(In reply to comment #10)
> Comment on attachment 295185 [details]
> Patch
> 
> It looks like EFL and only EFL needs icuuc and icuin.

But does the ordering matter in CMake? If the ordering was switched to have libicuuc at the front does CMake choose that or still icuuc?
Comment 12 Michael Catanzaro 2016-11-18 14:07:13 PST
(In reply to comment #10)
> Comment on attachment 295185 [details]
> Patch
> 
> It looks like EFL and only EFL needs icuuc and icuin.

I cannot imagine why the library name would be different... Gyuyoung, any ideas?
Comment 13 Don Olmstead 2016-11-18 14:25:26 PST
CMake has some variables for prefixes for libraries CMAKE_SHARED_LIBRARY_PREFIX and CMAKE_STATIC_LIBRARY_PREFIX. I believe it uses this with the find scripts.

For *NIX it appears that CMAKE_SHARED_LIBRARY_PREFIX and CMAKE_STATIC_LIBRARY_PREFIX are set to "lib". On Windows its set to "". That's why lib* had to be added to work with Windows.

Platform files are in share/cmake-${version}/Platform/ for reference.

For the WinCairoRequirements its a mix of stuff prefixed with lib and stuff without it. If all of them were prefixed then we could just set those values.

My only other thought is that maybe if its

NAMES libicui18n libicuin cygicuin cygicuin32 icui18n icuin

then the libicu ones will be used on windows as its the first match.
Comment 14 Brent Fulgham 2016-11-30 09:39:56 PST
Created attachment 295726 [details]
Patch
Comment 15 Brent Fulgham 2016-11-30 09:41:24 PST
I updated the patch to move the undesirable ICU names to the end of the set of choices. Let's see if that works!
Comment 16 Brent Fulgham 2016-11-30 09:50:31 PST
This seems to work on my development machine. By reordering the libraries being searched for, it preferentially locates the "libicuin.lib" and "libicuuc.lib" files, even when the "icuin.lib" and "icuuc.lib" files are present in the libraries folder.
Comment 17 WebKit Commit Bot 2016-11-30 10:46:17 PST
Comment on attachment 295726 [details]
Patch

Clearing flags on attachment: 295726

Committed r209138: <http://trac.webkit.org/changeset/209138>
Comment 18 WebKit Commit Bot 2016-11-30 10:46:22 PST
All reviewed patches have been landed.  Closing bug.