yamlchan: Add libfyaml support as an alternative to libyaml - #67
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #67 +/- ##
==========================================
+ Coverage 61.42% 61.46% +0.04%
==========================================
Files 83 84 +1
Lines 96467 96545 +78
Branches 30582 30597 +15
==========================================
+ Hits 59253 59340 +87
+ Misses 21066 21055 -11
- Partials 16148 16150 +2 ☔ View full report in Codecov by Harness. |
d717399 to
8a9db61
Compare
|
Since the YAML handling is mostly motivated by the ASDF needs I don't have a problem with adding support for a new YAML library. If I was a packager I don't really understand why I would have to build both variants -- wouldn't I always choose fyaml? There is a conda-forge package of AST which would need tweaking if we switched YAML library. We picked libyaml originally because we didn't really know any better. |
Possibly. Right now the only reason to do so would be my specific use-case really, so it has to be an informed choice.
I'd be more fair to yourself than that :) At the time libfyaml was quite new and not at all mature yet (its first release on github was only in 2019). Depending on how dependent upstream users are on byte-for-byte identical outputs from AST, it might still matter, as there are cosmetic differences in what the two libraries output. I actually find that some of libfyaml's default output formatting is a bit ugly (compromises it makes, I think, as a performance tradeoff). For example, even short flow-style sequences like which I find a bit annoying. Of course, for the event stream interface doing this requires some buffering/look-ahead of events. Those cosmetic output differences are my only concern though. I also plan when I have time to propose some fixes to libfyaml for these formatting issues. I think some options to tweak this might be acceptable, where performance is less of a concern--its maintainer has been amenable to some of my past suggestions. |
- Starlink/ast#37 - Starlink/ast#51 - Starlink/ast#66 - Starlink/ast#67 This includes the experimental SIMD support (enabled by default, and support in AST for libfyaml, allowing us to drop the libyaml requirement). Also updates the minimum supported versions of the ASDF tags supported by AST.
|
The ASAN build is revealing a number of memory leaks. Strangely some of them don't seem directly related to this branch, while others do, which suggests to me that the correct flags aren't being passed to ASAN to cause tests to halt, but I'll have to have a look at that. At least one of the leaks is a bug in libfyaml itself, fixed in v0.9, though Ubuntu <= 24.04 has a version of libfyaml with this bug and it doesn't appear to be patched. I'll keep investigating but that one might not be anything that can be easily worked around and should maybe just be documented. It's a small leak that only occurs while writing. |
|
Interesting. As I have expanded test coverage I have found some ASAN issues. Sometimes it's the system not including an astBegin/astEnd in the right place but others have been real. So you are saying that some of these are in general mapping code? |
No, just some specific to |
|
Fixed all leaks exposed in (the error in the output comes from one of the tests and is expected) |
b31aba1 to
8f782dc
Compare
This adds a new abstracted interface for the underlying YAML library used for YamlChan. It hews close to the original libyaml API with a few additions to smooth over differences between libraries, and adds support for libfyaml as an alternative backend. The build system supports using either libyaml or libfyaml. It is possible to specify one of the other explicitly, otherwise the build system will select one automatically, with preference given to libyaml for compatibility's sake, as there are slight differences in libyaml and libfyaml outputs (both still equivalent YAML though). This commit is a squashed commit of several WIP commits on this. wip: style tweaks wip: interface cleanup of libyaml vs libfyaml minor differences build: support libfyaml as alternative yaml backend at configure time Both cmake and autotools build get support: auto (the default) uses the first YAML library detected, preferring libyaml at first (for backwards-compat) and falling back on libfyaml. Both cases also allow an explicit backend (yaml or fyaml) to be specified. For the tests, decided to keep testyamlchan.c compiling even if there is no backend supported, in case we also want to add tests that work even in this case (even just testing the appropriate errors raised) wip: several small improvments and bug fixes, esp. in error handling - Clean up the error handling situation; rather than trying to directly smudge the interface into the previous libyaml error handling, produce equivalent sensible error messages from both backends (libfyaml doesn't have numeric error codes like libyaml so that details is folded into the libyaml error messages). The exact error messages between libraries are of course not exactly the same, but the format of the error messages in AST is made similar between the two, including correct line/column number context (which was not handled well to begin with). - Add a provisional test for error handling; could still be improved. - Smooth over differences between backends w.r.t. how node output styles are handled, by intrducing a new common style enum that abstracts out what's supported by each backend. - Fixes to the libfyaml backend write callback: libfyaml calls the emitter callback once *per token*, but our write callback is expected to produce full lines (without terminating newline), so _astFyamlWriteAdapter now maintains an internal line buffer to account for that, which is a bit more overcomplication than I'd like... wip: fix emission of explicit str tags for YAML strings There are still some issues with how tagged scalars and flow sequences are emitted by libfyaml but that seems to be more a libfyaml limitation that needs to poked at upstream... wip: improve the test to check the exact error message and line no Previously had a bug with the line numbering in libfyaml so this would catch that.
Not sure if we want to add yet two more build variants to the CI but it would be necessary for maximum code coverage. Also worth doing on both mac and linux because the homebrew coverage for libfyaml tends to be at the bleeding edge, while the Ubuntu version is older and more conservative.
- Adds the %YAML directive and %TAG directive defining the handle for core/asdf tags - Fix cludgy tag formatting in libfyaml, and handle shortening tags that can use the handle. Newer versions of libfyaml can handle this better automatically IIRC, but older versions like the 0.8.1 on Ubuntu need extra intervention :(
Unfortunate workaround for a bug in libfyaml 0.8 which mishandled a root node event with a non-default tag immediately following the document start. This was fixed in libfyaml 0.9+, but as ubuntu-latest (24.04) has libfyaml 0.8 it's best to try to support it as a minimum version. Newer versions of Ubuntu have 0.9+.
Most of these are difficult to simulate in tests, especially the emitter errors. ReadYAMLSequence's parse error does happen to be tested in test_parser_error.
Moves the YAML backend detection into a dedicated macro in m4/ast_yaml.m4 `AST_CHECK_YAML` This handles things like setting precious variables (F)YAML_CFLAGS, (F)YAML_LIBS consistently between backends; keeping some of the messier logic directly out of configure.ac. Pulled this in from a branch that tested merging this with my autotools branch for PR Starlink#37. As a follow-up we might want to consider adding optional pkg-config support here, which would make our lives easier. The new cmake build system already uses pkg-config. I just resisted adding it for now to the autotools build since there was no prior precedent of its use, and I worry about unintended breakage of the building within Starlink.
In the case where the ref_map is assigned a Mapping for the first time the returned (borrowed) reference was not annulled.
These two event cases did not invoke astYamlEventDelete before breaking out of the loop.
libyaml's input is line-oriented, whereas with libfyaml we would like to use fy_parser_set_input_callback which byte-oriented, but much better than the previous astYamlParserSetInput which required buffering the entire YAML into memory to pass to fy_parser_set_input_string. Providing YamlChan with a line buffer for its reads gives the best of both worlds.
Suppresses leak reports from the one known upstream bug in older versions of libfyaml that leak into AST via testyamlchan. This is mainly for the CI which still runs older versions of Ubuntu that have this leak in its system libfyaml.
2 participants
Something I've been working on off-and-on for a few weeks and finally had a chance to finish.
Motivation
AST uses the older (but still actively maintained) lower-level
libyamlto parse and emit YAML. My project,libasdf, on the other hand uses the newerlibfyaml. And then, in particular, the libasdf-gwcs plugin forlibasdfwhich is using AST as an evaluation backend, in turn also requireslibfyamland via ASTlibyaml.In effect, any code that depends on
libasdf-gwcsmust also detect two separate YAML libraries, which is a bit onerous. It's not the end of the world--both libraries are widely available. It just feels a bit rough.On the other hand I'd also understand if AST doesn't want to maintain support for both. I just wanted to see if it could be done, since both libraries are implementing the same recommended YAML parser/emitter interfaces, just with slight API differences.
Implementation
This adds a
yaml_backend.hheader-only library providingastYamlfunctions that roughly map to the equivalentyaml_functions originally used byyamlchan.c. This is an almost direct drop-in replacement, so the changes toyamlchan.citself are minimal; the only major difference is in error handling which is improved over the original implementation anyways (better reporting of line number context for errors).Between the two backends the
libyamlone is still much simpler--it's almost a direct wrapper. Thelibfyamlbackend needs to do a little extra work to adhere to the originallibyamlinterface.The backend to use can be selected explicitly when configuring the build. If not specified the build system chooses, with priority given for
libyamlfor backwards-compatibility: The outputs of the two libraries are not bytewise-identical, though they are of course equivalent YAML.Shortcomings
Compile-time backend selection: The YAML backend (libyaml, libfyaml, or none) is selected at CMake/autoconf configure time and compiled into libast. Packagers who want to ship both variants must build separate conflicting packages (there is precedent for this, however.). A cleaner long-term approach might be a satellite shared library, analogous to the existing libast_grf_*.so graphics plugins. Both backends could co-exist with one selected and
dlopen'd at runtime, though this would be a more complex refactor.