[CDRIVER-597] Support building without requiring vendored YAJL Created: 29/Mar/15 Updated: 03/May/17 Resolved: 01/Aug/16 |
|
| Status: | Closed |
| Project: | C Driver |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | TBD |
| Type: | Task | Priority: | Critical - P2 |
| Reporter: | Andrew Morrow (Inactive) | Assignee: | Hannes Magnusson |
| Resolution: | Duplicate | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||
| Backwards Compatibility: | Minor Change | ||||||||||||||||||||
| Description |
|
For packaging purposes, we may want to be able to build the C driver without having any vendored third party code like the yajl library, as some downstream packagers may prefer to use the system version of the library. Currently, we actually vendor a modified version of yajl, and we should figure out how to either make that dependency optional or to remove the need for our customizations. |
| Comments |
| Comment by Hannes Magnusson [ 01/Aug/16 ] |
|
Closing as "Duplicate" (for lack of better options). This will be fixed by |
| Comment by A. Jesse Jiryu Davis [ 07/Jan/16 ] |
|
Using another library for JSON parsing, or implementing it ourselves, is an option. But it hasn't reached the highest priority. |
| Comment by Thomas Champagne [ 07/Jan/16 ] |
|
Thanks for you two answers. |
| Comment by A. Jesse Jiryu Davis [ 06/Jan/16 ] |
|
Thanks for answering, Petr. Right, I hope YAJL will merge and release the fix someday. Meanwhile, we can vendor the updated YAJL on Fedora, and on Debian we're going a different direction and working around the missing YAJL feature, at the cost of a performance hit when reading many small JSON documents. |
| Comment by Petr Pisar [ 06/Jan/16 ] |
|
For yajl release, you have to ask yajl upstream. The yajl feature is in yajl_reset branch that has not yet been merged into master branch, also no new yajl release was made. Fedora will probably package the libbson with modifed bundled yajl as the Fedora's bundling policy changed recently. But I'm not happy about at all. |
| Comment by Thomas Champagne [ 06/Jan/16 ] |
|
Do you have any news on this packaging problem ? I see the patch for yajl. Is it work ? Do you know when it will be able to be merged in the master branch ? And last question, is there a new release planned for yajl (2.1.1 for example) with this patch ? |
| Comment by A. Jesse Jiryu Davis [ 24/Sep/15 ] |
|
The problem is in our implementation of bson_json_reader_read; when it comes to the end of a JSON object it resets the YAJL parser state and reuse it, rather than mallocing a new parser. For many small JSON objects this could be a substantial performance improvement, but unfortunately, libbson includes a private YAJL header in order to access private parser members and reset them. Lloyd Hilaiel, YAJL's maintainer, is working on a patch to provide us a yajl_reset_stack public function that does what we need: https://github.com/lloyd/yajl/issues/161 Once that's released, libbson will stop accessing private YAJL parser data. We'll continue to vendor YAJL into libbson for convenience, and update the vendored code to the latest upstream. We'll add a configure check for yajl_reset_stack (in configure.ac and CMakeLists.txt); if yajl_reset_stack is available we'll use it, otherwise we'll fall back to the slower yajl_alloc. The libbson packages for Debian, Fedora, etc. will comply with packaging policies by omitting the vendored copy of YAJL, and depend on the distro's YAJL package instead. When the distro's YAJL package includes the new yajl_reset_stack, then libbson can take advantage of it. |
| Comment by A. Jesse Jiryu Davis [ 24/Apr/15 ] |
|
Thanks Petr! I'll keep an eye on your pull request to the YAJL project. |
| Comment by Petr Pisar [ 24/Apr/15 ] |
|
In the previous patch. I forgot to replace inclusion of private <yajl/yajl_parser.h> with public <yajl/yajl_parse.h>. Use this patch instead. |
| Comment by Petr Pisar [ 24/Apr/15 ] |
|
This patch allows yajl to reset byte stack and libbson to use the feature instead of using private yajl headers. The yajl change has been proposed to yajl upstream as <https://github.com/lloyd/yajl/issues/161>. |
| Comment by A. Jesse Jiryu Davis [ 01/Apr/15 ] |
|
YAJL needs to be more consistent about letting us configure its allocator ( |