-
Notifications
You must be signed in to change notification settings - Fork 327
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
testsuite failure on i386 after applying test fix from master #60
Comments
So the problem here is when emitting two documents back to back with the plain style, where one document is %YAML 1.1
--- !!str "foo"
%YAML 1.1
--- !!str "bar" However, in the plain style, this is emitted as: %YAML 1.1
--- !!str foo
%YAML 1.1
--- !!str bar This is now incorrect, as the plain style allows continuation lines, so the second %YAML 1.1
--- !!str foo
...
%YAML 1.1
--- !!str bar
... Therefore, I believe that the commit should be reverted, as it was serving a purpose. Technically the final terminator in the stream is not needed, but it allows streams to be concatenated without having to worry, so I guess it's fine (though if you dropped it, I imagine that would also fix the tests in consumers of the library, as they probably aren't expecting the final one; this is certainly the case with |
Indeed, with the following patches, pyyaml's test suite passes again, but the final unnecessary trailing terminator is not emitted.
|
ping |
@ingydotnet do you have any advice for this patch? |
Ping |
I will add this patch as a candidate to the upcoming 0.1.8 release. https://github.com/yaml/libyaml/projects/1 |
@ingydotnet I think there has been a mistake in this patch apply... I see the latest libyaml included the first version of my patch, but the one from @jrtc27 is a better version... can you please revert and apply this one? #60 (comment) |
or maybe you have your reasons for this? 56400d9#diff-55aa019b48be9270c41a41aad1bd405c |
I’m not sure this should be closed. PyYAML 4.1 still fails its test suite with LibYAML 0.2.1 as described in the original report. |
reopened! |
This has come up as an issue with the libyaml-based YAML builder for Crystal in crystal-lang/crystal#6283 |
I created #122 to revert the commit |
I explained in my PR why this needs to be reverted. libyaml is creating invalid YAML in some cases. |
@jrtc27 I'll have a look at your patches, thanks |
In YAML 1.1, the document end marker `...` is optional even if the next document starts with a directive: https://github.com/yaml/pyyaml/blob/master/tests/data/spec-07-09.canonical ``` %YAML 1.1 --- !!str "foo" %YAML 1.1 --- !!str "bar" %YAML 1.1 --- !!str "baz" ``` It is only required if the scalar is "open ended", for example for plain scalars. In YAML 1.2 the `...` marker is always required before a directive. My suggestion would be to make the output 1.2 compatible. It will still be 1.1 compatible, so that shouldn't be a problem. I believe this will also make it easier to fix #123 which was introduced with the last fixes regarding `open_ended`. I think I can make a fix for this soon after this issue is fixed. Fixing #123 without this would be a bit more complicated. If we do this, we also need to adjust PyYAML to behave the same. Related issues/commits: - #60 - #122 - 56400d9, 8ee83c0, 56f4b17
In YAML 1.1, the document end marker `...` is optional even if the next document starts with a directive: https://github.com/yaml/pyyaml/blob/master/tests/data/spec-07-09.canonical ``` %YAML 1.1 --- !!str "foo" %YAML 1.1 --- !!str "bar" %YAML 1.1 --- !!str "baz" ``` It is only required if the scalar is "open ended", for example for plain scalars. In YAML 1.2 the `...` marker is always required before a directive. My suggestion would be to make the output 1.2 compatible. It will still be 1.1 compatible, so that shouldn't be a problem. I believe this will also make it easier to fix #123 which was introduced with the last fixes regarding `open_ended`. I think I can make a fix for this soon after this issue is fixed. Fixing #123 without this would be a bit more complicated. If we do this, we also need to adjust PyYAML to behave the same. Related issues/commits: - #60 - #122 - 56400d9, 8ee83c0, 56f4b17
In YAML 1.1, the document end marker `...` is optional even if the next document starts with a directive: https://github.com/yaml/pyyaml/blob/master/tests/data/spec-07-09.canonical ``` %YAML 1.1 --- !!str "foo" %YAML 1.1 --- !!str "bar" %YAML 1.1 --- !!str "baz" ``` It is only required if the scalar is "open ended", for example for plain scalars. In YAML 1.2 the `...` marker is always required before a directive. My suggestion would be to make the output 1.2 compatible. It will still be 1.1 compatible, so that shouldn't be a problem. I believe this will also make it easier to fix #123 which was introduced with the last fixes regarding `open_ended`. I think I can make a fix for this soon after this issue is fixed. Fixing #123 without this would be a bit more complicated. If we do this, we also need to adjust PyYAML to behave the same. Related issues/commits: - #60 - #122 - 56400d9, 8ee83c0, 56f4b17
Hello, as said, I updated libyaml with the fix for the testsuite failures (e.g. haskell-yaml), in particular this single commit
56400d9
the problem is that now pyyaml has some test failing on i386
original yaml (latest version)
https://launchpadlibrarian.net/325264273/buildlog_ubuntu-artful-i386.pyyaml_3.12-1build2_BUILDING.txt.gz
patched yaml (commit 56400d9)
https://launchpadlibrarian.net/325269141/buildlog_ubuntu-artful-i386.pyyaml_3.12-1build2_BUILDING.txt.gz
can you please help?
thanks a lot
(the same failure happens with the latest git master FWIW)
The text was updated successfully, but these errors were encountered: