(#13948) $libdir not in $LOAD_PATH

Change :call_on_define to :call_hook with values of
:on_define_and_write, :on_initialize_and_write, and :on_write_only.
Make hooks for $libdir, $factpath, and $storeconfig new value of
 :on_initialize_and_write to take advantage of bootstrapping sequence.
Change Puppet::Util::Settings to call hooks on settings designated as
:on_initialize_and_write immediately after initialing default
application values.
Deprecate Puppet::Util::Settings::StringSetting#call_on_define.

This change is backward compatible, although :call_on_define is
deprecated and should be changed to :call_hook => :on_define_and_write.
This change should also be forward compatible if we decide to call hooks
at other portions of the application bootstrapping lifecycle.

This commit fixes the problem that the default libdir was not being
added to Ruby's $LOAD_PATH properly.

Prior to this commit, the hooks for $libdir and $factpath were not
getting called unless explicitly overridden in the code. This meant that
the default $libdir was not part of $LOAD_PATH and the default $factpath
was not searched for facts.  This commit fixes those problems by
enabling settings to declare when the hook should be called.  A hook can
be only only when a value is overwritten (:on_write_only), which is the
default. A hook can be called with the default value when when the
setting is defined (as soon as Puppet::Util::Settings.define_settings is
called) (:on_define_and_write), in addition to any time the value is
overwritten. A hook can be called with the default value during
application initialization (:on_initialize_and_write), in addition to
any time the value is overwritten.

We've added the call to the initialize hooks in two places because we
have to deal with the bootstrapping of applications. We need to attempt
to parse command line options and the configuration file and call the
hooks. Then we need to do it again after the application is loaded
because it could very well have changed important things like $vardir,
which generally affects $libdir.

acceptance/tests/ticket_13948_lib_dir_hook_should_be_called_on_initialization.rb

@@ -0,0 +1,259 @@
+test_name "the $libdir setting hook is called on startup"
+
+#
+# This test is intended to ensure that pluginsync syncs face definitions to the agents.
+# Further, the face should be runnable on the agent after the sync has occurred.
+#
+# (NOTE: When this test is passing, it should resolve both #7316 re: verifying that apps/faces can
+#  be run on the agent node after a plugin sync, and #6753 re: being able to run a face without
+#  having a placeholder stub file in the "applications" directory.)
+#
+
+###############################################################################
+# BEGIN UTILITY METHODS - ideally this stuff would live somewhere besides in
+#  the actual test.
+###############################################################################
+
+# Create a file on the host.
+# Parameters:
+# [host] the host to create the file on
+# [file_path] the path to the file to be created
+# [file_content] a string containing the contents to be written to the file
+# [options] a hash containing additional behavior options.  Currently supported:
+# * :mkdirs (default false) if true, attempt to create the parent directories on the remote host before writing
+#       the file
+# * :owner (default 'root') the username of the user that the file should be owned by
+# * :group (default 'puppet') the name of the group that the file should be owned by
+# * :mode (default '644') the mode (file permissions) that the file should be created with
+def create_test_file(host, file_rel_path, file_content, options)
+
+  # set default options
+  options[:mkdirs] ||= false
+  options[:owner] ||= "root"
+  options[:group] ||= "puppet"
+  options[:mode] ||= "755"
+
+  file_path = get_test_file_path(host, file_rel_path)
+
+  mkdirs(host, File.dirname(file_path)) if (options[:mkdirs] == true)
+  create_remote_file(host, file_path, file_content)
+
+#
+# NOTE: we need these chown/chmod calls because the acceptance framework connects to the nodes as "root", but
+#  puppet 'master' runs as user 'puppet'.  Therefore, in order for puppet master to be able to read any files
+#  that we've created, we have to carefully set their permissions
+#
+
+  chown(host, options[:owner], options[:group], file_path)
+  chmod(host, options[:mode], file_path)
+
+end
+
+
+# Given a relative path, returns an absolute path for a test file.  Basically, this just prepends the
+# a unique temp dir path (specific to the current test execution) to your relative path.
+def get_test_file_path(host, file_rel_path)
+  File.join(@host_test_tmp_dirs[host.name], file_rel_path)
+end
+
+
+# Check for the existence of a temp file for the current test; basically, this just calls file_exists?(),
+# but prepends the path to the current test's temp dir onto the file_rel_path parameter.  This allows
+# tests to be written using only a relative path to specify file locations, while still taking advantage
+# of automatic temp file cleanup at test completion.
+def test_file_exists?(host, file_rel_path)
+  host.execute("test -f \"#{get_test_file_path(host, file_rel_path)}\"",
+               :acceptable_exit_codes => [0, 1])  do |result|
+    return result.exit_code == 0
+  end
+end
+
+def tmpdir(host, basename)
+  host_tmpdir = host.tmpdir(basename)
+  # we need to make sure that the puppet user can traverse this directory...
+  chmod(host, "755", host_tmpdir)
+  host_tmpdir
+end
+
+def mkdirs(host, dir_path)
+  on(host, "mkdir -p #{dir_path}")
+end
+
+def chown(host, owner, group, path)
+  on(host, "chown #{owner}:#{group} #{path}")
+end
+
+def chmod(host, mode, path)
+  on(host, "chmod #{mode} #{path}")
+end
+
+
+
+
+# pluck this out of the test case environment; not sure if there is a better way
+cur_test_file = @path
+cur_test_file_shortname = File.basename(cur_test_file, File.extname(cur_test_file))
+
+# we need one list of all of the hosts, to assist in managing temp dirs.  It's possible
+# that the master is also an agent, so this will consolidate them into a unique set
+all_hosts = Set[master, *agents]
+
+# now we can create a hash of temp dirs--one per host, and unique to this test--without worrying about
+# doing it twice on any individual host
+@host_test_tmp_dirs = Hash[all_hosts.map do |host| [host.name, tmpdir(host, cur_test_file_shortname)] end ]
+
+# a silly variable for keeping track of whether or not all of the tests passed...
+all_tests_passed = false
+
+###############################################################################
+# END UTILITY METHODS
+###############################################################################
+
+
+
+###############################################################################
+# BEGIN TEST LOGIC
+###############################################################################
+
+# create some vars to point to the directories that we're going to point the master/agents at
+master_module_dir = "master_modules"
+agent_var_dir = "agent_var"
+agent_lib_dir = "#{agent_var_dir}/lib"
+
+app_name = "superbogus"
+app_desc = "a simple %1$s for testing %1$s delivery via plugin sync"
+app_output = "Hello from the #{app_name} %s"
+
+master_module_file_content = {}
+
+
+master_module_face_content = <<-HERE
+Puppet::Face.define(:#{app_name}, '0.0.1') do
+  copyright "Puppet Labs", 2011
+  license   "Apache 2 license; see COPYING"
+
+  summary "#{app_desc % "face"}"
+
+  action(:foo) do
+    summary "a test action defined in the test face in the main puppet lib dir"
+
+    default
+    when_invoked do |*args|
+      puts "#{app_output % "face"}"
+    end
+  end
+
+end
+HERE
+
+master_module_app_content = <<-HERE
+require 'puppet/application/face_base'
+
+class Puppet::Application::#{app_name.capitalize} < Puppet::Application::FaceBase
+end
+
+HERE
+
+
+
+# this begin block is here for handling temp file cleanup via an "ensure" block at the very end of the
+# test.
+begin
+
+  # here we create a custom app, which basically doesn't do anything except for print a hello-world message
+  agent_module_face_file = "#{agent_lib_dir}/puppet/face/#{app_name}.rb"
+  master_module_face_file = "#{master_module_dir}/#{app_name}/lib/puppet/face/#{app_name}.rb"
+
+  agent_module_app_file = "#{agent_lib_dir}/puppet/application/#{app_name}.rb"
+  master_module_app_file = "#{master_module_dir}/#{app_name}/lib/puppet/application/#{app_name}.rb"
+
+
+  # copy all the files to the master
+  step "write our simple module out to the master" do
+    create_test_file(master, master_module_app_file, master_module_app_content, :mkdirs => true)
+    create_test_file(master, master_module_face_file, master_module_face_content, :mkdirs => true)
+  end
+
+  step "verify that the app file exists on the master" do
+    unless test_file_exists?(master, master_module_app_file) then
+      fail_test("Failed to create app file '#{get_test_file_path(master, master_module_app_file)}' on master")
+    end
+    unless test_file_exists?(master, master_module_face_file) then
+      fail_test("Failed to create face file '#{get_test_file_path(master, master_module_face_file)}' on master")
+    end
+  end
+
+  step "start the master" do
+    with_master_running_on(master,
+           "--modulepath=\"#{get_test_file_path(master, master_module_dir)}\" " +
+           "--autosign true") do
+
+      # the module files shouldn't exist on the agent yet because they haven't been synced
+      step "verify that the module files don't exist on the agent path" do
+        agents.each do |agent|
+            if test_file_exists?(agent, agent_module_app_file) then
+              fail_test("app file already exists on agent: '#{get_test_file_path(agent, agent_module_app_file)}'")
+            end
+            if test_file_exists?(agent, agent_module_app_file) then
+              fail_test("face file already exists on agent: '#{get_test_file_path(agent, agent_module_face_file)}'")
+            end
+        end
+      end
+
+      step "run the agent" do
+        agents.each do |agent|
+          run_agent_on(agent, "--trace --vardir=\"#{get_test_file_path(agent, agent_var_dir)}\" " +
+                              "--no-daemonize --verbose --onetime --test --server #{master}")
+        end
+      end
+
+    end
+  end
+
+  step "verify that the module files were synced down to the agent" do
+    agents.each do |agent|
+      unless test_file_exists?(agent, agent_module_app_file) then
+        fail_test("Expected app file not synced to agent: '#{get_test_file_path(agent, agent_module_app_file)}'")
+      end
+      unless test_file_exists?(agent, agent_module_face_file) then
+        fail_test("Expected face file not synced to agent: '#{get_test_file_path(agent, agent_module_face_file)}'")
+      end
+    end
+  end
+
+  step "verify that the application shows up in help" do
+    agents.each do |agent|
+      on(agent, PuppetCommand.new(:help, "--vardir=\"#{get_test_file_path(agent, agent_var_dir)}\"")) do
+        assert_match(/^\s+#{app_name}\s+#{app_desc % "face"}/, result.stdout)
+      end
+    end
+  end
+
+  step "verify that we can run the application" do
+    agents.each do |agent|
+      on(agent, PuppetCommand.new(:"#{app_name}", "--vardir=\"#{get_test_file_path(agent, agent_var_dir)}\"")) do
+        assert_match(/^#{app_output % "face"}/, result.stdout)
+      end
+    end
+  end
+
+  step "clear out the libdir on the agents in preparation for the next test" do
+    agents.each do |agent|
+      on(agent, "rm -rf #{get_test_file_path(agent, agent_lib_dir)}/*")
+    end
+  end
+
+  all_tests_passed = true
+
+ensure
+  ##########################################################################################
+  # Clean up all of the temp files created by this test.  It would be nice if this logic
+  # could be handled outside of the test itself; I envision a stanza like this one appearing
+  # in a very large number of the tests going forward unless it is handled by the framework.
+  ##########################################################################################
+  if all_tests_passed then
+    all_hosts.each do |host|
+      on(host, "rm -rf #{@host_test_tmp_dirs[host.name]}")
+    end
+  end
+end

lib/puppet/defaults.rb

@@ -136,7 +136,7 @@ module Puppet
         :default          => "none",
         :desc             => "The shell search path.  Defaults to whatever is inherited\n" +
             "from the parent process.",
-        :call_on_define   => true, # Call our hook with the default value, so we always get the libdir set.
+        :call_hook => :on_define_and_write,
         :hook             => proc do |value|
           ENV["PATH"] = "" if ENV["PATH"].nil?
           ENV["PATH"] = value unless value == "none"
@@ -155,7 +155,7 @@ module Puppet
             "guaranteed to work for those cases.  In fact, the autoload\n" +
             "mechanism is responsible for making sure this directory\n" +
             "is in Ruby's search path\n",
-      #:call_on_define   => true, # Call our hook with the default value, so we always get the libdir set.
+      :call_hook => :on_initialize_and_write,
       :hook             => proc do |value|
         $LOAD_PATH.delete(@oldlibdir) if defined?(@oldlibdir) and $LOAD_PATH.include?(@oldlibdir)
         @oldlibdir = value
@@ -370,7 +370,7 @@ module Puppet
     :certname => {
       :default => fqdn.downcase, :desc => "The name to use when handling certificates.  Defaults
       to the fully qualified domain name.",
-      :call_on_define => true, # Call our hook with the default value, so we're always downcased
+      :call_hook => :on_define_and_write, # Call our hook with the default value, so we're always downcased
       :hook => proc { |value| raise(ArgumentError, "Certificate names must be lower case; see #1168") unless value == value.downcase }},
     :certdnsnames => {
       :default => '',
@@ -692,7 +692,7 @@ module Puppet
         a proxy in front of the process or processes, since Mongrel cannot
         speak SSL.",
 
-        :call_on_define => true, # Call our hook with the default value, so we always get the correct bind address set.
+        :call_hook => :on_define_and_write, # Call our hook with the default value, so we always get the correct bind address set.
         :hook => proc { |value|  value == "webrick" ? Puppet.settings[:bindaddress] = "0.0.0.0" : Puppet.settings[:bindaddress] = "127.0.0.1" if Puppet.settings[:bindaddress] == "" }
       }
   )
@@ -1083,7 +1083,7 @@ module Puppet
     },
     :reportserver => {
       :default => "$server",
-      :call_on_define => false,
+      :call_hook => :on_write_only,
       :desc => "(Deprecated for 'report_server') The server to which to send transaction reports.",
       :hook => proc do |value|
         Puppet.settings[:report_server] = value if value
@@ -1202,7 +1202,7 @@ module Puppet
       :desc     => "Where Puppet should look for facts.  Multiple directories should
         be separated by the system path separator character. (The POSIX path separator is ':', and the Windows path separator is ';'.)",
 
-      #:call_on_define => true, # Call our hook with the default value, so we always get the value added to facter.
+      :call_hook => :on_initialize_and_write, # Call our hook with the default value, so we always get the value added to facter.
       :hook => proc { |value| Facter.search(value) if Facter.respond_to?(:search) }}
   )
 
@@ -1434,7 +1434,7 @@ module Puppet
 
 You can adjust the backend using the storeconfigs_backend setting.",
       # Call our hook with the default value, so we always get the libdir set.
-      #:call_on_define => true,
+      :call_hook => :on_initialize_and_write,
       :hook => proc do |value|
         require 'puppet/node'
         require 'puppet/node/facts'

lib/puppet/util/settings.rb

@@ -129,10 +129,24 @@ def initialize_app_defaults(app_defaults)
     app_defaults.each do |key, value|
       set_value(key, value, :application_defaults)
     end
+    call_hooks_deferred_to_application_initialization
 
     @app_defaults_initialized = true
   end
 
+  def call_hooks_deferred_to_application_initialization(options = {})
+    @hooks_to_call_on_application_initialization.each do |setting| 
+      begin
+        setting.handle(self.value(setting.name))
+      rescue Puppet::SettingsError => err
+        raise err unless options[:ignore_interpolation_dependency_errors]
+        #swallow. We're not concerned if we can't call hooks because dependencies don't exist yet
+        #we'll get another chance after application defaults are initialized
+      end
+    end
+  end
+  private :call_hooks_deferred_to_application_initialization
+
   # Do variable interpolation on the value.
   def convert(value, environment = nil)
     return nil if value.nil?
@@ -245,6 +259,8 @@ def initialize
 
     # The list of sections we've used.
     @used = []
+
+    @hooks_to_call_on_application_initialization = []
   end
 
   # NOTE: ACS ahh the util classes. . .sigh
@@ -369,6 +385,11 @@ def parse
     @sync.synchronize do
       unsafe_parse(files)
     end
+
+    # talking with cprice, Settings.parse will not be the final location for this. He's working on ticket
+    # that, as a side effect, will create a more appropriate place for this. At that time, this will be
+    # moved to the new location. --jeffweiss 24 apr 2012
+    call_hooks_deferred_to_application_initialization :ignore_interpolation_dependency_errors => true
   end
 
   def main_config_file
@@ -645,7 +666,7 @@ def set_value(param, value, type, options = {})
     end
 
     value = setting.munge(value) if setting.respond_to?(:munge)
-    setting.handle(value) if setting.respond_to?(:handle) and not options[:dont_trigger_handles]
+    setting.handle(value) if setting.has_hook? and not options[:dont_trigger_handles]
     if ReadOnly.include? param and type != :application_defaults
       raise ArgumentError,
         "You're attempting to set configuration parameter $#{param}, which is read-only."
@@ -742,7 +763,8 @@ def define_settings(section, defs)
       # Collect the settings that need to have their hooks called immediately.
       # We have to collect them so that we can be sure we're fully initialized before
       # the hook is called.
-      call << tryconfig if tryconfig.call_on_define
+      call << tryconfig if tryconfig.call_hook_on_define?
+      @hooks_to_call_on_application_initialization << tryconfig if tryconfig.call_hook_on_initialize?
     }
 
     call.each { |setting| setting.handle(self.value(setting.name)) }
@@ -1015,7 +1037,7 @@ def each_source(environment)
   # Return all settings that have associated hooks; this is so
   # we can call them after parsing the configuration file.
   def settings_with_hooks
-    @config.values.find_all { |setting| setting.respond_to?(:handle) }
+    @config.values.find_all { |setting| setting.has_hook? }
   end
 
   # Extract extra setting information for files.