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

Switch to a submodule for harfbuzz. #205

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

waywardmonkeys
Copy link
Collaborator

This simplifies updating as well as reducing the amount of churn in this repo when updating.

This doesn't change the version used or anything yet, it should just be a straight swap from extracted sources to using a submodule.

@waywardmonkeys
Copy link
Collaborator Author

I guess this is at least good for discussion ... I think this is the right direction to take.

We'll have to fix the one CI task though.

@waywardmonkeys waywardmonkeys mentioned this pull request Aug 17, 2023
@mrobinson
Copy link
Member

It looks like the CI error is:

Run make -f harfbuzz-sys/makefile.touch touch HARFBUZZ=harfbuzz-sys/harfbuzz
harfbuzz-sys/makefile.touch:9: *** 'configure' not found in path given by $HARFBUZZ: harfbuzz-sys/harfbuzz.  Stop.

Does the build script need to run autoconf?

@waywardmonkeys
Copy link
Collaborator Author

In theory, we have to switch to running meson to do that build.

But that build is used so that we can test linking against harfbuzz on the system and controlling what version it is.

An alternative is that we just link against what is on the system and not always provide new API. ubuntu-latest provides harfbuzz 2.7.4, I think ... same as what we're currently bundling.

on macOS, we use what is in homebrew, which tends to be fairly current.

on Windows, we don't perform this action and only test using the bundled one in our CI, I think.

@waywardmonkeys
Copy link
Collaborator Author

My plan was to circle back around to this once we had features re-worked and other things improved ... as that helps make it more clear what exactly we're checking in each scenario.

The other harfbuzz bindings always essentially copy our existing logic: Unless a special env var is set, look for one on the system and use it, otherwise, build the bundled copy.

But it doesn't bother testing nearly as much in CI.

@waywardmonkeys
Copy link
Collaborator Author

@mrobinson This is ready now!

This still keeps us on harfbuzz 2.7.4 for now ... just doing the switch of how we get the sources. After that, we can do the actual update to a newer version for our bundled builds. :)

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

This is a really nice change. The one I want to suggest here is a small note in the README letting folks know that they should do a git submodule init when checking out the source.

This simplifies updating as well as reducing the amount of churn
in this repo when updating.
@waywardmonkeys
Copy link
Collaborator Author

@mrobinson Okay, update to the README.md added.

@waywardmonkeys
Copy link
Collaborator Author

@jdm Could you land this one too while you are at it, please?

@mrobinson mrobinson added this pull request to the merge queue Aug 25, 2023
Merged via the queue into servo:master with commit 3552a2e Aug 25, 2023
8 checks passed
@waywardmonkeys waywardmonkeys deleted the switch-to-submodule branch August 25, 2023 09:45
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.

2 participants