-
Notifications
You must be signed in to change notification settings - Fork 169
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
Enable datagen fallbacking with full set of options #3640
Conversation
The errors are because I need to gate this on |
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 unclear to me how this is useful for both us and general users. Maybe you can just include this in the datagen PR?
OK I'll not merge this separately but continue working on the deduplication in this PR. |
The two most recent commits don't change behavior but have most of the new configuration options and docs. |
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.
Lgtm, not approving my own changes though
I generated the The only removals are |
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.
Happy with this, just some small things
tools/make/data.toml
Outdated
@@ -88,15 +88,21 @@ exit_on_error true | |||
if array_is_empty ${@} | |||
exec --fail-on-error cargo build -p icu_datagen --no-default-features --features rayon,provider_baked,bin,use_wasm,networking,icu_casemap,icu_displaynames,icu_relativetime,icu_compactdecimal --release | |||
bin = set "target/release/icu4x-datagen" | |||
verbose = set false |
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.
While this seems useful for you, when making changes to a component we probably don't want to be verbose, and I think the logging slows down datagen.
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.
Most people run cargo make bakeddata
which is usually faster than cargo make bakeddata component/foo
even without this. But for the sake of expediency I'll put this in a separate PR.
@@ -70,7 +70,7 @@ syn = {version = "2", features = ["parsing"], optional = true } | |||
|
|||
# Other external dependencies | |||
displaydoc = { version = "0.2.3", default-features = false } | |||
elsa = "1.7" | |||
elsa = { git = "https://github.com/Manishearth/elsa", rev = "56d05375eea36596432a0843721d26edf3b0ec75" } |
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.
Can you either move this to dev dependencies, or use a mutex in the test, or get Manish to do a release? I don't want to submit git hashes.
@@ -16,6 +16,6 @@ | |||
"path": "blob.postcard" | |||
} | |||
}, | |||
"fallback": "Legacy", | |||
"fallback": "Hybrid", |
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.
This should probably be RuntimeManual
, as this is literally used for testing manual fallback.
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.
There is no diff if I make this change, so I'll make it because if anything it helps cover RuntimeManual
.
However, note that locales: All, fallback: Hybrid
is the way to run a pass-through datagen.
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.
We don't want to run pass-through datagen though. We're testing LocaleFallbackAdapter
, which should be tested with data generated for manual fallback.
provider/datagen/src/lib.rs
Outdated
} | ||
// TODO: Consider figuring out the cases where we don't need the fallbacker. | ||
// We need it most of the time, so just pre-compute it here. | ||
provider.source.fallbacker = Some(LocaleFallbacker::try_new_unstable(&provider)?); |
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.
Note that in #2683 (comment) we decided that preresolved+explicit would not include ancestors and descendants. There should be at least one mode that allows you to exactly control which locales get included. We're hitting this problem with hello world, clients will hit it as well.
Conflicts: components/datetime/data/data/macros/datetime_skeletons_v1.data.rs provider/datagen/tests/data/postcard/fingerprints.csv tools/make/data.toml
provider/datagen/src/lib.rs
Outdated
} | ||
None => (), | ||
}; | ||
option_iter.get_or_insert_with(|| { |
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.
You can return the error here if you don't use get_or_insert_with
, then you don't have to deal with it in three other locations.
@robertbastian Do you approve this PR other than the last comment you made about code style? If I fix that comment during the day can I merge this with a rubber stamp from Manish? |
Yes |
Conflicts: components/datetime/data/data/macros/datetime_skeletons_v1.data.rs
Toward #2683