Skip to content

Move harfbuzz-traits implementations into component crates #7200

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 3, 2025
Merged

Move harfbuzz-traits implementations into component crates #7200

merged 2 commits into from
Nov 3, 2025

Conversation

@robertbastian
Copy link
Member

@robertbastian robertbastian commented Oct 29, 2025

icu_properties already does this for unicode-bidi. The current icu_harfbuzz crate is written for use with the harfbuzz crate, however there are other Rust HarfBuzz implementations that don't need the code in that crate.

sffc
sffc reviewed Oct 29, 2025
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Seems good to eliminate a leaf crate. Can we avoid growing the boilerplate in examples/cargo/harfbuzz?

@robertbastian
Copy link
Member Author

robertbastian commented Oct 30, 2025

the boilerplate in the example is additional example code for using buffer providers, as well as the ZST box optimization that not relevant to all HarfBuzz implementations (the harfbuzz crate needs it because it needs to box over FFI).

@robertbastian robertbastian marked this pull request as ready for review October 30, 2025 17:44
@robertbastian robertbastian requested review from sffc and removed request for echeran October 30, 2025 17:44
Manishearth
Manishearth reviewed Oct 31, 2025
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Overall in favor of this!

@hsivonen wrote this, we should probably wait for him to check in

Manishearth
Manishearth approved these changes Oct 31, 2025
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Would still prefer that @hsivonen sign off on this being removed.

@hsivonen
Copy link
Member

hsivonen commented Nov 3, 2025

Moving the trait implementations seems OK.

It would be good to publish a tombstone release of icu_harfbuzz (with [badges] maintenance = { status = "deprecated" }) to crates.io when when icu_properties and icu_normalizer with this change reach crates.io.

@Manishearth
Copy link
Member

Manishearth commented Nov 3, 2025

That's a good point. @robertbastian can you do that after merging this?

(my r+ stands, feel free to merge)

@robertbastian
Copy link
Member Author

robertbastian commented Nov 3, 2025

I filed #7210 because we can only do this after 2.2.

@robertbastian robertbastian merged commit 2aaac23 into unicode-org:main Nov 3, 2025
54 of 57 checks passed
@robertbastian robertbastian deleted the harfbuzz branch November 3, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Manishearth Manishearth Manishearth approved these changes

@hsivonen hsivonen Awaiting requested review from hsivonen hsivonen is a code owner

@sffc sffc Awaiting requested review from sffc sffc is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@robertbastian @hsivonen @Manishearth @sffc