Skip to content
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

Put date skeletons behind a feature #2370

Merged
merged 18 commits into from
Aug 16, 2022
Merged

Conversation

sffc
Copy link
Member

@sffc sffc commented Aug 12, 2022

Fixes #2324

I added two features to icu_datetime:

  • "experimental" for full-featured components bag
  • "experimental_skeleton_matching" for a subset required by datagen (this is also a dependency of the "dategen" feature)

@robertbastian
Copy link
Member

Activate this feature in https://github.com/unicode-org/icu4x/blob/main/components/icu/Cargo.toml#L73

@sffc sffc marked this pull request as ready for review August 12, 2022 21:27
@sffc sffc removed request for a team, gregtatum and robertbastian August 12, 2022 21:27
@sffc
Copy link
Member Author

sffc commented Aug 12, 2022

A thorny side-issue is that this brings up questions about whether or not "datagen" is stable for clients to use. We've gone back and forth on that topic before.

Manishearth
Manishearth previously approved these changes Aug 12, 2022
))
}

// No docs because the "experimental" version is what docs use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: worth noting the optional SkeletonsPattern bound in the docs of the other one ("The X bound is only needed with the experimental feature and enables blah blah")

Manishearth
Manishearth previously approved these changes Aug 12, 2022
Manishearth
Manishearth previously approved these changes Aug 13, 2022
@sffc
Copy link
Member Author

sffc commented Aug 16, 2022

As a side-effect, note how provider/datagen/tests/data/work_log.wasm got substantially smaller across this change, since it doesn't need to carry skeleton resolution logic any more. When we re-enable it, we should try to make the binary size impact as small as possible.

@sffc
Copy link
Member Author

sffc commented Aug 16, 2022

@zbraniecki The bench build is failing because the benches currently require components bag. If I do like I did in the tests where I skip over the components tests when the feature is not enabled, then the benches are going to have different results depending on what features are enabled, since enabling the experimental feature makes the benches run more examples. I could:

  1. Make the affected benches require the "experimental" feature
  2. Decide that it is fine for the benches to have different perf characteristics based on what features are turned on
  3. Block this PR until I have time to split the benches into experimental and non-experimental versions, and then make a smaller subset require the "experimental" feature

@sffc
Copy link
Member Author

sffc commented Aug 16, 2022

I think there are two issues remaining before CI is green:

  1. The issue about datetime benches discussed above
  2. How do we make testdata build properly when the experimental feature is disabled? We can't sprinkle #[cfg] everywhere because this is generated code, unless we change the generating code.

@sffc
Copy link
Member Author

sffc commented Aug 16, 2022

I resolved the two issues above by:

  1. Turns out there is only one bench that requires components data, "datetime/datetime_components". I simply put this one behind a #[cfg].
  2. I made icu_testdata/baked depend on icu_datetime/experimental. Longer term I hope we can do Allow icu_testdata's BakedDataProvider to be broken down by component and experiment #2383.

@sffc sffc merged commit a6987d1 into unicode-org:main Aug 16, 2022
@sffc
Copy link
Member Author

sffc commented Aug 16, 2022

Yippee! Merged.

@sffc sffc deleted the experimental-skeletons branch August 16, 2022 23:33
@zbraniecki
Copy link
Member

Turns out there is only one bench that requires components data, "datetime/datetime_components". I simply put this one behind a #[cfg].

That's what I would suggest. All good!

robertbastian pushed a commit to robertbastian/icu4x that referenced this pull request Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put date skeletons behind a feature
4 participants