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

deps: Upgrade freetype-sys to version 0.21 #258

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Conversation

LeoDog896
Copy link
Contributor

@LeoDog896 LeoDog896 commented Jun 16, 2024

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.

I'm not sure that bundling HarfBuzz should imply that FreeType is automatically bundled as well. Perhaps a user only wants to bundle HarfBuzz, but use the system FreeType. This change would make that impossible.

@waywardmonkeys
Copy link
Collaborator

Also, IIRC, if you are building harfbuzz with freetype support, it needs the freetype headers, which this wouldn't provide for the build, right?

@waywardmonkeys
Copy link
Collaborator

I think that's why I filed #235.

@LeoDog896
Copy link
Contributor Author

LeoDog896 commented Jun 16, 2024

I'm unaware of how to prevent the freetype library from being bundled twice. It only includes the headers 😅 this should be fine then.

optional = true

[features]
default = ["coretext", "directwrite", "freetype"]
bundled = []
freetype-bundled = ["freetype-sys/bundled"]
Copy link
Member

Choose a reason for hiding this comment

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

I don't thin you need another feature for this. Features are additive, so one should just need to make sure that freetype-sys is has the bundled feature when compiling. This is something that someone using rust-harfbuzz can do without having to expose a new feature here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the freetype-bundled feature be exposed in the harfbuzz wrapper only then, since features of subdependencies can't be specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe it's possible in CLI to enable features of sub dependencies (it is possible in Cargo.toml, though!)

@LeoDog896 LeoDog896 marked this pull request as draft June 16, 2024 19:09
@LeoDog896 LeoDog896 marked this pull request as ready for review June 16, 2024 19:26
@LeoDog896
Copy link
Contributor Author

Since the packaged freetype version on Ubuntu 20 is 2.11.1 which is lower than the required freetype version for freetype-sys, it should be bundled on CI - though, should it be this CI's responsibility to test freetype from the system rather than its default bundling behavior?

@LeoDog896 LeoDog896 requested a review from mrobinson June 16, 2024 20:39
@mrobinson mrobinson changed the title fix: propagate bundled option to freetype-sys Upgrade freetype-sys to version 0.21 Jun 17, 2024
@mrobinson mrobinson force-pushed the bundle branch 2 times, most recently from 230080a to a544953 Compare June 17, 2024 09:24
@mrobinson
Copy link
Member

mrobinson commented Jun 17, 2024

I've modified this branch to simply update to version 0.21 of freetype-sys. Changes:

  • Use the ubuntu-24.04 GitHub image. The issue here is that the version of FreeType used in ubunut-latest is too old. In addition, the FreeType development package needs to be installed.
  • Add the freetype-sys/bundled feature when necessary. It seems that FreeType is used by default even on platforms where it likely doesn't make sense (Windows).
  • Added a line in the README explaining that this may be necessary.

@mrobinson mrobinson changed the title Upgrade freetype-sys to version 0.21 deps: Upgrade freetype-sys to version 0.21 Jun 17, 2024
@mrobinson mrobinson added this pull request to the merge queue Jun 17, 2024
Merged via the queue into servo:main with commit 0a7ddc4 Jun 17, 2024
8 checks passed
@LeoDog896
Copy link
Contributor Author

Oh! I did not know that feature syntax was possible - though, how should #235 be done?

@mrobinson
Copy link
Member

Oh! I did not know that feature syntax was possible - though, how should #235 be done?

This needs a bit of exploration I think. I'm sure other crates have run into this issue and we should check what they do.

@narodnik
Copy link

narodnik commented Jun 24, 2024

I had to patch harfbuzz_rs to do https://github.com/narodnik/hbrs2/blob/master/build.rs#L22

cfg.define("HAVE_FREETYPE", "1");

Right now this is a blocker for me to switch to harfbuzz_sys

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.

4 participants