Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link System.Globalization.Native into coreclr #6928

Merged
merged 8 commits into from
Feb 3, 2020

Conversation

safern
Copy link
Member

@safern safern commented Jan 31, 2020

Contributes to: #826

This is the first effort of using ICU on windows. Linking S.G.Native into coreclr.

Next step is to add qcalls and change all the calls into S.G.Native to be runtime calls.

Thanks @jkoritzinsky for your help on plumbing this through 😄

src/coreclr/CMakeLists.txt Outdated Show resolved Hide resolved
@am11
Copy link
Member

am11 commented Jan 31, 2020

+1, do the sources compile on windows? last time when I attempted using icu on windows (when sources were still in dotnet/coreclr repo), there were some missing headers (icucommon.h and icui18n.h).

@safern
Copy link
Member Author

safern commented Jan 31, 2020

+1, do the sources compile on windows?

Thanks for the input, @am11. Once I get to the point where we are doing runtime calls (QCalls) instead of regular PInvokes, I will start working on getting it building for windows. Windows 19H1 should have ICU and the necessary headers from my understanding.

@am11
Copy link
Member

am11 commented Jan 31, 2020

Windows 19H1 should have ICU and the necessary headers from my understanding.

I could only find icu.h in %programfiles% and %programfiles(x86)% directories; on insider build 10.0.19546.1000. Perhaps those headers weren't deemed important for Windows port or they get just installed differently. Globalization project is using ICU-C headers (not the C++ ones) - some Linux distros differentiate between the two (i.e. icuc is a separate sub-package).

@tarekgh
Copy link
Member

tarekgh commented Jan 31, 2020

CC @jefgen who may help with the comment #6928 (comment)

@jefgen
Copy link

jefgen commented Jan 31, 2020

Windows 19H1 should have ICU and the necessary headers from my understanding.
I could only find icu.h.

Note: If you're limiting yourself to Windows 19H1 and above, then you'll want to use the header icu.h and link to icu.lib (which is for icu.dll) instead.

There is some history behind the various headers... but the short version is: Just use icu.h and icu.lib/icu.dll going forward.

The super-short history is that upstream ICU made a binary breaking change in version 63.1, which necessitated the creation of mini shim layer over top of the previous binaries (icuuc.dll and icuin.dll) This new binary (icu.dll) contains both, and provides the stable C API surface for icu.h.

@safern
Copy link
Member Author

safern commented Jan 31, 2020

Build is green and changes are ready 🎉

@am11
Copy link
Member

am11 commented Feb 1, 2020

@jefgen, thanks for summarizing. Are the unicode/* headers also available somewhere, I couldn't find these on my box:

// All ICU headers need to be included here so that all function prototypes are
// available before the function pointers are declared below.
#include <unicode/ucurr.h>
#include <unicode/ucal.h>
#include <unicode/uchar.h>
#include <unicode/ucol.h>
#include <unicode/udat.h>
#include <unicode/udatpg.h>
#include <unicode/uenum.h>
#include <unicode/uidna.h>
#include <unicode/uldnames.h>
#include <unicode/ulocdata.h>
#include <unicode/unorm2.h>
#include <unicode/unum.h>
#include <unicode/ures.h>
#include <unicode/usearch.h>
#include <unicode/utf16.h>
#include <unicode/utypes.h>
#include <unicode/urename.h>
#include <unicode/ustring.h>

@jefgen
Copy link

jefgen commented Feb 1, 2020

Are the unicode/* headers also available somewhere?

Thanks for the question! The various unicode/* headers are intentionally not available anywhere in the Windows SDK, as they contain both C and C++ APIs, as well as draft, unstable, and internal ICU APIs which we cannot expose in the SDK.

The icu.h header file is generated from the unicode/* headers by filtering out the C++/draft/unstable/internal APIs, and then all of the headers are merged/concatenated together into a single file.

As an aside: FWIW, you could sort of approximate the same thing with the raw/real ICU headers by defining the following values before including any headers:

#define U_DISABLE_RENAMING 1
#define U_SHOW_CPLUSPLUS_API 0
#define U_DEFAULT_SHOW_DRAFT 0
#define U_HIDE_DRAFT_API 1
#define U_HIDE_DEPRECATED_API 1
#define U_HIDE_OBSOLETE_API 1
#define U_HIDE_INTERNAL_API 1

@mjsabby
Copy link
Contributor

mjsabby commented Feb 1, 2020

Will this feature be 19H1+ only?

@mjsabby
Copy link
Contributor

mjsabby commented Feb 1, 2020

Will this feature be 19H1+ only?

Answered my own question. From the linked issue @tarekgh mentions it will be 19H1+.

@jkotas
Copy link
Member

jkotas commented Feb 1, 2020

And you are going to have an option to bundle the ICU with your app and use that everywhere.

@safern
Copy link
Member Author

safern commented Feb 3, 2020

@jkotas @janvorli is this good to merge?

@safern
Copy link
Member Author

safern commented Feb 3, 2020

Test failure is: #30732

@safern safern merged commit be3ebf6 into dotnet:master Feb 3, 2020
@safern safern deleted the LinkSystemNativeCoreclr branch February 3, 2020 21:53
@safern
Copy link
Member Author

safern commented Feb 3, 2020

Thanks for all your reviews 😄

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants