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

Unbundling of libyaml? #49

Open
pghmcfc opened this issue Jul 20, 2016 · 12 comments
Open

Unbundling of libyaml? #49

pghmcfc opened this issue Jul 20, 2016 · 12 comments

Comments

@pghmcfc
Copy link

pghmcfc commented Jul 20, 2016

Hi,

is there any prospect of being able to unbundle libyaml and build against a system-wide version at some point? It would certainly be appreciated downstream:

Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=664224
Fedora: https://bugzilla.redhat.com/show_bug.cgi?id=1081559
Gentoo: https://bugs.gentoo.org/show_bug.cgi?id=410653

@Leont
Copy link

Leont commented Jul 20, 2016

I agree this would make sense

@ingydotnet
Copy link
Owner

ingydotnet commented Jul 23, 2016

I'm not sure. The bundled version has a few small fixes, and I think the process of getting them merged and distributed is a rathole that I'd rather not go down.

Debian seems to think it's a doc and license file fix.

Suggestions?

@Leont
Copy link

Leont commented Jul 25, 2016

I'm not sure. The bundled version has a few small fixes, and I think the process of getting them merged and distributed is a rathole that I'd rather not go down.

Why would it be a rathole?

Debian seems to think it's a doc and license file fix.

I think that was an additional issue that is now resolved.

@gregoa
Copy link

gregoa commented Aug 26, 2016

Correct, the license documentation issue was https://bugs.debian.org/664196 which is fixed. https://bugs.debian.org/664224 was cloned from that one to track the embedded library issue itself. And yes, we still would like to be able to build against the system libyaml. Currently we are violating a "should" in Debian Policy 4.13: https://www.debian.org/doc/debian-policy/ch-source.html#s-embeddedfiles

@pghmcfc
Copy link
Author

pghmcfc commented Sep 12, 2016

Incidentally, did anyone notice the (unauthorized) release of YAML-LibYAML 0.71 by RURBAN?

https://metacpan.org/release/RURBAN/YAML-LibYAML-0.71
https://github.com/rurban/yaml-libyaml-pm

What's happening there?

@karenetheridge
Copy link

@pghmcfc why not ask him? Did he ever submit a PR with requested fixes?

@ingydotnet
Copy link
Owner

fwiw, I've been given maintenance bits for libyaml from the author. ie A python guy and I are the new maintainers. So now we can revisit this with some sanity. @pghmcfc @Leont @gregoa @karenetheridge advice welcome.

@pghmcfc I had not seen @rurban's release. @rurban what are you doing there?

@rurban
Copy link

rurban commented Sep 13, 2016

On Sep 13, 2016, at 1:19 AM, Ingy döt Net notifications@github.com wrote:

fwiw, I've been given maintenance bits for libyaml from the author. ie A python guy and I are the new maintainers. So now we can revisit this with some sanity. @pghmcfc @Leont @gregoa @karenetheridge advice welcome.

@pghmcfc I had not seen @rurban's release. @rurban what are you doing there?

I published the state I used for cperl, even without perms and even if I forgot to include
the Test::Base dependency. Deal with it.

Waiting a year is too long, i have release deadlines.

@pghmcfc
Copy link
Author

pghmcfc commented Oct 13, 2016

fwiw, I've been given maintenance bits for libyaml from the author. ie A python guy and I are the new maintainers. So now we can revisit this with some sanity. @pghmcfc @Leont @gregoa @karenetheridge advice welcome.

This is good news. Things I'd look at would include:

  • Which modifications made here in yaml-libyaml-pm would be suitable for the general population of libyaml users, and look to get those merged into libyaml; if that's all of them, then that's a big step towards being able to unbundle it
  • Take a look at @rurban's fork of this repo, and also the issues he previously raised here, and see if you agree with those changes; if so, bring them in
  • Consider making a new release with version number higher than 0.71 to avoid any possible confusion over which is the latest official release
  • If unbundling of libyaml is feasible because the changes here can be merged upstream, consider adding a hook to make it possible to unbundle. This might be disabled by default, at least initially whilst more extensive testing happens. See for example the "Note to Downstream Packagers" section in the Makefile.PL for DBD-SQLite (http://cpansearch.perl.org/src/ISHIGAKI/DBD-SQLite-1.50/Makefile.PL).
  • Consider adding tests that would fail with an unbundled libyaml that didn't contain the necessary fixes from here.

@ingydotnet
Copy link
Owner

@pghmcfc Thanks for the thoughtful comments. I've already made PRs for all the diffs between YAML::XS's master and libyaml, and some of them are even merged now. I'm not 100% sure if I got everything. Could you maybe take a look and see if the PRs are representative (ir I can look later this weekend).

@rurban
Copy link

rurban commented Oct 15, 2016

I fixed yesterday the intermediate YAML::XS/libyaml work for utf8 and option problems. I had several problems.
It's on my github now as 0.74.

What's still todo is safety. Either as in python with SafeLoad/SafeLoadFile, or as in YAML::Syck with global variables: local ($YAML::Syck::LoadCode, $YAML::Syck::UseCode, $YAML::Syck::LoadBlessed);.
cperl's Parse::CPAN::Meta can now use YAML::XS, YAML::Syck and it's own CPAN::Meta::YAML to parse META.yml, while ignoring the YAML::XS problem of embedded code and classes.
cpan ditto.

https://github.com/rurban/libyaml/tree/perl-yaml-xs is not mergable as of now, I'll see what happened.
https://github.com/rurban/yaml-libyaml-pm needs a tiny rebase for Tina's uploads.

@perlpunk
Copy link
Collaborator

@rurban yeah, I'd also like to see SafeLoad

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

7 participants