Skip to content

First part of glue code for HarfBuzz #2839

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

Merged
merged 14 commits into from
Feb 27, 2023
Merged

Conversation

hsivonen
Copy link
Member

Towards #2514.

I have reserved the crate name: https://crates.io/crates/icu_harfbuzz

@hsivonen hsivonen requested a review from echeran November 22, 2022 07:57
@hsivonen hsivonen requested a review from a team as a code owner November 22, 2022 07:57
@hsivonen
Copy link
Member Author

Follow-ups: #2840, #2833, #2832, #2838.

@sffc sffc self-requested a review December 14, 2022 09:30
sffc
sffc previously approved these changes Dec 19, 2022
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.

I reviewed this with @echeran. Looks quite good; thanks for figuring out how to do these bindings!

@Manishearth
Copy link
Member

So at a high level this looks really great!

One thing I'm wondering about is whether this should go into the metacrate. I kind of feel that this isn't an ICU4X component as much as it is an ICU4X integration (or "port"). A lot of the stuff under ffi/ serves this purpose (but in a more general way), I wonder if it's time we had a new "ports" folder for things that use ICU4X to do new exciting things in ways that are themselves crateworthy.

I also don't want a native dependency in our default deptree 😄 We can play around with that, of course.

@sffc
Copy link
Member

sffc commented Jan 10, 2023

Agree that it would be nice to avoid native deps in the icu metacrate. I think this should just be a standlone crate.

@hsivonen
Copy link
Member Author

hsivonen commented Jan 13, 2023

Agree that it would be nice to avoid native deps in the icu metacrate. I think this should just be a standlone crate.

Concretely, how should I place this crate in the repo's directory structure?

@sffc
Copy link
Member

sffc commented Feb 2, 2023

Agree that it would be nice to avoid native deps in the icu metacrate. I think this should just be a standlone crate.

Concretely, how should I place this crate in the repo's directory structure?

Discussed in today's call. It should be ffi/harfbuzz eventually, or you can start it as experimental/harfbuzz. It's just that it can be a separate crate icu_harfbuzz without being plugged into the icu metacrate.

@sffc
Copy link
Member

sffc commented Feb 4, 2023

@hsivonen What's the status of this PR? Are you waiting on reviewers or is there more you plan to do first?

@sffc
Copy link
Member

sffc commented Feb 23, 2023

Discussion: put this in the workspace but not the metacrate.

@hsivonen
Copy link
Member Author

I have both 1) removed icu_harfbuzz from the metacrate and 2) turned off the default features of harfbuzz-sys to build Harfbuzz itself and Freetype.

@hsivonen hsivonen requested a review from sffc February 24, 2023 14:24
@hsivonen
Copy link
Member Author

Apparently tests need Harfbuzz itself to be built.

@hsivonen hsivonen requested a review from sffc February 27, 2023 15:56
@hsivonen hsivonen merged commit 22337d6 into unicode-org:main Feb 27, 2023
@hsivonen hsivonen deleted the harfbuzz branch February 27, 2023 19:09
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.

3 participants