drobilla / serd

A lightweight C library for RDF syntax
https://gitlab.com/drobilla/serd
ISC License
88 stars 16 forks source link

Version >= 0.30.16 writes faulty syntax for tuples in TTL files #43

Closed SpotlightKid closed 10 months ago

SpotlightKid commented 10 months ago

To reproduce:

  1. jalv.gtk3 http://lsp-plug.in/plugins/lv2/impulse_responses_mono
  2. Load an IR file.
  3. Create a new preset.
  4. Close jalv and restart with above command.
  5. Try to load preset. It will not load correctly, i.e. the IR file will not be loaded.

If you examine the <name>.ttl file in the preset bundle, you'll find something like this at the end of the file:

...
state:state [
        <[http://lsp-plug.in/plugins/lv2/impulse_responses_mono/KVT>](http://lsp-plug.in/plugins/lv2/impulse_responses_mono/KVT%3E) [
            a atom:Tuple ;
            rdf:value ()
        ]<[http://lsp-plug.in/plugins/lv2/impulse_responses_mono/ports#ifn>](http://lsp-plug.in/plugins/lv2/impulse_responses_mono/ports#ifn%3E) <my-ir.wav> .

... which is not valid syntax. It should be

...
state:state [
    <http://lsp-plug.in/plugins/lv2/impulse_responses_mono/KVT> [
        a atom:Tuple ;
        rdf:value ()
    ] ;
    <http://lsp-plug.in/plugins/lv2/impulse_responses_mono/ports#ifn> <my-ir.wav>
] .

I bisected this by going back to older version of serd one-by-one. I use Arch (Manjaro), which currently has serd 0.32.0, so I tested 0.30.16 and 0.30.14. The latter works fine, while 0.30-16 exhibits the same faulty behavior as 0.32.0.

This of course affects not only jalv, but all hosts, which use the system serd lib to write presets or plugin state in their projects (e.g. Ardour from the Arch packages too).

drobilla commented 10 months ago

Thanks for bisecting. Odd, 0.30.16 was just a maintenance release, almost no significant changes to the code at all. I'll look into it.

drobilla commented 10 months ago

Thanks for bisecting. Odd, 0.30.16 was just a maintenance release, almost no significant changes to the code at all. I'll look into it.

Turns out this was wrong (but close), probably you have older installed versions lying around which got picked up by the linker instead. I confused myself in the same way.

The actual offending commit is f43066a36 (a backport of some writer improvements from the 1.x branch as part of my efforts to make that transition as graceful as possible), pre 0.32.0.

Oops. Missing coverage of some case or another (probably an empty list as the last object in a blank node or something), despite the ridiculously comprehensive test suite. I'll plug that hole and fix the issue so it can never happen again.

SpotlightKid commented 10 months ago

Sorry for leading you on the wrong path. I'm not sure why 0.32.14 worked for me and 0.32.16 did not. I checked and - apart from the copy in the Ardour Demo bundle at /opt/Ardour-8.2.0-demo/lib/libserd-0.so.0, which whould not have interfered - I don't have another copy of libserd lying around.

Anyway, glad you found the culprit.

drobilla commented 10 months ago

No worries, I'd have to narrow it down beyond that regardless. Regressions are so uncommon here I probably wouldn't have thought to start there if you hadn't already.

In any case, it doesn't seem to be reproducible by feeding ttl to serd, so it's something weird in how it's being called in code (e.g., serd's parser does the right thing, but sratom/lilv doesn't, or something like that) which is going to be a bit annoying to minimally reproduce but I'll figure it out.

Unfortunate that there's been a minor version bump in the meantime, but I'll kick out a point release as soon as I can.

drobilla commented 10 months ago

Fixed in 91786a70e