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] Revert fontique dep roxmltree to 0.18.1 #43

Conversation

waywardmonkeys
Copy link
Contributor

The update to 0.19.0 broke the masonry tests in the xilem repo on Ubuntu.

The update to 0.19.0 broke the masonry tests in the xilem repo
on Ubuntu.
@waywardmonkeys
Copy link
Contributor Author

This reverts #42.

@DJMcNab
Copy link
Member

DJMcNab commented Apr 26, 2024

This is a not a very compelling fix. Can you test linebender/xilem#221 from 9a519ba instead, which should have this same tree.

I would much prefer us to just fix this failure, if at all possible. cc @xorgy

@waywardmonkeys
Copy link
Contributor Author

I bisected the tree by running it in CI outside of the Linebender org (my own fork). The memmap2 update and previous commits are okay. Only this last one that updated roxmltree makes things sad.

It does show that we really need actual tests for things.

Copy link
Collaborator

@dfrg dfrg 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 confused as to why updating an xml dependency would break tests. Unfortunately, I probably won’t have time to investigate this today.

@waywardmonkeys
Copy link
Contributor Author

It was several tests failing with:

---- widget::align::tests::centered stdout ----
thread 'widget::align::tests::centered' panicked at /home/runner/.cargo/git/checkouts/parley-7d89eb78db3c5efe/9e2be05/src/layout/line/greedy.rs:516:37:
range end index 1 out of range for slice of length 0

And that line of code was:

    for (i, run_data) in layout.runs[state.runs.clone()].iter().enumerate() {

My guess is that the fontconfig failed to load and then things went wrong after that.

@dfrg
Copy link
Collaborator

dfrg commented Apr 26, 2024

At a glance, this might have to do with changes to DTD parsing in roxmltree. Changing the fontconfig backend to use Document::parse_with_options with allow_dtd set to true might fix it. If not, we’ll likely need to consider a different xml library.

@dfrg
Copy link
Collaborator

dfrg commented Apr 26, 2024

The specific line is here:

let Ok(doc) = roxmltree::Document::parse(&text) else {

@dfrg
Copy link
Collaborator

dfrg commented Apr 26, 2024

This seems to fix it. See #44 and linebender/xilem#224

@waywardmonkeys
Copy link
Contributor Author

This isn’t needed now!

@waywardmonkeys waywardmonkeys deleted the fontique-revert-roxmltree-update branch April 27, 2024 10: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