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

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

Closed
cmyr opened this issue Oct 26, 2023 · 7 comments · Fixed by #337
Closed

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

cmyr opened this issue Oct 26, 2023 · 7 comments · Fixed by #337

Comments

@cmyr
Copy link
Member

cmyr commented Oct 26, 2023

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

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
Copy link
Member Author

cmyr commented Oct 27, 2023

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

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

@cmyr
Copy link
Member Author

cmyr commented Nov 2, 2023

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

@RickyDaMa
Copy link
Collaborator

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

rsheeter commented Nov 28, 2023

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 tafia/quick-xml@b7787b0 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: tafia/quick-xml#580 has an example of a similar issue.

EDIT2: testing with local quick-xml points to the commit referenced above, merged in tafia/quick-xml#662

cargo test empty_array_is_a_okay:

@rsheeter
Copy link
Collaborator

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.

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 a pull request may close this issue.

3 participants