+1. I reviewed the code in person with Paul. I don't see a problem saving binary plists back to xml when there's changes since they're both equally readable by the OS. I think Paul has addressed Nigel's concerns on this ticket. Correct me if I'm wrong.
On Fri, Sep 24, 2010 at 3:46 PM, Paul Berry <[email protected]> wrote: > Ah, ok. Thanks for the clarification, Nigel. I'd failed to understand the > distinction between plists and preference files. I've removed my > parenthetical comments about "default" format from the commit message. It > now says: > Modified the launchd provider to use OSX's "plutil" command to read > plists. This allows it to handle properly lists in both XML format > and binary format. > Launchd continues to write out propertly lists in XML format. This is > not a problem because the operating system is able to understand both > formats. > Paul > On Fri, Sep 24, 2010 at 3:25 PM, Nigel Kersten <[email protected]> wrote: >> >> On Fri, Sep 24, 2010 at 3:12 PM, Paul Berry <[email protected]> wrote: >> > My understanding was that Apple changed the default format for plists >> > from >> > XML to binary with release 10.4 >> > (http://en.wikipedia.org/wiki/Property_list#Mac_OS_X). I believe what >> > happens now is that many plist files are still shipped in XML format, >> > but >> > the moment a user tries to change one of them using the GUI (e.g. >> > "System >> > Preferences...") it gets rewritten in binary format. >> >> Ah, but what you've said is different to the Wikipedia article. It >> says that the default format changed for "preference files" which are >> a subset of plists. >> >> A "default format" for plists doesn't really make sense. >> >> Instead we have default formats that various frameworks use when >> emitting plists, and the major change was Apple moving NSDefaults and >> a few others to default to binary, thus the reference to "preference >> files" which should be referring to the NSDefault preference domains >> such as /Library/Preferences, ~/Library/Preferences, and >> ~/Library/Preferences/ByHost, not the plists launchd uses. >> >> Not all System Preference changes cause the plist to change format. >> Many of the plists we're seeing change formats don't have a GUI >> control at all. >> >> >> > Nigel, you make a good point that there is other code in Puppet that >> > deals >> > with plists, and we ought to refactor so that everything that handles >> > plists >> > goes through the same code path, and that code path can work with both >> > binary and XML formats. But that's a more major change, and I don't >> > think >> > we'll be able to slip it into the 2.6.2 release. I'll put a ticket in >> > the >> > system to remind us to revisit this issue when we've finished our >> > current >> > focus on 2.6.2. >> >> Awesome. >> >> > Paul >> > >> > On Fri, Sep 24, 2010 at 3:00 PM, Nigel Kersten <[email protected]> >> > wrote: >> >> >> >> We have a lot of plist parsing around the place with the Mac code. >> >> >> >> Is there any chance we can make a convenience method in Puppet::Util >> >> or something so that we can start using that everywhere, and deal with >> >> all theses issues in a single location? >> >> >> >> The description isn't quite right by the way, as it makes it sound >> >> like launchd switched to binary plists with 10.4. >> >> >> >> NSDefaults and thus the "defaults" binary changed to modify all xml1 >> >> plists to binary on write with 10.4, but Apple all throughout 10.5 >> >> were only shipping xml1 format plists with launchd. >> >> >> >> Even when 10.6.0 came out all launchd plists were xml1, but something >> >> has changed since then, and there are some conditions under which a >> >> plist changes from xml1 to binary behind our back. I've yet to work >> >> out what that is. >> >> >> >> It would be ideal if we could preserve the plist format when writing >> >> to it... but it would be annoying work. >> >> >> >> >> >> On Fri, Sep 24, 2010 at 2:52 PM, Paul Berry <[email protected]> >> >> wrote: >> >> > Modified the launchd provider to use OSX's "plutil" command to read >> >> > plists. This allows it to handle properly lists in both XML format >> >> > (which was the default before OSX 10.4) and binary format (which is >> >> > the default starting with OSX 10.4). >> >> > >> >> > Launchd continues to write out propertly lists in XML format. This >> >> > is >> >> > not a problem because the operating system is able to understand both >> >> > formats. >> >> > >> >> > Signed-off-by: Paul Berry <[email protected]> >> >> > --- >> >> > lib/puppet/provider/service/launchd.rb | 25 >> >> > ++++++++++++++++--------- >> >> > spec/unit/provider/service/launchd_spec.rb | 10 +++++----- >> >> > 2 files changed, 21 insertions(+), 14 deletions(-) >> >> > >> >> > diff --git a/lib/puppet/provider/service/launchd.rb >> >> > b/lib/puppet/provider/service/launchd.rb >> >> > index 9703595..b296e0a 100644 >> >> > --- a/lib/puppet/provider/service/launchd.rb >> >> > +++ b/lib/puppet/provider/service/launchd.rb >> >> > @@ -38,6 +38,7 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent >> >> > => :base do >> >> > >> >> > commands :launchctl => "/bin/launchctl" >> >> > commands :sw_vers => "/usr/bin/sw_vers" >> >> > + commands :plutil => "/usr/bin/plutil" >> >> > >> >> > defaultfor :operatingsystem => :darwin >> >> > confine :operatingsystem => :darwin >> >> > @@ -52,6 +53,12 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent >> >> > => :base do >> >> > Launchd_Overrides = >> >> > "/var/db/launchd.db/com.apple.launchd/overrides.plist" >> >> > >> >> > >> >> > + # Read a plist, whether its format is XML or in Apple's "binary1" >> >> > + # format. >> >> > + def self.read_plist(path) >> >> > + Plist::parse_xml(plutil('-convert', 'xml1', '-o', '-', path)) >> >> > + end >> >> > + >> >> > # returns a label => path map for either all jobs, or just a single >> >> > # job if the label is specified >> >> > def self.jobsearch(label=nil) >> >> > @@ -62,8 +69,7 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent >> >> > => :base do >> >> > next if f =~ /^\..*$/ >> >> > next if FileTest.directory?(f) >> >> > fullpath = File.join(path, f) >> >> > - job = Plist::parse_xml(fullpath) >> >> > - if job and job.has_key?("Label") >> >> > + if FileTest.file?(fullpath) and job = read_plist(fullpath) >> >> > and job.has_key?("Label") >> >> > if job["Label"] == label >> >> > return { label => fullpath } >> >> > else >> >> > @@ -118,8 +124,11 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent => :base do >> >> > def plist_from_label(label) >> >> > job = self.class.jobsearch(label) >> >> > job_path = job[label] >> >> > - job_plist = Plist::parse_xml(job_path) >> >> > - raise Puppet::Error.new("Unable to parse launchd plist at path: >> >> > #{job_path}") if not job_plist >> >> > + if FileTest.file?(job_path) >> >> > + job_plist = self.class.read_plist(job_path) >> >> > + else >> >> > + raise Puppet::Error.new("Unable to parse launchd plist at >> >> > path: >> >> > #{job_path}") >> >> > + end >> >> > [job_path, job_plist] >> >> > end >> >> > >> >> > @@ -200,9 +209,7 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent => :base do >> >> > job_plist_disabled = job_plist["Disabled"] if >> >> > job_plist.has_key?("Disabled") >> >> > >> >> > if self.class.get_macosx_version_major == "10.6": >> >> > - overrides = Plist::parse_xml(Launchd_Overrides) >> >> > - >> >> > - unless overrides.nil? >> >> > + if FileTest.file?(Launchd_Overrides) and overrides = >> >> > self.class.read_plist(Launchd_Overrides) >> >> > if overrides.has_key?(resource[:name]) >> >> > overrides_disabled = overrides[resource[:name]]["Disabled"] >> >> > if >> >> > overrides[resource[:name]].has_key?("Disabled") >> >> > end >> >> > @@ -227,7 +234,7 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent => :base do >> >> > # versions this is stored in the job plist itself. >> >> > def enable >> >> > if self.class.get_macosx_version_major == "10.6" >> >> > - overrides = Plist::parse_xml(Launchd_Overrides) >> >> > + overrides = self.class.read_plist(Launchd_Overrides) >> >> > overrides[resource[:name]] = { "Disabled" => false } >> >> > Plist::Emit.save_plist(overrides, Launchd_Overrides) >> >> > else >> >> > @@ -242,7 +249,7 @@ Puppet::Type.type(:service).provide :launchd, >> >> > :parent => :base do >> >> > >> >> > def disable >> >> > if self.class.get_macosx_version_major == "10.6" >> >> > - overrides = Plist::parse_xml(Launchd_Overrides) >> >> > + overrides = self.class.read_plist(Launchd_Overrides) >> >> > overrides[resource[:name]] = { "Disabled" => true } >> >> > Plist::Emit.save_plist(overrides, Launchd_Overrides) >> >> > else >> >> > diff --git a/spec/unit/provider/service/launchd_spec.rb >> >> > b/spec/unit/provider/service/launchd_spec.rb >> >> > index 320ee3a..116ba21 100755 >> >> > --- a/spec/unit/provider/service/launchd_spec.rb >> >> > +++ b/spec/unit/provider/service/launchd_spec.rb >> >> > @@ -98,19 +98,19 @@ describe provider_class do >> >> > it "should return true if the job plist says disabled is true and >> >> > the global overrides says disabled is false" do >> >> > provider_class.stubs(:get_macosx_version_major).returns("10.6") >> >> > @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" >> >> > => >> >> > true}]) >> >> > - Plist.stubs(:parse_xml).returns({...@resource[:name] => >> >> > {"Disabled" >> >> > => false}}) >> >> > + >> >> > �[email protected](:read_plist).returns({...@resource[:name] >> >> > => >> >> > {"Disabled" => false}}) >> >> > @provider.enabled?.should == :true >> >> > end >> >> > it "should return false if the job plist says disabled is false >> >> > and >> >> > the global overrides says disabled is true" do >> >> > provider_class.stubs(:get_macosx_version_major).returns("10.6") >> >> > @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" >> >> > => >> >> > false}]) >> >> > - Plist.stubs(:parse_xml).returns({...@resource[:name] => >> >> > {"Disabled" >> >> > => true}}) >> >> > + >> >> > �[email protected](:read_plist).returns({...@resource[:name] >> >> > => >> >> > {"Disabled" => true}}) >> >> > @provider.enabled?.should == :false >> >> > end >> >> > it "should return true if the job plist and the global overrides >> >> > have no disabled keys" do >> >> > provider_class.stubs(:get_macosx_version_major).returns("10.6") >> >> > @provider.stubs(:plist_from_label).returns(["foo", {}]) >> >> > - Plist.stubs(:parse_xml).returns({}) >> >> > + �[email protected](:read_plist).returns({}) >> >> > @provider.enabled?.should == :true >> >> > end >> >> > end >> >> > @@ -182,7 +182,7 @@ describe provider_class do >> >> > describe "when enabling the service on OS X 10.6" do >> >> > it "should write to the global launchd overrides file once" do >> >> > provider_class.stubs(:get_macosx_version_major).returns("10.6") >> >> > - Plist.stubs(:parse_xml).returns({}) >> >> > + �[email protected](:read_plist).returns({}) >> >> > Plist::Emit.expects(:save_plist).once >> >> > @provider.enable >> >> > end >> >> > @@ -191,7 +191,7 @@ describe provider_class do >> >> > describe "when disabling the service on OS X 10.6" do >> >> > it "should write to the global launchd overrides file once" do >> >> > provider_class.stubs(:get_macosx_version_major).returns("10.6") >> >> > - Plist.stubs(:parse_xml).returns({}) >> >> > + �[email protected](:read_plist).returns({}) >> >> > Plist::Emit.expects(:save_plist).once >> >> > @provider.enable >> >> > end >> >> > -- >> >> > 1.7.2 >> >> > >> >> > -- >> >> > You received this message because you are subscribed to the Google >> >> > Groups "Puppet Developers" group. >> >> > To post to this group, send email to [email protected]. >> >> > To unsubscribe from this group, send email to >> >> > [email protected]. >> >> > For more options, visit this group at >> >> > http://groups.google.com/group/puppet-dev?hl=en. >> >> > >> >> > >> >> >> >> >> >> >> >> -- >> >> nigel >> >> >> >> -- >> >> You received this message because you are subscribed to the Google >> >> Groups >> >> "Puppet Developers" group. >> >> To post to this group, send email to [email protected]. >> >> To unsubscribe from this group, send email to >> >> [email protected]. >> >> For more options, visit this group at >> >> http://groups.google.com/group/puppet-dev?hl=en. >> >> >> > >> > -- >> > You received this message because you are subscribed to the Google >> > Groups >> > "Puppet Developers" group. >> > To post to this group, send email to [email protected]. >> > To unsubscribe from this group, send email to >> > [email protected]. >> > For more options, visit this group at >> > http://groups.google.com/group/puppet-dev?hl=en. >> > >> >> >> >> -- >> nigel >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Puppet Developers" group. >> To post to this group, send email to [email protected]. >> To unsubscribe from this group, send email to >> [email protected]. >> For more options, visit this group at >> http://groups.google.com/group/puppet-dev?hl=en. >> > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
