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}}) > >> > + @provider.class.stubs(: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}}) > >> > + @provider.class.stubs(: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({}) > >> > + @provider.class.stubs(: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({}) > >> > + @provider.class.stubs(: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({}) > >> > + @provider.class.stubs(: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]<puppet-dev%[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]<puppet-dev%[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]<puppet-dev%[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]<puppet-dev%[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.
