Support shared builds of Chakra.ICU #4737

jackhorton · 2018-02-23T22:27:22Z

This PR enables Chakra.ICU to be built into DLLs, both for ease of embedding and for internal testing scenarios. I have confirmed that the DLLs produced here work on Windows ~8, including with binaries that linked to Windows Kit ICU in Windows 10.

The full workflow for using these binaries with Windows Kit ICU-linked exes:

Build host exe targeting RS2+, importing <icu.h>/<icucommon.h>/<icuin.h> from the Windows Kit and linking to icuuc.lib and icuin.lib from System32
Copy exe to Windows 8
Copy Chakra.ICU.Common.dll, Chakra.ICU.i18n.dll, and Chakra.ICU.Data.dll to the same folder on the Windows 8 machine as the exe
Make a copy of Chakra.ICU.Common.dll to icuuc.dll, or symlink it (if thats a thing on Windows 8? I couldn't get it to work on chakrafs, which was my scratchpad here). Do the same for Chakra.ICU.i18n.dll -> icuin.dll
With 6 resulting files (exe, 3x Chakra.ICU.dll, icuuc, icuin), the exe should be runnable.

jackhorton · 2018-02-23T22:37:46Z

deps/Chakra.ICU/Chakra.ICU.Stubdata.vcxproj

+      <SubSystem>Console</SubSystem>
+      <!-- Make Chakra.ICU.Stubdata pretend like its Chakra.ICU.Data for linking purposes -->
+      <OutputFile>$(OutDir)\Chakra.ICU.Data.dll</OutputFile>
+      <ImportLibrary>$(OutDir)\Chakra.ICU.Data.lib</ImportLibrary>


@curtisman is there any way around this? At runtime, we want Common.dll to auto-load Data.dll, but to create Data.dll we need Common. Thus, Stubdata is built and common links against Stubdata.lib... normally. Except then Common will try to auto-load Stubdata.dll rather than Data.dll, which isn't what we want. I couldn't find a way to work around this, and having the stubdata project output misnamed files like this is what stubdata.vcxproj in upstream ICU does.

jefgen · 2018-02-23T22:58:43Z

deps/Chakra.ICU/Chakra.ICU.Build.props

+        U_DISABLE_RENAMING=1;
+        %(PreprocessorDefinitions)
+      </PreprocessorDefinitions>
+
      <!-- Default ICU Configuration (see source\common\common.vcxproj) -->


Do you actually build using the "common.vcxproj" file, or do you build ICU using some other mechanism?
(Sorry -- I'm a complete noob when it comes to how ChakraCore is built...)

We avoided the default vcxprojs for a few reasons:

They enforce building DLLs, which I wasn't a huge fan of

In general, they don't allow any options to be overridden -- for instance, the warning output is exceptionally noisy, but its impossible to disable warnings because it unanimously sets <WarningLevel>Level3</WarningLevel>, for example. Another concerning option that it set like this was turning off ASLR and DYNAMICBASE, which just seems really unnecessary if not dangerous

They hardcoded where binaries go, which didn't sit well with the rest of our vcxprojs (there is a custom-build hierarchy of variables and such that make all of the subprojects use a common IntDir and OutDir.

Basically, instead of building /source/common.vcxproj, we build Chakra.ICU.Common.vcxproj -- theyre functionally equivalent. I took a lot of the inspiration for this from the gyp-based custom build system that Steven Loomis implemented for Node.

jefgen · 2018-02-23T23:00:22Z

deps/Chakra.ICU/Chakra.ICU.Build.props

@@ -20,6 +20,11 @@
        %(PreprocessorDefinitions)
      </PreprocessorDefinitions>

+      <PreprocessorDefinitions>
+        U_DISABLE_RENAMING=1;
+        %(PreprocessorDefinitions)


If this is only for Win8+, then you likely want U_PLATFORM_HAS_WINUWP_API=1 as well.

These are used back to Windows 7, so that probably wont work. U_DISABLE_RENAMING seems to be supported on its own, right?

Ah, if this must go down-level to Win7 then you can't set U_PLATFORM_HAS_WINUWP_API.

Right, the U_DISABLE_RENAMING macro should be platform independent in that regard.

I'm a bit curious about the Win7 requirement though -- I thought this was just for working around the Server 2012 build environment limitations?

The requirement is because builds of ChakraCore are supported down to Windows 7 and VS 2015, so all changes have to be made in that context.

Right, ChakraCore != Edge. :)

jefgen · 2018-02-24T00:37:58Z

Build/Chakra.Build.props

+        %(PreprocessorDefinitions);
+        U_STATIC_IMPLEMENTATION=1
+      </PreprocessorDefinitions>
+      <!-- Act like we are using Windows Kit ICU -->


If you really want to mimic the SDK/Kit ICU version then consider perhaps adding the following:
U_DEFAULT_SHOW_DRAFT=0; U_HIDE_DRAFT_API=1; U_HIDE_DEPRECATED_API=1; U_HIDE_OBSOLETE_API=1; U_HIDE_INTERNAL_API=1;

I added these to CommonDefines.h so they they don't interfere with the in-tree build of ICU. Thanks for the suggestion!

dilijev · 2018-02-24T01:27:52Z

The instructions (workflow) in the PR description sound complicated and manual/hacky. Will there be automation to enable users to set this up more easily and/or to test builds this way?

dilijev · 2018-02-24T01:30:38Z

lib/Runtime/PlatformAgnostic/ICU.h

@@ -8,8 +8,6 @@
 #ifdef WINDOWS10_ICU
 #include <icu.h>
 #else
-#define U_STATIC_IMPLEMENTATION 1


Is this not still needed if building non-shared? (I didn't see this added to CommonDefines.h or anywhere else)

It looks like Chakra.Build.props sets the define, but I don't know if you build with both static and dynamic linking of ICU or not..?

We build with one or the other at any given time. So, the build files set this if ChakraICU=static now.

dilijev · 2018-02-24T01:38:35Z

lib/Common/CommonDefines.h

+#define U_HIDE_DRAFT_API 1
+#define U_HIDE_DEPRECATED_API 1
+#define U_HIDE_OBSOLETE_API 1
+#define U_HIDE_INTERNAL_API 1


LGTM (though I don't know if there are more flags we might find relevant for the purpose of ensuring stable-C-API-surface-only).

See @jefgen's comment (now "outdated") about what flags will make us look like windows 10.

@jefgen the Linux build is failing here because uloc_toUnicodeLocale* is marked as "stable" in ICU 54 but is actually U_DRAFT. Is that valid? If so, I may only define these U_HIDEs on Windows to allow us to keep using system ICU in Ubuntu 16.04 (ICU 55)

It looks like the API was promoted to stable in ICU version 56.
Looking at the source for ICU version 54, I don't see it marked as "stable" in the official ICU sources though. It is marked as "@draft" and also "U_DRAFT"
http://source.icu-project.org/repos/icu/icu/tags/release-54-1/source/common/unicode/uloc.h

Perhaps the copy of ICU 54 on your Linux build machines is slightly modified...? What does the header file common\unicode\uloc.h look like?

dilijev

dilijev · 2018-02-24T01:40:37Z

Build/Chakra.Build.props

@@ -55,6 +55,10 @@
        %(PreprocessorDefinitions);
        INTL_ICU=1
      </PreprocessorDefinitions>
+      <PreprocessorDefinitions Condition="'$(ChakraICU)'=='static'">
+        %(PreprocessorDefinitions);
+        U_STATIC_IMPLEMENTATION=1


U_STATIC_IMPLEMENTATION=1 [](start = 8, length = 25)

Ah, I missed this. Thanks

Not that I expect conflicts on this define name, but this exposes this define to every compilation unit (as opposed to ICU-related-only), no?

In reply to: 170405597 [](ancestors = 170405597)

Yes, but I am of the opinion that it is much easier in the long run to not keep ICU integration points separate. While its theoretically possible that we will need to change globalization libraries yet again, in the meantime it is much nicer to be able to use the recycler/common data structures right next to ICU calls. So, in that sense, its good to make the entire codebase aware of ICU in this way.

dilijev · 2018-02-24T01:41:07Z

deps/Chakra.ICU/Chakra.ICU.Build.props

@@ -20,6 +20,12 @@
        %(PreprocessorDefinitions)
      </PreprocessorDefinitions>

+      <!-- Disable renaming to maintain compatibility with Windows Kit ICU's icuuc/icuin.lib -->
+      <PreprocessorDefinitions>
+        U_DISABLE_RENAMING=1;


U_DISABLE_RENAMING=1; [](start = 8, length = 21)

Redundant with commondefines.h?

This is intentionally specific to Chakra.ICU while CommonDefines was intentionally specific to ChakraCore, though I suppose this specific flag could be combined.

jefgen · 2018-02-24T01:44:30Z

Make a copy of Chakra.ICU.Common.dll to icuuc.dll, or symlink it (if thats a thing on Windows 8? I couldn't get it to work on chakrafs, which was my scratchpad here). Do the same for Chakra.ICU.i18n.dll -> icuin.dll

Step 4 seems a bit confusing. Are you taking the contents of "Chakra.ICU.Common.dll" and then renaming it to be "icuuc.dll"? (Similarly with icuin.dll?)

jackhorton · 2018-02-24T03:46:05Z

@dilijev the instructions are specifically for when we want to use ChakraICU=shared to power a build of ChakraFull (or any other executable) that is built with Windows Kit ICU and run on Windows 8. So, we want to make that use case possible, but since theres a bunch of scripts and such automating how tests and CIs happen, I am not as interested in making it user-friendly (keeping in mind that I have no idea how to make this any more user-friendly than it currently is). If you are just a chakracore embedder and want ICU to be shared instead of static, all you need is ChakraICU=shared and it will Just Work ™️. You'll get a Build\VcBuild\bin folder that has ch.exe, chakracore.dll, chakra.icu.common.dll, chakra.icu.i18n.dll, and chakra.icu.data.dll.

@jefgen step 4 is where the messiness comes in. We have to have two copies of Chakra.ICU.Common.dll, one named as is and one named as icuuc.dll. icuuc is required because the link library seems to dictate the name of the forwarded dll, so since we link against icuuc.lib in the Windows Kit, the implementing DLL has to be called icuuc.dll (I think -- if someone with more knowledge of how DLLs work can correct me, I welcome it). However, we need to keep Chakra.ICU.Common.dll because Chakra.ICU.i18n.dll linked against Chakra.ICU.Common.lib, so for the same reason we need something with the right name. Now, for i18n, I suppose we don't actually need to keep Chakra.ICU.i18n.dll on its own, we could simply have one copy and have it called icuin.dll to satisfy the Windows Kit icuin.lib link library. I can update the instructions to mention that. We don't need to rename Chakra.ICU.Data.dll because both common and i18n linked to Chakra.ICU.Data.lib (through some hacky stuff in the stubdata project). So, its all a pile of hacks, but to be honest the default the default vcxprojs felt even hackier to me.

dilijev · 2018-02-26T21:27:22Z

@jackhorton Why not compile them originally to the expected DLL names (on all configurations) so that all linkage matches the expectation without duplicate files? I don't see any particular reason that Chakra.ICU.*.dll is a naming scheme we need to have.

jackhorton · 2018-02-26T21:34:23Z

I suppose we could do that, but this also marks that they come from a custom build system. You explicitly can not drop in icuuc.dll from either common.vcxproj or common_uwp.vcxproj in place of what is currently called Chakra.ICU.dll -- that's sort of the whole point of this whole circus. common.vcxproj has renamed symbols, among other things, and common_uwp.vcxproj requires a data file in C:\Windows\Globalization\ICU, which we also want to avoid.

dilijev · 2018-02-26T22:19:45Z

@jackhorton because of the naming of the result, it's already not clear that the libraries cannot have drop-in replacements.

dilijev · 2018-02-27T21:22:22Z

Note: @jackhorton and I discussed offline and while we both agree this is somewhat less than ideal, it's for a very special purpose embedding scenario, so the naming and instructions seem fine to me.

Merge pull request #4737 from jackhorton:icu/shared This PR enables Chakra.ICU to be built into DLLs, both for ease of embedding and for internal testing scenarios. I have confirmed that the DLLs produced here work on Windows ~8, including with binaries that linked to Windows Kit ICU in Windows 10. The full workflow for using these binaries with Windows Kit ICU-linked exes: 1) Build host exe targeting RS2+, importing <icu.h>/<icucommon.h>/<icuin.h> from the Windows Kit and linking to icuuc.lib and icuin.lib from System32 1) Copy exe to Windows 8 1) Copy Chakra.ICU.Common.dll, Chakra.ICU.i18n.dll, and Chakra.ICU.Data.dll to the same folder on the Windows 8 machine as the exe 1) Make a copy of Chakra.ICU.Common.dll to icuuc.dll, or symlink it (if thats a thing on Windows 8? I couldn't get it to work on chakrafs, which was my scratchpad here). Do the same for Chakra.ICU.i18n.dll -> icuin.dll 1) With 6 resulting files (exe, 3x Chakra.ICU.dll, icuuc, icuin), the exe should be runnable.

jackhorton added the Intl-ICU label Feb 23, 2018

jackhorton requested review from digitalinfinity, dilijev and curtisman February 23, 2018 22:27

jackhorton commented Feb 23, 2018

View reviewed changes

jefgen reviewed Feb 23, 2018

View reviewed changes

jefgen reviewed Feb 24, 2018

View reviewed changes

dilijev reviewed Feb 24, 2018

View reviewed changes

dilijev approved these changes Feb 24, 2018

View reviewed changes

dilijev reviewed Feb 24, 2018

View reviewed changes

Support shared builds of Chakra.ICU

a3e07be

jackhorton force-pushed the icu/shared branch from bedaef2 to a3e07be Compare February 27, 2018 18:29

chakrabot merged commit a3e07be into chakra-core:master Feb 28, 2018

jackhorton deleted the icu/shared branch March 7, 2018 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support shared builds of Chakra.ICU #4737

Support shared builds of Chakra.ICU #4737

jackhorton commented Feb 23, 2018 •

edited

Loading

jackhorton Feb 23, 2018

jefgen Feb 23, 2018

jackhorton Feb 23, 2018

jefgen Feb 23, 2018

jackhorton Feb 23, 2018

jefgen Feb 23, 2018

jefgen Feb 24, 2018

dilijev Feb 24, 2018

jefgen Feb 24, 2018

jefgen Feb 24, 2018

jackhorton Feb 24, 2018

dilijev commented Feb 24, 2018 •

edited

Loading

dilijev Feb 24, 2018 •

edited

Loading

jefgen Feb 24, 2018

jackhorton Feb 24, 2018

dilijev Feb 24, 2018 •

edited

Loading

jackhorton Feb 24, 2018

jackhorton Feb 27, 2018

jefgen Feb 27, 2018

dilijev left a comment

dilijev Feb 24, 2018

dilijev Feb 24, 2018

jackhorton Feb 24, 2018

dilijev Feb 24, 2018

jackhorton Feb 24, 2018

jefgen commented Feb 24, 2018

jackhorton commented Feb 24, 2018

dilijev commented Feb 26, 2018

jackhorton commented Feb 26, 2018

dilijev commented Feb 26, 2018

dilijev commented Feb 27, 2018

Support shared builds of Chakra.ICU #4737

Support shared builds of Chakra.ICU #4737

Conversation

jackhorton commented Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev commented Feb 24, 2018 • edited Loading

dilijev Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dilijev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefgen commented Feb 24, 2018

jackhorton commented Feb 24, 2018

dilijev commented Feb 26, 2018

jackhorton commented Feb 26, 2018

dilijev commented Feb 26, 2018

dilijev commented Feb 27, 2018

jackhorton commented Feb 23, 2018 •

edited

Loading

dilijev commented Feb 24, 2018 •

edited

Loading

dilijev Feb 24, 2018 •

edited

Loading

dilijev Feb 24, 2018 •

edited

Loading