It is only needed for OS X at this time, yes. I agree with Adrien in that if there are bugs found with CFPropertyList (i.e. CFP), they should be submitted upstream and remedied so we can take advantage of them. The original premise of vendoring CFP into Puppet was indeed to take advantage of its speed improvements and also to be able to natively read binary plists without having to use `plutil` on every call...which adds time to every run.
I don't consider CFP to be 'unstable' as suggested (for all the reasons that Brian outlined), rather I think it's a drastic change to the current method Puppet is using for plist parsing and is thus DIFFERENT than we've used before. I feel Adrien's pain in that every CFP release would need to be diffed and applied to our vendored library in Facter, and that's not necessarily easy to do. We also don't really want to have Facter rely on the gem because then that would be restricting Facter's entire operation on OS X based on a gem that a user has to install. Since we own the packaging, it's entirely conceivable that we could bundle the gem along with Puppet, but now we have a packaging issue. On Wed, Aug 28, 2013 at 11:06 AM, Adrien Thebo <[email protected]>wrote: > I'm almost certain that this is only needed on OSX, so if we're doing an > all in one package that could work. And I would much rather contribute > upstream and defer packaging to the upstream project rather than load that > on ourselves. > > > On Wed, Aug 28, 2013 at 11:04 AM, Michael Stanhke > <[email protected]>wrote: > >> >> >> 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<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<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<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/513<https://github.com/puppetlabs/facter/pull/513>introduced >>> 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. >> > > > > -- > 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. > -- Gary Larizza Professional Services Engineer 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.
