-
Notifications
You must be signed in to change notification settings - Fork 183
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
Baked data for FFI examples #3630
Conversation
wasm_default = ["buffer_provider", "logging"] | ||
wasm_default = ["logging"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: It would break people who rely on the buffer provider in WASM if we turn this off, but it's probably okay because I think not too many people are using WASM yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably ok. The node package should probably use wasm_default
as it's the canonical way to use this, and should also use compiled data.
@@ -15,54 +15,51 @@ GCC := gcc | |||
CLANG := clang-14 | |||
LLD := lld-14 | |||
|
|||
baked_data/macros.rs: | |||
cargo run -p icu_datagen -- --locales en bn --keys all --fallback expand --format mod --use-separate-crates --out baked_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observation: Previously you had --keys "decimal/symbols@1"
and now you have --keys all
. This seems to slow down CI: running this step takes about 1m 18s on CI, whereas previously it took 1s.
You already depend on icu_capi
with all features disabled except for icu_decimal
. Can you generate the data with only the keys for the decimal component?
However, overall CI speed does not seem to be affected, so this is fine for now if it's not an easy fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this actually made me notice that we should probably generate empty implementations for all keys, otherwise compiled data will not compile. I'll make that change in datagen and then undo the --keys all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
# Manually running datagen with the required keys | ||
cargo run -p icu_datagen -- --key-file ../wasm-demo/required-keys.txt --locales full --format blob --out data.postcard | ||
cd ../wasm-demo | ||
make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add cd ../wasm-demo
after make
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'll be it, thanks!
#2945