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

srl295 · 2023-03-15T23:48:37Z

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

For: #8435 and #8495

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

@keymanapp-test-bot skip

keymanapp-test-bot · 2023-03-15T23:48:40Z

User Test Results

Test specification and instructions

User tests are not required

- icu4c as an optional dependency For: #8435

srl295 · 2023-03-15T23:51:01Z

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…

mcdurdin · 2023-03-16T02:17:20Z

core/tests/unit/ldml/meson.build

+pkgconfig = 'pkg-config'
+icu4c = dependency('icu-uc', required: false)


How will this work on Windows builds?

Choco? I'll see. Making it optional as a PoC.

Should be able to use icu (minus data) in a static link scenario if needed.

Also eventually this will be a Dev tool not a unit test.

Is this intended just for tests?

I would be happy to include icu as a dependency but we need to think about how we want to add it strategically because adding dependencies is always complicated for a cross-platform project.

Yea for tests. I think it's ultimately a part of developer tools. Which will have wasm linked icu4c or icu4x available. So this is just a way to bring up and prove out.

- wip: right now passes, because it emits the repertoire text as it goes For: #8435

- wip: simple lookup for an exact key For: #8435

srl295 · 2023-03-16T20:51:51Z

🤦 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.

- wip: conditional for hasStrings For: #8435

srl295 · 2023-04-13T12:49:05Z

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!

- https://icu.unicode.org/download/73 for #8495

srl295 · 2023-04-14T13:57:44Z

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 · 2023-04-17T07:30:05Z

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 · 2023-04-18T08:26:03Z

@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 😄

srl295 · 2023-04-18T16:06:13Z

@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 · 2023-04-19T06:18:24Z

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 · 2023-04-19T13:16:32Z

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

mcdurdin · 2023-04-20T04:57:09Z

core/subprojects/README.md

+- Ideally we would use meson's [wrapdb](https://mesonbuild.com/Wrapdb-projects.html), and in fact the files here are sourced from that project.
+- However, it requires a much newer meson version to work properly: 0.55 at least.
+- Patches for icu (i.e. meson build files) are in [`packagefiles/icu`](./packagefiles/icu/)
+- The file `packagefiles/icu-minimal.zip` is generated from the files in [`packagefiles/icu`](./packagefiles/icu/)
+- Regenerate it with these commands:


So, given we can build with meson 1.0+ on our ba-bionic64ta agent now (which is where we are limited on meson version), we should be able to rely on system-supplied ICU (v66) for Ubuntu and thus simplify the integration here, am I right?

If so, let's go ahead and do that simplification of the ICU integration here and then this should be ready to merge!

We can drop the zip file (again!) that's the simplification we can do now. We can't use upstream wrap db AND minimize size yet. But I hope to upstream changes to make the icu configurable so we can someday depend on the wrap

That's a good step forward, really happy to not include the .zip!

That's a good step forward, really happy to not include the .zip!

You ok if I force push just to drop the last couple commits? That will remove the zip I re added.

Nevermind. That's not going to work with the merges. Anyway, I did a revert and also another merge

- Revert "feat(core): icu integration steps 🙀" - This reverts commit c168253.

core/subprojects/README.md

core/subprojects/packagefiles/icu/meson.build

- version is 73.1 not 72.1 - add mention in linux build docs - add libicu-dev dependency in debian control (but not runtime) for #8495

srl295 · 2023-04-24T14:20:27Z

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.

core/subprojects/README.md

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

srl295 · 2023-04-24T15:34:34Z

🎉

keyman-server · 2023-04-24T18:01:44Z

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

srl295 self-assigned this Mar 15, 2023

github-actions bot added common/ common/web/ core/ Keyman Core epic-ldml feat labels Mar 15, 2023

feat(core): ldml repertoire test 🙀

0693e64

- icu4c as an optional dependency For: #8435

srl295 force-pushed the feat/core-8435-repertoire-test-epic-ldml branch from c1c9f4f to 0693e64 Compare March 15, 2023 23:50

github-actions bot added common/ common/web/ and removed common/ common/web/ labels Mar 15, 2023

mcdurdin reviewed Mar 16, 2023

View reviewed changes

feat(core): ldml repertoire test 🙀

ec68f6c

- wip: right now passes, because it emits the repertoire text as it goes For: #8435

github-actions bot added common/ common/web/ and removed common/ common/web/ labels Mar 16, 2023

feat(core): ldml repertoire test 🙀

ab61467

- wip: simple lookup for an exact key For: #8435

github-actions bot added common/ common/web/ and removed common/web/ common/ labels Mar 16, 2023

feat(core): ldml repertoire test 🙀

cd47909

- wip: conditional for hasStrings For: #8435

github-actions bot added common/ and removed common/ common/web/ labels Mar 16, 2023

srl295 added 2 commits April 13, 2023 17:50

feat(core): icu to 73.1 🙀

1a58a59

- https://icu.unicode.org/download/73 for #8495

Merge branch 'master' into feat/core-8435-repertoire-test-epic-ldml

ee0c660

mcdurdin modified the milestones: A17S10, A17S11 Apr 15, 2023

mcdurdin reviewed Apr 20, 2023

View reviewed changes

srl295 added 2 commits April 21, 2023 09:13

chore: revert zipfile add

8576484

- Revert "feat(core): icu integration steps 🙀" - This reverts commit c168253.

Merge branch 'master' into feat/core-8435-repertoire-test-epic-ldml

489a0ac

mcdurdin approved these changes Apr 24, 2023

View reviewed changes

core/subprojects/README.md Outdated Show resolved Hide resolved

ermshiperete reviewed Apr 24, 2023

View reviewed changes

core/subprojects/packagefiles/icu/meson.build Outdated Show resolved Hide resolved

chore(core): icu fixes in comments and linux packaging 🙀

aa1a702

- version is 73.1 not 72.1 - add mention in linux build docs - add libicu-dev dependency in debian control (but not runtime) for #8495

srl295 requested a review from darcywong00 as a code owner April 24, 2023 14:04

github-actions bot added docs linux/ labels Apr 24, 2023

Merge branch 'master' into feat/core-8435-repertoire-test-epic-ldml

4247cad

srl295 requested review from mcdurdin and ermshiperete April 24, 2023 14:19

ermshiperete approved these changes Apr 24, 2023

View reviewed changes

core/subprojects/README.md Outdated Show resolved Hide resolved

doc(core): Update core/subprojects/README.md

d52e932

Co-authored-by: Eberhard Beilharz <ermshiperete@users.noreply.github.com>

srl295 merged commit 24b4836 into master Apr 24, 2023
2 checks passed

srl295 deleted the feat/core-8435-repertoire-test-epic-ldml branch April 24, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

srl295 commented Mar 15, 2023 •

edited

keymanapp-test-bot bot commented Mar 15, 2023 •

edited

srl295 commented Mar 15, 2023

mcdurdin Mar 16, 2023

srl295 Mar 16, 2023 •

edited

mcdurdin Mar 16, 2023

srl295 Mar 16, 2023

srl295 commented Mar 16, 2023

srl295 commented Apr 13, 2023 •

edited

srl295 commented Apr 14, 2023

mcdurdin commented Apr 17, 2023

ermshiperete commented Apr 18, 2023

srl295 commented Apr 18, 2023

ermshiperete commented Apr 19, 2023 •

edited

ermshiperete commented Apr 19, 2023

mcdurdin Apr 20, 2023 •

edited

srl295 Apr 20, 2023

mcdurdin Apr 21, 2023

srl295 Apr 21, 2023

mcdurdin Apr 21, 2023

srl295 Apr 21, 2023

srl295 commented Apr 24, 2023

srl295 commented Apr 24, 2023

keyman-server commented Apr 24, 2023

		pkgconfig = 'pkg-config'
		icu4c = dependency('icu-uc', required: false)

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

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

Conversation

srl295 commented Mar 15, 2023 • edited

keymanapp-test-bot bot commented Mar 15, 2023 • edited

User Test Results

srl295 commented Mar 15, 2023

Choose a reason for hiding this comment

srl295 Mar 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Mar 16, 2023

srl295 commented Apr 13, 2023 • edited

srl295 commented Apr 14, 2023

mcdurdin commented Apr 17, 2023

ermshiperete commented Apr 18, 2023

srl295 commented Apr 18, 2023

ermshiperete commented Apr 19, 2023 • edited

ermshiperete commented Apr 19, 2023

mcdurdin Apr 20, 2023 • edited

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

srl295 commented Apr 24, 2023

srl295 commented Apr 24, 2023

keyman-server commented Apr 24, 2023

srl295 commented Mar 15, 2023 •

edited

keymanapp-test-bot bot commented Mar 15, 2023 •

edited

srl295 Mar 16, 2023 •

edited

srl295 commented Apr 13, 2023 •

edited

ermshiperete commented Apr 19, 2023 •

edited

mcdurdin Apr 20, 2023 •

edited