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

Port xetex_layout to Rust #1138

Draft
wants to merge 137 commits into
base: master
Choose a base branch
from

Conversation

CraftSpider
Copy link
Contributor

Been poking this on and off for a while, finally reached the point where it's... not done, but should pass most tests on Windows and hopefully Linux. Mac will require a bit more love before it works. Opening to see what CI has to say so far.

@pkgw
Copy link
Collaborator

pkgw commented Feb 5, 2024

Ooh, very exciting!

@@ -27,6 +27,175 @@ mod linkage {
use tectonic_bridge_icu as clipyrenamehack5;
}

macro_rules! cstr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi. CStr literals have been stabilized recently so they'd fit in nicely 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.

It looks like they were delayed by a release, so they're stabilizing in 1.77, the release after the one coming out in a couple days. I'll leave a TODO to fix it then, if this merges before that release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh whoops, right. "Tracking issue closed as stabilized" doesn't mean that the version is stable already ^^

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, we don't have a formal Minimum Supported Rust Version (MSRV) policy, but I think it's generally good to give new stabilizations at least a few features to propagate through the ecosystem before starting to rely on them.

@CraftSpider
Copy link
Contributor Author

I am so baffled by the linking errors on everything - they aren't happening locally, which makes them pretty hard to diagnose.

@Mrmaxmeier
Copy link
Contributor

Yep, I saw undefined reference to symbol 'BrotliDecoderDecompress' in my CI logs and was also wondering what that's about. Probably some strange interaction with dependencies.. (?) I'll try poking at the CI container a bit.

pub type FT_Generic_Finalizer = extern "C" fn(object: *mut ());
pub type FT_Pointer = *mut ();

#[link(name = "freetype", kind = "static")]
Copy link
Contributor

@Mrmaxmeier Mrmaxmeier Feb 8, 2024

Choose a reason for hiding this comment

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

It seems like this directive links to a second (static) copy of freetype that doesn't match up with our harfbuzz freetype build. I'm not sure of the consequences but simply dropping the #[link] line both here and in crates/bridge_harfbuzz/src/lib.rs seems to work for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good find, I'll try it locally. I'm using a Windows vcpkg dev environment, and it needs some stuff static so I suspect I added it while narrowing down another issue, and either it's only necessary in some cases, or it's not necessary at all and I just left it in by accident.

@CraftSpider
Copy link
Contributor Author

The icu/unicode stuff is probably because the library does symbol versioning, which you only disable normally on say system installs. I probably, somehow, am locally getting some system installed version pulled in which is providing the symbols. This is... kind of annoying, but I can fix it.

@CraftSpider
Copy link
Contributor Author

Found a solution but it will require updates to vcpkg-rs - that PR will go up tonight or tomorrow, probably.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Attention: Patch coverage is 65.29370% with 969 lines in your changes missing coverage. Please review.

Project coverage is 46.93%. Comparing base (c2ae25f) to head (fc66480).

Files with missing lines Patch % Lines
crates/xetex_layout/src/manager.rs 45.19% 234 Missing ⚠️
crates/xetex_layout/src/c_api/engine.rs 60.86% 108 Missing ⚠️
crates/bridge_graphite2/src/lib.rs 39.53% 104 Missing ⚠️
crates/xetex_layout/src/font.rs 65.31% 103 Missing ⚠️
crates/bridge_freetype2/src/lib.rs 69.02% 70 Missing ⚠️
crates/xetex_layout/src/manager/fc.rs 66.66% 58 Missing ⚠️
crates/xetex_layout/src/c_api/font.rs 53.44% 54 Missing ⚠️
crates/bridge_harfbuzz/src/font_funcs.rs 73.36% 49 Missing ⚠️
crates/bridge_harfbuzz/src/lib.rs 85.71% 37 Missing ⚠️
crates/bridge_freetype2/src/sys.rs 14.70% 29 Missing ⚠️
... and 14 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   46.35%   46.93%   +0.58%     
==========================================
  Files         184      201      +17     
  Lines       66222    67768    +1546     
==========================================
+ Hits        30695    31808    +1113     
- Misses      35527    35960     +433     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CraftSpider
Copy link
Contributor Author

I should look into whether there's an existing CoreFoundation/CoreText library for mac. Since they're, well, core, it should be possible to use a standard sys crate for them without jumping through the hoops other dependencies need due to supporting so many ways of handling them.

@pkgw
Copy link
Collaborator

pkgw commented Feb 10, 2024

There's https://github.com/servo/core-foundation-rs ... the crates don't seem as popular as I'd have thought, but a Servo project is probably going to be trustworthy?

@rm-dr rm-dr added the port-to-rust Replacing old code with Rust label Feb 27, 2024
@CraftSpider
Copy link
Contributor Author

@pkgw What's your take on dropping support for MacOS < 10.7, OSX Lion? It's more than 10 years old, and some quick googling says the number of mac users on it should be basically negligible. It would allow some minor simplification.

@CraftSpider
Copy link
Contributor Author

Related note - the core-foundation-rs crate and similar from servo target 10.7 and above, so it would allow using them.

@pkgw
Copy link
Collaborator

pkgw commented Mar 26, 2024

@CraftSpider Dropping support for the older macOS seems fine to me. It generally becomes intractable to support the older OSes without paying for your own custom CI setup, anyway. It looks like conda-forge is currently targeting >=10.9, and I'd generally lean towards copying them.

@CraftSpider CraftSpider force-pushed the xetex-layout-port branch 3 times, most recently from f5ad6d0 to ee6af21 Compare March 29, 2024 18:46
@CraftSpider
Copy link
Contributor Author

This is a big change - if desired, I can split it out into the primary change to Rust, then the individual library wrappers as follow-ups to the big change, as I made sure to get tests passing between each major change.

@CraftSpider CraftSpider force-pushed the xetex-layout-port branch 2 times, most recently from f8dacb6 to d8a9393 Compare April 17, 2024 22:24
@CraftSpider
Copy link
Contributor Author

Obviously I'm biased, but I'm tempted to pull in my crate enrede to replace icu at some point. It means both one fewer allocation for recoding strings, but also would allow replacing most usages of the std CString with enrede's CString<Utf8>, an encoding-strict type that is null terminated, but safe to turn to a &str for easy use in Rust code.

@CraftSpider CraftSpider force-pushed the xetex-layout-port branch 2 times, most recently from ec9e1d4 to 5d3570a Compare August 9, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port-to-rust Replacing old code with Rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants