srl295 commented 1 year ago

icu4c as a required, statically linked, dependency, only on the ldml test (for now

For: #8435 and #8495

Note, f2b8b9b10d056fc7ba88acbbaa3be26cdead1324 does add build files from mesonbuild/wrapdb, includes license file (MIT)

@keymanapp-test-bot skip

keymanapp-test-bot[bot] commented 1 year ago

User Test Results

Test specification and instructions

User tests are not required

srl295 commented 1 year ago

Trying to see how this builds.

Had to run:

$ sudo ln -s /usr/local/opt/icu4c/lib/pkgconfig/* /opt/X11/lib/pkgconfig/

due to meson issues, see https://bugs.freedesktop.org/show_bug.cgi?id=111126#c8

may be able to do: meson… --build.pkg-config-path "$PKG_CONFIG_PATH"

which seems very redundant. c'mon meson…

srl295 commented 1 year ago

🤦 the mac cross compile stuff means…… the homebrew linkage doesn’t work because it’s the “wrong” architecture (arm vs x64)
WIP. It's able to get 1-1 keys (a, b, c etc.) but fails for Khmer with "No string for repertoire test: ា" since tiny….xml does not have a single key with ា
I originally excluded strings in UnicodeSet but wonder if I should rethink that. Instead of [ħ ថា] it would be [ħ {ថា}] i.e. expecting that there's a key (flick etc) that can generate "ថា" wholesale as a string
no transforms yet.

srl295 commented 1 year ago

$ ls -lh core/build/mac-x86_64/release/tests/unit/ldml/ldml
-rwxr-xr-x 1 srl295 staff 1.2M Mar 31 13:14 core/build/mac-x86_64/release/tests/unit/ldml/ldml
$ nm core/build/mac-x86_64/release/tests/unit/ldml/ldml | fgrep _72 | head
0000000100041620 T _T_CString_int64ToString_72
0000000100041570 T _T_CString_integerToString_72
00000001000416e0 T _T_CString_stringToInteger_72
00000001000414f0 T _T_CString_toLowerCase_72
0000000100041530 T _T_CString_toUpperCase_72
000000010005fbf0 T _UDataMemory_createNewInstance_72
000000010005fb90 T _UDataMemory_init_72
000000010005fda0 T _UDataMemory_isLoaded_72
000000010005fc50 T _UDataMemory_normalizeDataPointer_72
000000010005fc80 T _UDataMemory_setData_72
$ nm core/build/mac-x86_64/release/tests/unit/ldml/ldml | fgrep _72 | wc -l
1350
$ otool -L core/build/mac-x86_64/release/tests/unit/ldml/ldml
core/build/mac-x86_64/release/tests/unit/ldml/ldml:
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.36.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)

^^ statically linked

mcdurdin commented 1 year ago

From Windows build:

06:33:00   FAILED: tests/unit/ldml/ldml.exe.p/ldml_test_source.cpp.obj
06:33:00   "cl" "-Itests\unit\ldml\ldml.exe.p" "-Itests\unit\ldml" "-I..\..\..\tests\unit\ldml" "-Iinclude" "-I..\..\..\include" "-I..\..\..\..\common\include" "-Isrc" "-I..\..\..\src" "-I..\..\..\..\developer\src\ext\json" "-Isubprojects\icu\source\common" "-I..\..\..\subprojects\icu\source\common" "/MT" "/nologo" "/showIncludes" "/Zc:__cplusplus" "/W3" "/WX" "/EHsc" "/std:c++14" "/permissive-" "/O2" "/Gw" "/source-charset:utf-8" "-DU_STATIC_IMPLEMENTATION" "-DKMN_KBP" "-DKMN_KBP_EXPORTING" "-D_SCL_SECURE_NO_WARNINGS" "-D_CRT_SECURE_NO_WARNINGS" "-DUNICODE" "-DKMN_KBP_STATIC" "-DHAVE_ICU4C" "/Fdtests\unit\ldml\ldml.exe.p\ldml_test_source.cpp.pdb" /Fotests/unit/ldml/ldml.exe.p/ldml_test_source.cpp.obj "/c" ../../../tests/unit/ldml/ldml_test_source.cpp
06:33:00   ../../../tests/unit/ldml/ldml_test_source.cpp(620): error C2220: the following warning is treated as an error
06:33:00   ../../../tests/unit/ldml/ldml_test_source.cpp(620): warning C4267: 'argument': conversion from 'size_t' to 'int32_t', possible loss of data

From WASM build:

06:17:53   Downloading icu-minimal source from https://github.com/unicode-org/icu/releases/download/release-72-1/icu4c-72_1-src.tgz
06:17:53   Download size: 26303933
06:17:55   Downloading: ..........
06:17:55   
06:17:55   tests/unit/ldml/meson.build:9:0: ERROR: Subproject exists but has no meson.build file

From Linux build:

06:15:45   Downloading icu-minimal source from https://github.com/unicode-org/icu/releases/download/release-72-1/icu4c-72_1-src.tgz
06:15:45   Download size: 26303933
06:15:47   Downloading: ..........
06:15:47   
06:15:47   tests/unit/ldml/meson.build:9:0: ERROR: Subproject exists but has no meson.build file

mcdurdin commented 1 year ago

I'm keen to see the before-and-after sizes, in particular for the WASM build. macOS build time seems to be roughly doubled with the addition of ICU, so that is worth being aware of.

srl295 commented 1 year ago

I'm not quite ready to approve it yet 😁 would like to see the build passing on all platforms and have the additional docs on the subprojects folder (as well as file size delta data)

100%. I'm trying to think of how to make it work on Debian.

What would you think of falling back to pkg-config (i.e. including existing icu) if meson was too old? Or some kind of build switch. Then Windows, and anyone else with later meson, can download icu via the wrap, and debian can include the icu4c package which is already available there.

mcdurdin commented 1 year ago

What would you think of falling back to pkg-config (i.e. including existing icu) if meson was too old? Or some kind of build switch. Then Windows, and anyone else with later meson, can download icu via the wrap, and debian can include the icu4c package which is already available there.

Possible? @ermshiperete can you weigh in?

srl295 commented 1 year ago

From Windows build:

06:33:00   FAILED: tests/unit/ldml/ldml.exe.p/ldml_test_source.cpp.obj
06:33:00   "cl" "-Itests\unit\ldml\ldml.exe.p" "-Itests\unit\ldml" "-I..\..\..\tests\unit\ldml" "-Iinclude" "-I..\..\..\include" "-I..\..\..\..\common\include" "-Isrc" "-I..\..\..\src" "-I..\..\..\..\developer\src\ext\json" "-Isubprojects\icu\source\common" "-I..\..\..\subprojects\icu\source\common" "/MT" "/nologo" "/showIncludes" "/Zc:__cplusplus" "/W3" "/WX" "/EHsc" "/std:c++14" "/permissive-" "/O2" "/Gw" "/source-charset:utf-8" "-DU_STATIC_IMPLEMENTATION" "-DKMN_KBP" "-DKMN_KBP_EXPORTING" "-D_SCL_SECURE_NO_WARNINGS" "-D_CRT_SECURE_NO_WARNINGS" "-DUNICODE" "-DKMN_KBP_STATIC" "-DHAVE_ICU4C" "/Fdtests\unit\ldml\ldml.exe.p\ldml_test_source.cpp.pdb" /Fotests/unit/ldml/ldml.exe.p/ldml_test_source.cpp.obj "/c" ../../../tests/unit/ldml/ldml_test_source.cpp
06:33:00   ../../../tests/unit/ldml/ldml_test_source.cpp(620): error C2220: the following warning is treated as an error
06:33:00   ../../../tests/unit/ldml/ldml_test_source.cpp(620): warning C4267: 'argument': conversion from 'size_t' to 'int32_t', possible loss of data

From WASM build:

06:17:53   Downloading icu-minimal source from https://github.com/unicode-org/icu/releases/download/release-72-1/icu4c-72_1-src.tgz
06:17:53   Download size: 26303933
06:17:55   Downloading: ..........
06:17:55   
06:17:55   tests/unit/ldml/meson.build:9:0: ERROR: Subproject exists but has no meson.build file

From Linux build:

06:15:45   Downloading icu-minimal source from https://github.com/unicode-org/icu/releases/download/release-72-1/icu4c-72_1-src.tgz
06:15:45   Download size: 26303933
06:15:47   Downloading: ..........
06:15:47   
06:15:47   tests/unit/ldml/meson.build:9:0: ERROR: Subproject exists but has no meson.build file

There's no line 620 in that source file.

mcdurdin commented 1 year ago

There's no line 620 in that source file.

https://github.com/keymanapp/keyman/pull/8441/files#diff-b3dcd5df28d0cfcea5425d941f1e3618be88ea8c5a0501a2fd66687cd129405aR620

ermshiperete commented 1 year ago

What would you think of falling back to pkg-config (i.e. including existing icu) if meson was too old? Or some kind of build switch. Then Windows, and anyone else with later meson, can download icu via the wrap, and debian can include the icu4c package which is already available there.

Possible? @ermshiperete can you weigh in?

Yes, I think it would be beneficial to be able to use pkg-config for Debian/Ubuntu packaging. Otherwise the download step would have to happen when we create the source package because during a package build it's not possible to access the Internet. Additionally, Debian prefers dynamic libraries [1], [2].

srl295 commented 1 year ago

we could have it prefer pkg-config on linux. For now it's a meson version check. Let's see if this actually builds.

ermshiperete commented 1 year ago

we could have it prefer pkg-config on linux. For now it's a meson version check. Let's see if this actually builds.

Yes, it would be good to have it prefer pkg-config on linux since the meson version check won't help with packaging on Jammy and newer. Ubuntu 22,.04 Jammy comes with Meson 0.61, upcoming 23.04 Lunar with Meson 1.0.1.

srl295 commented 1 year ago

Will fix

El El jue, abr. 6, 2023 a la(s) 2:51 a.m., Eberhard Beilharz < @.***> escribió:

we could have it prefer pkg-config on linux. For now it's a meson version check. Let's see if this actually builds.

Yes, it would be good to have it prefer pkg-config on linux since the meson version check won't help with packaging on Jammy and newer. Ubuntu 22,.04 Jammy comes with Meson 0.61 https://packages.ubuntu.com/search?suite=jammy&arch=any&mode=exactfilename&searchon=names&keywords=meson, upcoming 23.04 Lunar with Meson 1.0.1 https://packages.ubuntu.com/search?suite=lunar&arch=any&mode=exactfilename&searchon=names&keywords=meson .

— Reply to this email directly, view it on GitHub https://github.com/keymanapp/keyman/pull/8441#issuecomment-1498637855, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGQZM2JSV7QEKDZEDURPU3W7ZYZ5ANCNFSM6AAAAAAV4QEJLE . You are receiving this because you were assigned.Message ID: @.***>

srl295 commented 1 year ago

@mcdurdin

I'm not quite ready to approve it yet 😁 would like to see the build passing on all platforms

Should be passing now.

and have the additional docs on the subprojects folder

Done

(as well as file size delta data)

Should be ±0 for core, as this is linked into the ldml tests, not into the core lib yet.

I will get you the numbers for the optimized ldml test before and after.

Build Time

As to build time, it'll be very quick on linux with the include!
If there's a way to cache core/subprojects/packagecache that will reduce network bandwidth and time
The build time can be optimized, but I'd like to actually implement new-transforms before tweaking too much. I actually might need the i18n library for regex support. That's why this is the initial ICU integration.

srl295 commented 1 year ago

With this change:

1.2M ./core/build/mac-x86_64/release/tests/unit/ldml/ldml

Without this change:

406K ./core/build/mac-x86_64/release/tests/unit/ldml/ldml

srl295 commented 1 year ago

the penultimate commit has the builds. the ultimate one is only docs

mcdurdin commented 1 year ago

New meson warning in build that we should address:

WARNING: Project targets '>=0.53.0' but uses feature introduced in '0.55.0': Local wrap patch files without patch_url.

mcdurdin commented 1 year ago

ldml.wasm doubles in size from 495948 to 954686 bytes. We can't go to release with a 500kb jump in size for web platform for this dependency. I am assuming that the size increase is largely due to ICU?

For other platforms, size is not so important, but for web it is very important -- and right now we are pushing a lot of size boundaries in various areas and we need to keep working on it.

I think I'd like to see this resolved before it disappears into a flood of TODO items if possible.

srl295 commented 1 year ago

ldml.wasm doubles in size from 495948 to 954686 bytes. We can't go to release with a 500kb jump in size for web platform for this dependency. I am assuming that the size increase is largely due to ICU?

Yes.

For other platforms, size is not so important, but for web it is very important -- and right now we are pushing a lot of size boundaries in various areas and we need to keep working on it.

Any rough guidelines on boundaries ?

I think I'd like to see this resolved before it disappears into a flood of TODO items if possible.

So as mentioned one challenge is the transform syntax, thus requirements, thus implementation is not final. Over slicing can be tricky to back out.

Could we say that the size must be acceptable before transforms go into core depending on icu? Or is the ldml tests size itself a concern?

Just trying to understand the time and product dependencies.

mcdurdin commented 1 year ago

Any rough guidelines on boundaries ?

So the current size of KeymanWeb is (before line compression) 392,929 bytes. This is already much bigger than I want it to be. A big chunk of this is ES6 polyfills for downlevel support (double-whammy here -- the people on the oldest devices can least afford the large download size but most need it). But there are also architectural choices and designs in KMW that are contributing to this.

See #3796 for more on this topic.

My preference would be for the LDML keyboard support to add at most 100KB to the size of KMW, but I recognise that this is probably unachievable. Okay, actually, my preference would be for the LDML keyboard code to reduce the size of KMW but now I am really being silly.

Could we say that the size must be acceptable before transforms go into core depending on icu?

My primary reason for blocking on web download size now is that I think that otherwise it will turn into something we don't care enough about and we'll end up going to release with a monster.

Or is the ldml tests size itself a concern?

No, I don't care about unit test file size.

srl295 commented 1 year ago

[x] tests/unit/ldml/meson.build:13:2: ERROR: Pkg-config binary for machine MachineChoice.HOST not found. Giving up.

if ( target == 'linux'  )  … 
 use   pkg-config
else
  use wrap
endif

srl295 commented 1 year ago

@mcdurdin ptal

srl295 commented 1 year ago

🤦 but the linux server (even if it's building for wasm) still has the backlevel meson, which requires the .zipfile for patching. I'll add the .zipfile back in I suppose.

that's the cause of

tests/unit/ldml/meson.build:17:2: ERROR: Subproject exists but has no meson.build file

srl295 commented 1 year ago

reverted the zipfile…

mcdurdin commented 1 year ago

🤦 but the linux server (even if it's building for wasm) still has the backlevel meson, which requires the .zipfile for patching. I'll add the .zipfile back in I suppose.

@ermshiperete we should be able to update meson on ba-bionic64ta right? It's only on packaging agents we can't do that.

srl295 commented 1 year ago

As a minimum, yes, but no reason to make that the build.

in fact 73.1 ~was just released a few hrs ago~ is coming soon … and includes improvements for building under wasm IIRC

srl295 commented 1 year ago

all passed

srl295 commented 1 year ago

@mcdurdin down to +58 bytes, ok? 😆

Warning: keymanweb.js is 58 bytes (0%) larger than 17.0.86

mcdurdin commented 1 year ago

So @srl295 are you okay with waiting on @ermshiperete's response next week before merging this, or is it blocking something else?

srl295 commented 1 year ago

So @srl295 are you okay with waiting on @ermshiperete's response next week before merging this, or is it blocking something else?

I'm ok.

… it might well be an ICU 73.1 integration then!

srl295 commented 1 year ago

18:46:21 1) KmpCompiler 18:46:21 should generates a valid .kmp (zip) file: 18:46:21 Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\BuildAgent\work\7ac43416c45637e9\keyman\developer\src\kmc-package\build\test\test-package-compiler.js) 18:46:21 at listOnTimeout (node:internal/timers:557:17) 18:46:21 at processTimers (node:internal/timers:500:7)

seems like something unrelated, rerunning

mcdurdin commented 1 year ago

seems like something unrelated, rerunning

I saw this on one of our agents with another build, checking the agents now. We may have to adjust the test timeout for that specific test.

ermshiperete commented 1 year ago

@ermshiperete we should be able to update meson on ba-bionic64ta right? It's only on packaging agents we can't do that.

Probably. I can't think of a reason why it should not work although I have the feeling I'm overlooking something. I guess we can try and find out if it works :smile:

srl295 commented 1 year ago

@ermshiperete we should be able to update meson on ba-bionic64ta right? It's only on packaging agents we can't do that.

Probably. I can't think of a reason why it should not work although I have the feeling I'm overlooking something. I guess we can try and find out if it works 😄

so, where are we on this? PR as it is requires icu to be installed beforehand if you're on linux, otherwise builds its own. Ready to merge?

ermshiperete commented 1 year ago

so, where are we on this? PR as it is requires icu to be installed beforehand if you're on linux, otherwise builds its own. Ready to merge?

Fine with me (once the agent got updated), as long as it is possible to build with ICU 66 on linux.

ermshiperete commented 1 year ago

ba-bionic-64-ta updated to meson 1.1.0.

srl295 commented 1 year ago

I'm re-requesting review because I've updated the linux docs and the controlfile. Note that it adds icu to the debian requirements for build but not deploy yet, since it's not yet a requirement until in core.

srl295 commented 1 year ago

🎉

keyman-server commented 1 year ago

Changes in this pull request will be available for download in Keyman version 17.0.93-alpha

keymanapp / keyman

feat(core): ldml repertoire test, initial ICU integration 🙀 #8441

User Test Results

Build Time