Merge branch 'dev/cli_logging_fix' into test/ceedling_0_32_rc

bin/app_cfg.rb

@@ -42,7 +42,10 @@ def initialize()
       # Blank initial value for completeness
       :project_config => {},
 
-      # Default, blank value
+      # Default path (in build directory) to hold logs
+      :logging_path => '',
+
+      # If logging enabled, the filepath for Ceedling's log (may be explicitly set to be outside :logging_path)
       :log_filepath => '',
 
       # Only specified in project config (no command line or environment variable)
@@ -76,6 +79,10 @@ def set_project_config(config)
     @app_cfg[:project_config] = config
   end
 
+  def set_logging_path(path)
+    @app_cfg[:logging_path] = path
+  end
+
   def set_log_filepath(filepath)
     @app_cfg[:log_filepath] = filepath
   end

bin/cli.rb

@@ -278,8 +278,9 @@ def upgrade(path)
     method_option :project, :type => :string, :default => nil, :aliases => ['-p'], :desc => DOC_PROJECT_FLAG
     method_option :mixin, :type => :string, :default => [], :repeatable => true, :aliases => ['-m'], :desc => DOC_MIXIN_FLAG
     method_option :verbosity, :type => :string, :default => VERBOSITY_NORMAL, :aliases => ['-v'], :desc => "Sets logging level"
-    method_option :log, :type => :boolean, :default => false, :aliases => ['-l'], :desc => "Enable logging to default filepath in build directory"
-    method_option :logfile, :type => :string, :default => '', :desc => "Enable logging to given filepath"
+    # --log defaults to nil so we can check if it's been set by user as part of --logfile mutually exclusive option check
+    method_option :log, :type => :boolean, :default => nil, :aliases => ['-l'], :desc => "Enable logging to default <build path>/logs/#{DEFAULT_CEEDLING_LOGFILE}"
+    method_option :logfile, :type => :string, :default => nil, :desc => "Enable logging to filepath (ex. foo/bar/mybuild.log)"
     method_option :graceful_fail, :type => :boolean, :default => nil, :desc => "Force exit code of 0 for unit test failures"
     method_option :test_case, :type => :string, :default => '', :desc => "Filter for individual unit test names"
     method_option :exclude_test_case, :type => :string, :default => '', :desc => "Prevent matched unit test names from running"
@@ -309,10 +310,20 @@ def build(*tasks)
       # Get unfrozen copies so we can add / modify
       _options = options.dup()
       _options[:project] = options[:project].dup() if !options[:project].nil?
+      _options[:logfile] = options[:logfile].dup()
       _options[:mixin] = []
       options[:mixin].each {|mixin| _options[:mixin] << mixin.dup() }
       _options[:verbosity] = VERBOSITY_DEBUG if options[:debug]
 
+      # Mutually exclusive options check (Thor does not offer option combination limitations)
+      # If :log is not nil, then the user set it. If :logfile is not empty, the user set it too.
+      if !options[:log].nil? and !options[:logfile].empty?
+        raise Thor::Error, "Use only one of --log(-l) or --logfile"
+      end
+
+      # Force default of false since we use nil as default value in Thor option definition
+      _options[:log] = false if options[:log].nil?
+
       @handler.build( env:ENV, app_cfg:@app_cfg, options:_options, tasks:tasks )
     end
 

bin/cli_handler.rb

@@ -174,10 +174,14 @@ def build(env:, app_cfg:, options:{}, tasks:)
       default_tasks: default_tasks
     )
 
-    log_filepath = @helper.process_logging( options[:log], options[:logfile] )
+    logging_path = @helper.process_logging_path( config )
+    log_filepath = @helper.process_log_filepath( options[:log], logging_path, options[:logfile] )
+
+    @loginator.log( " > Logfile: #{log_filepath}" ) if !log_filepath.empty?
 
     # Save references
     app_cfg.set_project_config( config )
+    app_cfg.set_logging_path( logging_path )
     app_cfg.set_log_filepath( log_filepath )
     app_cfg.set_include_test_case( options[:test_case] )
     app_cfg.set_exclude_test_case( options[:exclude_test_case] )
@@ -240,6 +244,7 @@ def dumpconfig(env, app_cfg, options, filepath, sections)
 
         # Save references
         app_cfg.set_project_config( config )
+        app_cfg.set_logging_path( @helper.process_logging_path( config ) )
 
         _, path = @helper.which_ceedling?( env:env, config:config, app_cfg:app_cfg )
 
@@ -269,6 +274,7 @@ def environment(env, app_cfg, options)
 
     # Save references
     app_cfg.set_project_config( config )
+    app_cfg.set_logging_path( @helper.process_logging_path( config ) )
 
     _, path = @helper.which_ceedling?( env:env, config:config, app_cfg:app_cfg )
 
@@ -446,6 +452,7 @@ def list_rake_tasks(env:, app_cfg:, filepath:nil, mixins:[], silent:false)
 
     # Save reference to loaded configuration
     app_cfg.set_project_config( config )
+    app_cfg.set_logging_path( @helper.process_logging_path( config ) )
 
     _, path = @helper.which_ceedling?( env:env, config:config, app_cfg:app_cfg )
 

bin/cli_helper.rb

@@ -175,18 +175,32 @@ def process_graceful_fail(config:, cmdline_graceful_fail:, tasks:, default_tasks
   end
 
 
-  def process_logging(enabled, filepath)
+  def process_logging_path(config)
+    build_root, _ = @config_walkinator.fetch_value( :project, :build_root, hash:config )
+
+    return '' if build_root.nil?
+
+    return File.join( build_root, 'logs' )
+  end
+
+
+  def process_log_filepath(enabled, logging_path, filepath)
     # No log file if neither enabled nor a specific filename/filepath
-    return '' if !enabled && (filepath.nil? || filepath.empty?())
+    return '' if !enabled && (filepath.nil? || filepath.empty?)
 
-    # Default logfile name (to be placed in default location) if enabled but no filename/filepath
-    return DEFAULT_CEEDLING_LOGFILE if enabled && filepath.empty?()
+    # Default logfile name (to be placed in default location of logging_path) if enabled but no filename/filepath
+    if (enabled && filepath.empty?)
+      filepath = File.join( logging_path, DEFAULT_CEEDLING_LOGFILE )
+    end
 
     # Otherwise, a filename/filepath was provided that implicitly enables logging
+
+    filepath = File.expand_path( filepath )
+
     dir = File.dirname( filepath )
 
     # Ensure logging directory path exists
-    if not dir.empty?
+    if !File.exist?( dir )
       @file_wrapper.mkdir( dir )
     end
 

lib/ceedling/configurator.rb

@@ -609,10 +609,10 @@ def validate_final(config, app_cfg)
 
 
   # Create constants and accessors (attached to this object) from given hash
-  def build(ceedling_lib_path, config, *keys)
+  def build(ceedling_lib_path, logging_path, config, *keys)
     flattened_config = @configurator_builder.flattenify( config )
 
-    @configurator_setup.build_project_config( ceedling_lib_path, flattened_config )
+    @configurator_setup.build_project_config( ceedling_lib_path, logging_path, flattened_config )
 
     @configurator_setup.build_directory_structure( flattened_config )
 

lib/ceedling/configurator_builder.rb

@@ -109,7 +109,7 @@ def cleanup(in_hash)
   end
 
 
-  def set_build_paths(in_hash)
+  def set_build_paths(in_hash, logging_path)
     out_hash = {}
 
     project_build_artifacts_root = File.join(in_hash[:project_build_root], 'artifacts')
@@ -139,7 +139,7 @@ def set_build_paths(in_hash)
       [:project_release_build_output_path,      File.join(project_build_release_root, 'out'),               in_hash[:project_release_build] ],
       [:project_release_dependencies_path,      File.join(project_build_release_root, 'dependencies'),      in_hash[:project_release_build] ],
 
-      [:project_log_path,                       File.join(in_hash[:project_build_root], 'logs'), true ],
+      [:project_log_path,                       logging_path, true ],
 
       [:project_test_preprocess_includes_path,  File.join(project_build_tests_root, 'preprocess/includes'), (in_hash[:project_use_test_preprocessor] != :none) ],
       [:project_test_preprocess_files_path,     File.join(project_build_tests_root, 'preprocess/files'),    (in_hash[:project_use_test_preprocessor] != :none) ],

lib/ceedling/configurator_setup.rb

@@ -30,12 +30,12 @@ def inspect
     return this.class.name
   end
 
-  def build_project_config(ceedling_lib_path, flattened_config)
+  def build_project_config(ceedling_lib_path, logging_path, flattened_config)
     # Housekeeping
     @configurator_builder.cleanup( flattened_config )
 
     # Add to hash values we build up from configuration & file system contents
-    flattened_config.merge!( @configurator_builder.set_build_paths( flattened_config ) )
+    flattened_config.merge!( @configurator_builder.set_build_paths( flattened_config, logging_path ) )
     flattened_config.merge!( @configurator_builder.set_rakefile_components( ceedling_lib_path, flattened_config ) )
     flattened_config.merge!( @configurator_builder.set_release_target( flattened_config ) )
     flattened_config.merge!( @configurator_builder.set_build_thread_counts( flattened_config ) )
@@ -53,7 +53,7 @@ def build_directory_structure(flattened_config)
 
     flattened_config[:project_build_paths].each do |path|
       if path.nil? or path.empty?
-        raise CeedlingException.new( "Blank internal project build path subdirectory value" )
+        raise CeedlingException.new( "An internal project build path subdirectory path is unexpectedly blank" )
       end
 
       @file_wrapper.mkdir( path )

lib/ceedling/setupinator.rb

@@ -54,7 +54,7 @@ def do_setup( app_cfg )
     @configurator.set_verbosity( config_hash )
 
     # Logging configuration
-    @loginator.set_logfile( form_log_filepath( app_cfg[:log_filepath] ) )
+    @loginator.set_logfile( app_cfg[:log_filepath] )
     @configurator.project_logging = @loginator.project_logging
 
     log_step( 'Validating configuration contains minimum required sections', heading:false )
@@ -162,7 +162,7 @@ def do_setup( app_cfg )
     # Skip logging this step as the end user doesn't care about this internal preparation
 
     # Partially flatten config + build Configurator accessors and globals
-    @configurator.build( app_cfg[:ceedling_lib_path], config_hash, :environment )
+    @configurator.build( app_cfg[:ceedling_lib_path], app_cfg[:logging_path], config_hash, :environment )
 
     ##
     ## 8. Final plugins handling
@@ -192,19 +192,6 @@ def reset_defaults(config_hash)
 
 private
 
-  def form_log_filepath( log_filepath )
-    # Bail out early if logging is disabled
-    return log_filepath if log_filepath.empty?()
-
-    # If there's no directory path, put named log file in default location
-    if File.dirname( log_filepath ).empty?()
-      return File.join( @configurator.project_log_path, log_filepath )
-    end
-
-    # Otherwise, log filepath includes a directory (that's already been created)
-    return log_filepath
-  end
-
   # Neaten up a build step with progress message and some scope encapsulation
   def log_step(msg, heading:true)
     if heading