I agree that 'unstable' is a bit harsh; as soon as I sent that first email I realized that I worded things poorly. Bad developer! No coffee!
That being said, in order to justify vendoring something we need a guarantee that the vendored library is beyond 'stable' and verging into 'absurdly solid', because of the high cost of updating a vendored copy of the library. If we had a downstream version of CFPropertyList and packaged it separately, this would alleviate a lot of the concerns about upstream changes. Instead of having to diff and patch files, it would be a matter of 'git fetch upstream && git merge --no-ff upstream'. On Wed, Aug 28, 2013 at 12:03 PM, Gary Larizza <[email protected]> wrote: > 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. > -- 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.
