search

linebender / norad

Rust crate for working with Unified Font Object files
Apache License 2.0
43 stars 12 forks source link

updating to quick-xml 0.31 breaks our lib deserialization #330

Closed cmyr closed 11 months ago

cmyr commented 1 year ago

this has been an unexpected rabbit hole, but basically: if we want to upgrade to the latest quick-xml, we need to figure out how to get our custom deserialization code working again, which I've just burnt a couple of hours on.

RickyDaMa commented 1 year ago

Changelog link

Happy to take a look at the designspace stuff when I get some time. Such is the nature of pre-v1 dependencies I get. Is there any pressing need to get onto the latest version, or just "it'd be nice to keep dependencies up-to-date"?

cmyr commented 1 year ago

it would reduce upstream duplication of dependencies, but isn't necessary. I'll spend a bit more time today and see if I'm close to having a fix, otherwise we can defer.

RickyDaMa commented 1 year ago

If you don't many any progress, let me know, I can take a look as well

cmyr commented 1 year ago

I have not continued trying to hack at this, feel free to take a look if you're feeling adventurous/masochistic 💁‍♂️

RickyDaMa commented 1 year ago

Had a couple tries at this over the last two days, and wow I see why you gave up. I couldn't get anything to even start deserialising, just immediately hitting errors at every turn

Maybe we could reach out on the quick_xml repo and see if they'd be willing to help or give us a starting point on where I/we are going so terribly wrong. Reading the changelog on their repo even I don't understand what's broken our code so badly, unless we were relying on apparently bad behaviour

Let me know your thoughts!

rsheeter commented 11 months ago

Unable to update quick-xml seems like a bad state to stall in :(

quick-xml does seem to have fiddled with the relevant parts, for example https://github.com/tafia/quick-xml/commit/b7787b0fa35fd8dd0279271430db868776172594 looked suspicious to me. Sadly reverting it did not fix the problem. My default assumption would be it's a problem with our deserializer, but I couldn't immediately spot anything suspicious.

IMHO a good next step would be to try to narrow it down to a simpler reproduction than norad using plist and quick-xml together. For example, can we make a standalone repo with a simple data structure that fails on quick-xml 0.31.0 and works on 0.30.0? - if so, we should report it as a bug. EDIT: https://github.com/tafia/quick-xml/issues/580 has an example of a similar issue.

EDIT2: testing with local quick-xml points to the commit referenced above, merged in https://github.com/tafia/quick-xml/pull/662

cargo test empty_array_is_a_okay:

rsheeter commented 11 months ago

Copying from IM to here so it doesn't get lost, another path would be to use quick-xml but not serde, just handwrite an event handling parser in The Old Style. Tiresome but simple and unlikely to break.

@cmyr mentioned glif files already work this way.