On Wednesday, August 28, 2013 10:53:42 AM UTC-7, Adrien Thebo wrote: > > Hi folks, > > I've been working on some issues on how Facter handles plist libraries, > and right now we're in a bit of a bind. The short story is that in Facter > 1.7 we vendored a new plist library that breaks backwards compatibility, > but there's a lot of pressure to keep the plist library in and make it > compatible. The full discussion can be found at > https://github.com/puppetlabs/facter/pull/499 . > > > Prior to Facter 1.7, Facter had a basic plist library that could handle > plain text plist files, but had to offload binary plist files to Apple's > CoreFoundation libraries. My understanding is that this caused a severe > performance hit on OSX systems. > > As part of #11299 (https://projects.puppetlabs.com/issues/11299) we > vendored the CFPropertyList library to replace the old plist > implementation. The implementation can opportunistically use libxml2, > nokogiri, and REXML, and is significantly faster than the old plist > library. This change was released in Facter 1.7. > > However, the old plist implementation monkey patched Symbol, Array, and > Hash, and CFPropertyList monkey patched those classes as well - but with a > different method signature. The old plist implementation defines `to_plist` > to take an optional *boolean* value, while CFPropertyList defines > `to_plist` to take an optional *hash*. > > Moreover, by our current standards CFPropertyList is not a candidate for > vendoring. The general guideline for vendoring is that candidates must be > stable libraries, and should only be one or two libraries. In addition the > CFPropertyList doesn't seem particularly stable, as demonstrated by the > comment > https://github.com/puppetlabs/facter/pull/499#issuecomment-21770546 which > indicates that CFPropertyList may not be very stable. Having to continually > re-vendor that library inside of Facter is a special level of torment > that's normally reserved for people that take up two parking spaces with > one car. > > But wait, there's more! > https://github.com/puppetlabs/facter/pull/513introduced a patch to > CFPropertyList to force the string encoding type for > Ruby compatibility. So on top of having to re-vendor the CFPropertyList to > handle updates, we would have to carry our own patches on top of everything > else. This means that the platform team will be forced to more or less > maintain this library. > > With all of these problems in mind it seems reasonable to remove the > CFPropertyList, but there have been some very strong arguments that > CFPropertyList is essential, removing it would cripple the performance of > Facter on OSX, and it's unacceptable to make CFPropertyList an external > dependency. > > At this point I would like additional feedback from the community - what > would be best. Keep CFPropertyList vendored inside of Facter and carry > patches to ensure compatibility? Remove the library and try to load it > opportunistically? Remove it entirely? > > My vote would be to unvendor it, make it a dependency, and submit fixes upstream to the authors. Having vendored libs is generally painful for exactly the reasons you outline above. Is this lib needed on platforms that are not Mac? If it's only on mac there are a few other options available since the way we package mac is kind of an all-in-one, unless you're using gems.
> > > -- > Adrien Thebo | Puppet Labs > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/puppet-dev. For more options, visit https://groups.google.com/groups/opt_out.
