(#7749) Parse command line args and config file before loading app/face

This commit is mostly focused on solving the following chicken-and-egg
problem: the way the code worked before, the command_line class would
try to load an app before doing any parsing of the cli args or the
puppet config file.  However, we want to support being able to load apps
and faces from the libdir... and if your libdir is in a non-standard
location, which you've specified via the command line or config file, then
the command_line class would fail to find any apps that were in your
libdir and exit before it ever even looked at the cli args or the config
file.

To get around this, and to prepare for supporting the ability to run
faces without an application stub, we needed to move the parsing of
the cli args and config file to an earlier point in the lifecycle.

The major changes in this commit are:

* Move the initial parsing of the cli args and the config file out of
application.rb and into command_line.rb so that they happen before
puppet tries to locate apps/faces.

* Remove all of the "should_parse_config" and "should_not_parse_config"
options and tests from the various apps.  These are not relevant any
longer because we always need to parse the config file to make sure
we know where the libdir is before we try to load faces / apps.

* Introduce a new option parser implementation, to supplement and
perhaps eventually replace our use of the Ruby stdlib OptionParser.
The stdlib OptionParser has some very undesirable behaviors that
are not configurable; the new PuppetOptionParser is a wrapper
around an open source parser called "trollop"; it maintains the
same API that OptionParser exposes, but gives us complete control
over what is happening behind the scenes.

Conflicts:

	spec/unit/util/command_line_spec.rb

Author: cprice404

ext/upload_facts.rb

@@ -11,7 +11,6 @@
 require 'puppet/network/http_pool'
 
 class Puppet::Application::UploadFacts < Puppet::Application
-  should_parse_config
   run_mode :master
 
   option('--debug', '-d')

lib/puppet/application.rb

@@ -174,19 +174,6 @@ def controlled_run(&block)
       result
     end
 
-    def should_parse_config
-      @parse_config = true
-    end
-
-    def should_not_parse_config
-      @parse_config = false
-    end
-
-    def should_parse_config?
-      @parse_config = true if ! defined?(@parse_config)
-      @parse_config
-    end
-
     # used to declare code that handle an option
     def option(*options, &block)
       long = options.find { |opt| opt =~ /^--/ }.gsub(/^--(?:\[no-\])?([^ =]+).*$/, '\1' ).gsub('-','_')
@@ -230,7 +217,7 @@ def [](name)
     end
 
     #
-    # TODO: look into changing this into two methods (getter/setter)
+    # TODO cprice: look into changing this into two methods (getter/setter)
     #  --cprice 2012-03-06
     #
 
@@ -259,10 +246,6 @@ def run_mode( mode_name = nil)
     exit
   end
 
-  def should_parse_config?
-    self.class.should_parse_config?
-  end
-
   # override to execute code before running anything else
   def preinit
   end
@@ -291,15 +274,15 @@ def set_run_mode(mode)
     $puppet_application_mode = @run_mode
     $puppet_application_name = name
 
-    # XXXXXXXXXXX TODO: this is crap.  need to refactor this so that applications have a hook to initialize
+    # XXXXXXXXXXX TODO cprice: this is crap.  need to refactor this so that applications have a hook to initialize
     #  the settings that are specific to them; need to decide whether to formally specify the list of such
     #  settings, or just allow them to run willy-nilly.
     if (mode.name == :master)
       Puppet.settings.set_value(:facts_terminus, "yaml", :mutable_defaults)
     end
 
 
-    ## TODO: get rid of this whole block, push it to some kind of application-specific hook or something
+    ## TODO cprice: get rid of this whole block, push it to some kind of application-specific hook or something
 
     if Puppet.respond_to? :settings
       # This is to reduce the amount of confusion in rspec
@@ -316,12 +299,11 @@ def set_run_mode(mode)
 
   # This is the main application entry point
   def run
-    exit_on_fail("initialize")                                   { hook('preinit')       { preinit } }
-    exit_on_fail("parse options")                                { hook('parse_options') { parse_options } }
-    exit_on_fail("parse configuration file")                     { Puppet.settings.parse } if should_parse_config?
-    exit_on_fail("prepare for execution")                        { hook('setup')         { setup } }
+    exit_on_fail("initialize")                                   { plugin_hook('preinit')       { preinit } }
+    exit_on_fail("parse application options")                    { plugin_hook('parse_options') { parse_options } }
+    exit_on_fail("prepare for execution")                        { plugin_hook('setup')         { setup } }
     exit_on_fail("configure routes from #{Puppet[:route_file]}") { configure_indirector_routes }
-    exit_on_fail("run")                                          { hook('run_command')   { run_command } }
+    exit_on_fail("run")                                          { plugin_hook('run_command')   { run_command } }
   end
 
   def main
@@ -362,6 +344,11 @@ def parse_options
     # Create an option parser
     option_parser = OptionParser.new(self.class.banner)
 
+    # TODO cprice: document this better.  We need to include the globals so that the app
+    #  has an opportunity to override them.
+    # Might be able to make this a little more efficient by sharing the Parser object
+    #  betwween the command line class and this one.
+    #
     # Add all global options to it.
     Puppet.settings.optparse_addargs([]).each do |option|
       option_parser.on(*option) do |arg|
@@ -385,22 +372,11 @@ def parse_options
     option_parser.parse!(self.command_line.args)
   end
 
-  def handlearg(opt, arg)
-    # rewrite --[no-]option to --no-option if that's what was given
-    if opt =~ /\[no-\]/ and !arg
-      opt = opt.gsub(/\[no-\]/,'no-')
-    end
-    # otherwise remove the [no-] prefix to not confuse everybody
-    opt = opt.gsub(/\[no-\]/, '')
-    unless respond_to?(:handle_unknown) and send(:handle_unknown, opt, arg)
-      # Puppet.settings.handlearg doesn't handle direct true/false :-)
-      if arg.is_a?(FalseClass)
-        arg = "false"
-      elsif arg.is_a?(TrueClass)
-        arg = "true"
-      end
-      Puppet.settings.handlearg(opt, arg)
-    end
+
+
+  def handlearg(opt, val)
+    opt, val = Puppet::Util::CommandLine.clean_opt(opt, val)
+    send(:handle_unknown, opt, val) if respond_to?(:handle_unknown)
   end
 
   # this is used for testing
@@ -416,20 +392,14 @@ def help
     "No help available for puppet #{name}"
   end
 
-  private
 
-  def exit_on_fail(message, code = 1)
-    yield
-  rescue ArgumentError, RuntimeError, NotImplementedError => detail
-    Puppet.log_exception(detail, "Could not #{message}: #{detail}")
-    exit(code)
-  end
 
-  def hook(step,&block)
+  def plugin_hook(step,&block)
     Puppet::Plugins.send("before_application_#{step}",:application_object => self)
     x = yield
     Puppet::Plugins.send("after_application_#{step}",:application_object => self, :return_value => x)
     x
   end
+  private :plugin_hook
 end
 end

lib/puppet/application/agent.rb

@@ -2,7 +2,6 @@
 
 class Puppet::Application::Agent < Puppet::Application
 
-  should_parse_config
   run_mode :agent
 
   attr_accessor :args, :agent, :daemon, :host

lib/puppet/application/apply.rb

@@ -2,8 +2,6 @@
 
 class Puppet::Application::Apply < Puppet::Application
 
-  should_parse_config
-
   option("--debug","-d")
   option("--execute EXECUTE","-e") do |arg|
     options[:code] = arg

lib/puppet/application/cert.rb

@@ -2,7 +2,6 @@
 
 class Puppet::Application::Cert < Puppet::Application
 
-  should_parse_config
   run_mode :master
 
   attr_accessor :all, :ca, :digest, :signed

lib/puppet/application/describe.rb

@@ -170,8 +170,6 @@ def list_providers(type)
 class Puppet::Application::Describe < Puppet::Application
   banner "puppet describe [options] [type]"
 
-  should_not_parse_config
-
   option("--short", "-s", "Only list parameters without detail") do |arg|
     options[:parameters] = false
   end

lib/puppet/application/device.rb

@@ -4,7 +4,6 @@
 
 class Puppet::Application::Device < Puppet::Application
 
-  should_parse_config
   run_mode :agent
 
   attr_accessor :args, :agent, :host

lib/puppet/application/doc.rb

@@ -1,7 +1,6 @@
 require 'puppet/application'
 
 class Puppet::Application::Doc < Puppet::Application
-  should_not_parse_config
   run_mode :master
 
   attr_accessor :unknown_args, :manifest

lib/puppet/application/face_base.rb

@@ -4,7 +4,6 @@
 require 'pp'
 
 class Puppet::Application::FaceBase < Puppet::Application
-  should_parse_config
   run_mode :agent
 
   option("--debug", "-d") do |arg|

lib/puppet/application/filebucket.rb

@@ -2,8 +2,6 @@
 
 class Puppet::Application::Filebucket < Puppet::Application
 
-  should_not_parse_config
-
   option("--bucket BUCKET","-b")
   option("--debug","-d")
   option("--local","-l")