Skip to content

Port xetex_layout to Rust - #1138

Merged
CraftSpider merged 107 commits into
tectonic-typesetting:masterfrom
CraftSpider:xetex-layout-port
Aug 7, 2025
Merged

Port xetex_layout to Rust#1138
CraftSpider merged 107 commits into
tectonic-typesetting:masterfrom
CraftSpider:xetex-layout-port

Conversation

@CraftSpider

@CraftSpider CraftSpider commented Feb 5, 2024

Copy link
Copy Markdown
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.

Merge Readiness:

  • Split tests into separate PR, merge first
  • Drop test-only changes
    • CI changes
    • ICU bridge changes
  • Split each bridge's Rust interface into its own PR
    • Create fontconfig bridge
    • Harfbuzz
    • Graphite
    • Freetype
    • Mac Core
  • Ensure tectonic-on-arXiv works on the final result

@pkgw

pkgw commented Feb 5, 2024

Copy link
Copy Markdown
Collaborator

Ooh, very exciting!

Comment thread crates/xetex_layout/src/lib.rs Outdated
@CraftSpider

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread crates/bridge_freetype2/src/lib.rs Outdated
@CraftSpider

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@codecov

codecov Bot commented Feb 9, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 58.64258% with 847 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.84%. Comparing base (fe11279) to head (bc933fa).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
crates/xetex_layout/src/manager.rs 39.05% 324 Missing and 13 partials ⚠️
crates/xetex_layout/src/c_api/engine.rs 62.74% 151 Missing and 1 partial ⚠️
crates/xetex_layout/src/font.rs 64.84% 116 Missing and 6 partials ⚠️
crates/xetex_layout/src/manager/fc.rs 64.32% 65 Missing and 6 partials ⚠️
crates/xetex_layout/src/c_api/font.rs 51.38% 70 Missing ⚠️
crates/xetex_layout/src/engine.rs 82.69% 22 Missing and 23 partials ⚠️
crates/engine_xetex/xetex/xetex-ext.c 50.00% 7 Missing and 9 partials ⚠️
crates/xetex_layout/src/c_api/manager.rs 71.42% 13 Missing and 1 partial ⚠️
crates/xetex_layout/src/c_api.rs 56.66% 13 Missing ⚠️
crates/xetex_layout/src/utils.rs 60.00% 4 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   28.59%   28.84%   +0.24%     
==========================================
  Files         272      273       +1     
  Lines      203632   203491     -141     
  Branches   203632   203491     -141     
==========================================
+ Hits        58236    58701     +465     
- Misses      64049    64159     +110     
+ Partials    81347    80631     -716     
Components Coverage Δ
tectonic 50.89% <ø> (ø)
bridge_core 34.98% <100.00%> (+0.24%) ⬆️
bridge_flate 38.82% <ø> (ø)
bridge_fontconfig 96.17% <ø> (+13.11%) ⬆️
bridge_freetype2 92.70% <ø> (+0.70%) ⬆️
bridge_graphite2 42.47% <ø> (+41.93%) ⬆️
bridge_harfbuzz 82.83% <ø> (+11.97%) ⬆️
bridge_icu ∅ <ø> (∅)
bridge_png 100.00% <ø> (ø)
bundles 34.78% <ø> (ø)
cfg_support 94.84% <ø> (ø)
dep_support 0.00% <ø> (ø)
docmodel 84.09% <ø> (ø)
engine_bibtex 66.45% <ø> (ø)
engine_spx2html 0.00% <ø> (ø)
engine_xdvipdfmx 55.58% <ø> (ø)
engine_xetex 35.28% <48.64%> (-0.01%) ⬇️
errors 47.61% <ø> (ø)
geturl 27.16% <ø> (ø)
io_base 76.04% <ø> (ø)
pdf_io 14.29% <ø> (-0.03%) ⬇️
status_base 71.76% <ø> (ø)
xdv 55.21% <ø> (ø)
xetex_format 22.80% <ø> (ø)
xetex_layout 58.62% <58.60%> (+15.80%) ⬆️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@CraftSpider

Copy link
Copy Markdown
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

pkgw commented Feb 10, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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

pkgw commented Mar 26, 2024

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
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
Copy Markdown
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
@CraftSpider
CraftSpider marked this pull request as ready for review August 6, 2025 20:56
@CraftSpider

Copy link
Copy Markdown
Contributor Author

Did a last pass over the code to clean up privacy, and improve readability/quality of a couple minor spots. This should now be ready to merge, soon as ToA approves it.

@CraftSpider

Copy link
Copy Markdown
Contributor Author

Alright, after just about a year and a half, I'm going to merge this. It passes ToA and all tests, contains fairly idiomatic Rust in a lot of places, and generally should be a notable improvement on the C/C++ code it replaces in terms of both maintainability and leak safety (I'm pretty sure this patch fixes 1-2 leaks already).

@CraftSpider
CraftSpider merged commit e8dd681 into tectonic-typesetting:master Aug 7, 2025
29 checks passed
@CraftSpider
CraftSpider deleted the xetex-layout-port branch May 9, 2026 00:33
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.

5 participants