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

system libyaml #102

Closed
Alessandro-Barbieri opened this issue May 31, 2021 · 9 comments
Closed

system libyaml #102

Alessandro-Barbieri opened this issue May 31, 2021 · 9 comments

Comments

@Alessandro-Barbieri
Copy link

Can you provide a method to link to systemwide libyaml instead of the bundled one?

@spgarbet
Copy link
Member

Why would such a behavior be desirable? Is this a way to get 1.2 support?

@Alessandro-Barbieri
Copy link
Author

As now 1.2 isn't ready in libyaml yaml/libyaml#20

You can get an idea of why bundling is bad here https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries

@spgarbet
Copy link
Member

The current issue with this is that the current version of libyaml has a patch for indentation that doesn't not exist upstream. Indentation will break unless libyaml accepts that patch upstream if this is implemented.

@Alessandro-Barbieri
Copy link
Author

Have you proposed that patch upstream?

@spgarbet
Copy link
Member

spgarbet commented Feb 9, 2022

I inherited this project. The patch was not proposed upstream. I'm doing diffs now to determine the scope of the patch. The secondary investigation is what compile flags exist and do for the libyaml library.

@spgarbet
Copy link
Member

spgarbet commented Feb 9, 2022

I think this is difference.


--- libyaml/src/api.c	2022-02-09 13:06:31.373103740 -0600
+++ yaml/src/api.c	2022-02-09 13:28:42.162132786 -0600
@@ -541,6 +541,18 @@
 }
 
 /*
+ * Set whether or not to indent block sequences in mapping context.
+ */
+
+YAML_DECLARE(void)
+yaml_emitter_set_indent_mapping_sequence(yaml_emitter_t *emitter, int indent_mapping_sequence)
+{
+    assert(emitter);    /* Non-NULL emitter object expected. */
+
+    emitter->indent_mapping_sequence = indent_mapping_sequence;
+}
+
+/*
  * Set the preferred line width.
  */
 
--- libyaml/src/emitter.c	2022-02-09 13:06:31.374103725 -0600
+++ yaml/src/emitter.c	2022-02-09 13:28:45.995087663 -0600
@@ -886,7 +886,9 @@
     if (first)
     {
         if (!yaml_emitter_increase_indent(emitter, 0,
-                    (emitter->mapping_context && !emitter->indention)))
+                    (emitter->mapping_context
+                             && !emitter->indent_mapping_sequence
+                             && !emitter->indention)))
             return 0;
     }
 
@@ -1825,6 +1827,7 @@
 
     emitter->whitespace = is_whitespace;
     emitter->indention = (emitter->indention && is_indention);
+    emitter->open_ended = 0;
 
     return 1;
 }
@@ -1977,6 +1980,10 @@
 
     emitter->whitespace = 0;
     emitter->indention = 0;
+    if (emitter->root_context)
+    {
+        emitter->open_ended = 1;
+    }
 
     return 1;
 }

indent_mapping_sequence.txt

@spgarbet
Copy link
Member

I submitted the modifications as a pull request to the libyaml project. They had some outstanding tickets the pull request would solve. There's hope these two can be brought to the same version.

@spgarbet
Copy link
Member

One divergence in behavior between a system and the modified one in this package, now has a test case and an isolated patch with Issue #114

@spgarbet
Copy link
Member

Pull requests were rejected. I cannot use the system library and get the expected behavior. I tried changed these things to the default of the library and the data.table project immediately complained. There's no way to fix this unfortunately without the upstream library maintainers cooperation, short of changing libraries.

I started this project to change over to a yaml12 supported library: https://github.com/vubiostat/yaml12

I'll make it so it uses the system library.

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

No branches or pull requests

2 participants