Icu 63.1 min static data #666

kevinkreiser · 2018-11-06T00:35:28Z

This is a build of ICU with -Os and with the data packaging set to put the data in a static lib. This also removes a significant amount of the data. It narrows down the datasets to just: locales, unit, currency and a couple basic things in misc. Additionaly for those datasets the only locales supported are:

da de en eo es fi fr he id it ko my nl pl pt pt_PT ro ru sv tr uk vi zh zh_Hans

With all of these changes the total data size of the data library is down from 26MB to 3.1MB. Which is quite convenient for cross platform libraries that have to ship the same code to a variety of platforms.

There was a slight issue with the standard download/unpacking workflow. ICU does not ship the "code" used to build the data in the same targz as the code. Because of this the package must download the code and the data-code and splice them together (to get the static library with data built-in). What I've done is updated the downloading function with the ability to add a suffix to the file you downloaded. This allows the unpacking functions to then use the same suffix and thefore avoids destroying the cache of the other files that were downloaded for the package in question. Because of this change I am wondering if I should update the mason version from 0.18.0 to 0.19.0 or if even just new packages in mason also obviate the version update? @springmeyer or maybe @brunoabinader can you advise?

… number of locals and a few datasets such as locale units and currency

kevinkreiser · 2018-11-06T00:54:32Z

nice pull request number! 👿

kevinkreiser · 2018-11-06T01:05:08Z

scripts/icu/63.1-min-static-data/script.sh

+        a0eb02a525822ea5ab409e04cf280bacccbb5d4d \
+        data
+
+    mason_extract_zip -d ${MASON_NAME}/source


prior to the SLUG_SUFFIX this function would have zapped the cache for the previous download.

kevinkreiser · 2018-11-06T01:06:49Z

scripts/icu/63.1-min-static-data/script.sh

+    fi
+}
+
+function trim_data {


this function is what "patches" the ICU data files so that we only build some of the datasets and only for some of the locales. i could have used a patch file but ICU docs say to copy files and modify those so I regex'd my way there instead.

kevinkreiser · 2018-11-06T01:07:25Z

scripts/icu/63.1-min-static-data/script.sh

+    ./configure ${MASON_HOST_ARG} --prefix=${MASON_PREFIX} \
+    $(icu_debug_configure) \
+    $(cross_build_configure) \
+    --with-data-packaging=static \


this is the flag that makes it so that the data is included in the static library itself as opposed to a separate file

kevinkreiser · 2018-11-06T01:08:02Z

scripts/icu/63.1-min-static-data/script.sh

+
+MASON_NAME=icu
+MASON_VERSION=63.1-min-static-data
+MASON_LIB_FILE=lib/libicuuc.a


@springmeyer can one put more than one library on this line?

This is used to check that the install worked. It only supports a single file and can be anything that exists after the build completes. So, the way you have it here is 👍. No need (or ability) to point to multiple files and not pointing to the other libraries is okay - they will still get installed.

brunoabinader

Changes looks good, but I'll defer the approval to @springmeyer.

springmeyer · 2018-11-06T18:19:36Z

scripts/icu/63.1-min-static-data/script.sh

+
+    # Using uint_least16_t instead of char16_t because Android Clang doesn't recognize char16_t
+    # I'm being shady and telling users of the library to use char16_t, so there's an implicit raw cast
+    ICU_CORE_CPP_FLAGS="-DU_CHARSET_IS_UTF8=1 -DU_CHAR_TYPE=uint_least16_t"


@kevinkreiser this U_CHAR_TYPE customization (and the below -DUCHAR_TYPE=char16_t in the mason_cflags) was done by @ChrisLoer for MBGL and may not benefit your usecase, so I would consider removing it unless you specific think it is helpful for your usecase.

FWIW, mbgl-core is no longer using mason for ICU, so totally fine for the mason build to stop using the U_CHAR_TYPE customization.

indeed, we looked at vendoring as well (which i think we want to do long term) but decided against biting the bullet of figuring out how to vendor the data into the lib (so we can ship without having to bundle the data separately)

springmeyer

@kevinkreiser this is fantastic work. Awesome job getting the data library re-packing working. However I'm requesting changes here since I think the MASON_SLUG_SUFFIX should:

Land in a different PR if we want to support this
For this PR I think it would be simpler to just download the http://download.icu-project.org/files/icu4c/63.1/icu4c-63_1-data.zip manually with curl (rather than use mason_download and running into the cache issue)

So, how about that? You revert the MASON_SLUG_SUFFIX work and just download the data.zip directly? That would avoid us needing to worry about potential breakages with other packages and would isolate this particular nuance (of needing to download a second package) to just the ICU package.

kevinkreiser · 2018-11-06T19:14:51Z

@springmeyer your comments should now be addressed. can i get another look?

springmeyer

Looks great. I think this is ready for ./mason trigger which will start giving you feedback about which platforms are working and which are not.

kevinkreiser added 3 commits November 5, 2018 19:16

add extract_zip allow for multiple downloads per package via suffix

f88af17

add icu 63.1 with -Os and data in static lib. only supports a minimum…

c149fbe

… number of locals and a few datasets such as locale units and currency

check for the existance of the suffix argument before use

193ebe4

kevinkreiser requested review from springmeyer and brunoabinader November 6, 2018 00:53

kevinkreiser commented Nov 6, 2018

View reviewed changes

brunoabinader reviewed Nov 6, 2018

View reviewed changes

kevinkreiser added 2 commits November 6, 2018 12:52

sed on osx requres an exension for the in-place arg

1c93189

sed compatibility for osx

2262960

springmeyer reviewed Nov 6, 2018

View reviewed changes

springmeyer suggested changes Nov 6, 2018

View reviewed changes

kevinkreiser added 2 commits November 6, 2018 13:47

revert MASON_SLUG_SUFFIX

e599cec

shallow clone icu tagged release to avoid 2 downloads

c15d081

springmeyer approved these changes Nov 6, 2018

View reviewed changes

forgot the cloned version is up a level

d5c4ce2

kevinkreiser merged commit e8fc6bf into master Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Icu 63.1 min static data #666

Icu 63.1 min static data #666

kevinkreiser commented Nov 6, 2018 •

edited

Loading

kevinkreiser commented Nov 6, 2018

kevinkreiser Nov 6, 2018

kevinkreiser Nov 6, 2018

springmeyer Nov 6, 2018

kevinkreiser Nov 6, 2018

kevinkreiser Nov 6, 2018

springmeyer Nov 6, 2018

brunoabinader left a comment

springmeyer Nov 6, 2018

ChrisLoer Nov 6, 2018

kevinkreiser Nov 6, 2018

springmeyer left a comment

kevinkreiser commented Nov 6, 2018 •

edited

Loading

springmeyer left a comment

Icu 63.1 min static data #666

Icu 63.1 min static data #666

Conversation

kevinkreiser commented Nov 6, 2018 • edited Loading

kevinkreiser commented Nov 6, 2018

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

brunoabinader 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

springmeyer left a comment

Choose a reason for hiding this comment

kevinkreiser commented Nov 6, 2018 • edited Loading

springmeyer left a comment

Choose a reason for hiding this comment

kevinkreiser commented Nov 6, 2018 •

edited

Loading

kevinkreiser commented Nov 6, 2018 •

edited

Loading