Puppet

We have several options here:

1. skip binary plists
2. convert binary plists to xml1
3. switch to using Foundation calls with osx/cocoa

1) means we simply can’t cope with binary plists, so anytime anyone uses ‘defaults’ it will make the job such that puppet can’t cope with it 2) means we are changing the state of the system even when simply checking the state of a service resource 3) means we’re dropping support for 10.4

When I first wrote this, there were no binary launchd plists shipped by Apple. That seems to have changed.

I haven’t verified on a clean retail image, but I think the launchd plists as shipped are all text, but they may be converted to binary as they’re changed.

This would be a change in the provider, right? So if we skip binary plists, we’d no longer be able to control them with service resources in a puppet manifest?

I vote for option 3; changing things just to query them seems icky, and I’d rather not skip binary plists.

I can’t see what’s changing them though? It’s like something is using NSDefaults to modify them… which I’m not doing in the provider at all for precisely this reason.

Surely launchtl isn’t converting them to binary now ?

Diff below. Is there a better method to get this patched?

diff --git a/lib/puppet/provider/service/launchd.rb b/lib/puppet/provider/service/launchd.
index 9703595..716afa3 100644
--- a/lib/puppet/provider/service/launchd.rb
+++ b/lib/puppet/provider/service/launchd.rb
@@ -1,4 +1,4 @@
-require 'facter/util/plist'
+require 'osx/cocoa'
 
 Puppet::Type.type(:service).provide :launchd, :parent => :base do
   desc "launchd service management framework.
@@ -62,7 +62,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)
+          job = OSX::NSDictionary.dictionaryWithContentsOfFile(fullpath).to_ruby
           if job and job.has_key?("Label")
             if job["Label"] == label
               return { label => fullpath }
@@ -118,7 +118,7 @@ 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)
+    job_plist = OSX::NSDictionary.dictionaryWithContentsOfFile(job_path)
     raise Puppet::Error.new("Unable to parse launchd plist at path: #{job_path}") if not 
     [job_path, job_plist]
   end
@@ -200,7 +200,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)
+      overrides = OSX::NSDictionary.dictionaryWithContentsOfFile(Launchd_Overrides).to_ru
 
       unless overrides.nil?
         if overrides.has_key?(resource[:name])
@@ -227,14 +227,14 @@ 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 = OSX::NSMutableDictionary.dictionaryWithContentsOfFile(Launchd_Overrides
       overrides[resource[:name]] = { "Disabled" => false }
-      Plist::Emit.save_plist(overrides, Launchd_Overrides)
+      overrides.writeToFile_atomically_(Launchd_Overrides, true)
     else
       job_path, job_plist = plist_from_label(resource[:name])
       if self.enabled? == :false
         job_plist.delete("Disabled")
-        Plist::Emit.save_plist(job_plist, job_path)
+        job_plist.writeToFile_atomically_(job_path, true)
       end
     end
   end
@@ -242,13 +242,13 @@ 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 = OSX::NSMutableDictionary.dictionaryWithContentsOfFile(Launchd_Overrides
       overrides[resource[:name]] = { "Disabled" => true }
-      Plist::Emit.save_plist(overrides, Launchd_Overrides)
+      overrides.writeToFile_atomically_(Launchd_Overrides, true)
     else
       job_path, job_plist = plist_from_label(resourcecommit::name])
       job_plist["Disabled" = true
-      Plist::Emit.save_plist(job_plist, job_path)
+      job_plist.writeToFile_atomically_(job_path, true)
     end
   end
 

Clay, have you looked at this?

http://projects.puppetlabs.com/projects/puppet/wiki/Development_Development_Lifecycle

I’d really like to see you guys commit more patches, and this is an easier way, happy to sit down at work and run through this if you have any issues.

I’m pretty sure we’ll need to update some unit tests too.

Clay, how do you feel about this patch?

http://github.com/nigelkersten/puppet/commit/9b87bb8f25d896c41010f0469e75d1a875d016dd

I’ll mail it to the dev-list if it passes your test, but I did some quick on the fly conversions and it worked fine.

(This should also patch against 2.6.x cleanly)

That’s a nice solution. Looks good to me.

The situation isn’t that clear though Paul.

Apple weren’t shipping binary plists or converting xml plists to binary during the 0.25.x lifecycle, they only started doing this after 2.6.0 was released.

This is a bug that pretty much breaks the Service provider on OS X. We’re not going to fix this in 2.6.x at all?

What Nigel said. :)

My git-fu isn’t so amazing, but the patch should at least apply reasonably cleanly? I didn’t think we’d really made any changes to the provider in 2.6.x….

It was mostly failing because of whitespace changes we made between 0.25 and 2.6 (we switched from 4-space indentation to 2-space indentation), and that confused git.

I’ve now fixed all the whitespace problems (I think) and cherry-picked the patch to 2.6.x, and I’m getting spec test failures. I’m happy to track them down, Nigel, but you probably are more familiar with this corner of the code than I am. Do you want to have a look? I uploaded my cherry-pick to http://github.com/stereotype441/puppet/tree/wip/4025 if you’re interested.