Remove OSX Homebrew ICU dependency #1851

ellismg · 2015-10-22T23:16:43Z

This change converts the System.Globalization.Native shim to use the ICU C API (instead of C++). Once that is done, we are able to link against the version of ICU that OSX uses for all of their locale APIs.

Please take a look @stephentoub @ericeil

The individual commits have some duplicated work, I did it this way so I could make progress incrementally, continue to test and slowly make progress driving towards C. I'm not sure if it is easier to review the 8 files or the 15 commits.

I ran all of the corefx tests locally on both Ubuntu and OSX.

We would like to be able to link against versions of ICU installed as a "operating system level library" which means we can't take a dependency on any C++ APIs. This change moves away from icu::Calendar in favor of UCalendar. I also introduce a small helper template to manage the lifetime of ICU resources.

Part of the effort to remove our usage of C++ ICU APIs. The major issue here was that the C++ API used char*'s for some things whereas the C API used UChar*'s so we needed to define our own copies of some constants. We also need to manage a buffer ourselves, instead of being able to use the underlying buffer of a retured UnicodeString.

Remove uses of the C++ DateFormat and SimpleDateFormat classes in favor of UDateFormat. As part of this change, it was easier to move some of the code that converts an ICU format string to a .NET Style format string from native code up to managed code. This code used UnicodeString and we'll need to move away from that as well as we remove all the C++ usage.

This change removes NumberFormat in favor of UNumberFormat. There is a bit of work that needs to happen in order to keep the normalization code we use to convert an ICU pattern so to something we can match against working. Instead of UnicodeStrings, the input to the normalization function is now a UChar* and we build up a std::string during normalization. This allows us to also skip a conversion from UChar* back to char* so we can find the correct pattern in our collection of patterns to examine.

Getting the regular eras is straight forward, we can do the thing we do for other locale data and just ask ICU using a specific UDateFormatSymbolType. For abbreviated eras, there's no C API, but we can try to just read the data from ICU resources and fall back to the standard width eras if that doesn't work.

To prepare for removing icu::Locale in favor of just using the id directly, remove all the uses of Locale methods except for .getName(). We now use GetLocale to create a Locale but then turn it into a char* for all the helper methods. After this change, we can update GetLocale to do locale parsing into a char buffer and remove all the locale.getName() calls with just `locale'.

Remove all the uses of the icu::Locale type in favor of just using a char* which is the raw locale id (which is exactly what all the ICU C apis use for a locale). The meat of this change si in locale.cpp to actually handle doing the conversion from UChar* to char*. The rest of the places are dealing with the fallout (GetLocale now has a different signiture and the .getName() dance is no longer needed as we have a raw locale name all the time now).

Use "= delete" syntax to make it clear the IcuHolder copy constructor and assignment opperators are removed. Remove superfluous "public" modifier on the struct closers used by the IcuHolders.

OSX ships with a copy of ICU (their globalization APIs are built on top of it). Since we only use stable c based APIs, we can link against it using the methods described in using a System ICU in the ICU User's Guide (basically we disable function renaming, don't use C++ and only use stable APIs). The ICU headers are not part of the SDK, so we continue to need ICU installed via Homebrew as a build time dependency. Fixes dotnet/corefx#3849

nguerrera · 2015-10-22T23:46:07Z

src/corefx/System.Globalization.Native/holders.h

+typedef IcuHolder<UDateFormat, UDateFormatCloser> UDateFormatHolder;
+typedef IcuHolder<UNumberFormat, UNumberFormatCloser> UNumberFormatHolder;
+typedef IcuHolder<ULocaleDisplayNames, ULocaleDisplayNamesCloser> ULocaleDisplayNamesHolder;
+typedef IcuHolder<UResourceBundle, UResourceBundleCloser> UResourceBundleHolder;


FWIW, you could cut down on the ceremony by just making the closers overloads of an IcuClose function. That would eliminate the extra template arg on IcuHolder, which you could then use directly as IcuHolder<T> without the typedefs.

Good idea. I will do this.

Actually, I am not sure if this will work. All of these Uxxx things are typedefs of void*, so I don't think we can overload.

Yuck (talking to ICU, not you). Ok.

The code did look nicer, so maybe partial template specialization is the way to go in the future. I doubt I will make any changes for RC1 however, what we have works even if it is ugly.

Could just be trading one set of ceremony for another.

Not sure how specialization would work any better than overloading if these are all really the same type.

You are right, I was not thinking clearly.

This matches what we do in other places in calendarData.cpp, the RAII pattern will make it easier to not leak memory.

stephentoub · 2015-10-23T00:52:37Z

Looks great, Matt.

stephentoub · 2015-10-23T00:52:52Z

cc: @eerhardt

ellismg · 2015-10-23T01:01:56Z

Ugh, tagged the wrong Eric at first. Feel free to ignore this @ericeil.

nguerrera · 2015-10-23T02:47:15Z

src/corefx/System.Globalization.Native/holders.h

+{
+    void operator()(UNumberFormat* pNumberFormat) const
+    {
+        udat_close(pNumberFormat);


Does NumberFormat have the same close as DateFormat or is this an all too easy mistake to make with the "everything is really void*" silliness?

Good catch. Will fix.

eerhardt · 2015-10-23T15:48:16Z

src/corefx/System.Globalization.Native/CMakeLists.txt

-    message(FATAL_ERROR "Cannot find libicui18n, try installing libicu-dev (or the appropriate package for your platform)")
-    return()
+if(NOT CLR_CMAKE_PLATFORM_DARWIN)
+    find_library(ICUUC NAMES icuuc PATHS ${ICU_HOMEBREW_LIB_PATH})


Does it make sense to have "HOMEBREW_LIB_PATH" for non-Darwin platforms?

How do you feel about switching this to use find_package(ICU REQUIRED), and then use the ${ICU_LIBRARIES} in target_link_libraries?

I fixed the HOMEBREW_LIB_PATH thing. I will look to into find_package in a follow-up to this PR.

eerhardt · 2015-10-23T16:28:42Z

src/corefx/System.Globalization.Native/calendarData.cpp

        case AbbrevMonthNames:
-            return EnumMonths(
-                locale, calendarId, DateFormatSymbols::STANDALONE, DateFormatSymbols::ABBREVIATED, callback, context);
+            return EnumSymbols(locale, calendarId, UDAT_STANDALONE_SHORT_MONTHS, 0, callback, context);
        case SuperShortDayNames:
 #ifdef HAVE_DTWIDTHTYPE_SHORT


The check in the cmake file is out of date with what is being used in code here.

eerhardt · 2015-10-23T16:50:11Z

LGTM - I just had a few minor questions.

There were a few problems that needed to be addressed: - Our detection logic around testing if ICU supported a feature was still checking for C++ stuff instead of the coresponding C code (which we ended up using). - There was some cleanup we could do now that the OSX and other Unix builds were split apart

Remove OSX Homebrew ICU dependency

davidfowl · 2015-10-26T00:35:10Z

👏

Port PR #1851 to RC1 branch

…-plus Remove OSX Homebrew ICU dependency Commit migrated from dotnet/coreclr@e93beb1

ellismg added 15 commits October 21, 2015 13:48

Remove use of ICU C++ DateFormatSymbols

39edeba

Remove ICU C++ LocaleDisplayNames

dc4b128

Remove useage of ICU C++ DecimalFormatSymbols

47b624f

Remove use of ICU C++ DateFormatSymbols

d77e6b4

Remove C++ Locale use in EnumSymbols

1f60ac7

Cleanup include directives

b3efdce

Hygine cleanups in holders.h

806dea8

Use "= delete" syntax to make it clear the IcuHolder copy constructor and assignment opperators are removed. Remove superfluous "public" modifier on the struct closers used by the IcuHolders.

dotnet-bot added the 2 - In Progress label Oct 22, 2015

dnfclas added the cla-already-signed label Oct 22, 2015

ellismg mentioned this pull request Oct 22, 2015

Remove use of ICU C++ Calendar class #1766

Closed

nguerrera reviewed Oct 22, 2015
View reviewed changes

Use std::vector instead of calloc

26e8747

This matches what we do in other places in calendarData.cpp, the RAII pattern will make it easier to not leak memory.

nguerrera reviewed Oct 23, 2015
View reviewed changes

Use correct close function for UNumberFormat

036964e

eerhardt reviewed Oct 23, 2015
View reviewed changes

ellismg added 2 commits October 23, 2015 14:23

Fix spelling issues

398dc12

ellismg force-pushed the icu-remove-c-plus-plus branch from 3eac0d9 to 392f5f4 Compare October 23, 2015 22:43

ellismg added a commit that referenced this pull request Oct 26, 2015

Merge pull request #1851 from ellismg/icu-remove-c-plus-plus

e93beb1

Remove OSX Homebrew ICU dependency

ellismg merged commit e93beb1 into dotnet:master Oct 26, 2015

dotnet-bot removed the 2 - In Progress label Oct 26, 2015

ellismg mentioned this pull request Oct 26, 2015

Port PR #1851 to RC1 branch #1869

Merged

joshfree added a commit that referenced this pull request Oct 26, 2015

Merge pull request #1869 from ellismg/port-1851-to-rc

af48d72

Port PR #1851 to RC1 branch

eerhardt mentioned this pull request Jan 31, 2020

TimeZoneInfo DisplayName is always set to StandardName on Unix dotnet/runtime#16232

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#1851 from ellismg/icu-remove-c-plus…

2280388

…-plus Remove OSX Homebrew ICU dependency Commit migrated from dotnet/coreclr@e93beb1

Remove OSX Homebrew ICU dependency #1851

Remove OSX Homebrew ICU dependency #1851

Uh oh!

Conversation

ellismg commented Oct 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Oct 23, 2015

Uh oh!

stephentoub commented Oct 23, 2015

Uh oh!

ellismg commented Oct 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Oct 23, 2015

Uh oh!

davidfowl commented Oct 26, 2015

Uh oh!

Uh oh!