From 5dfca043eb5df6d578336aaa00f25c9ffad3dfc6 Mon Sep 17 00:00:00 2001 From: Michael Karlesky Date: Wed, 9 Oct 2024 23:20:01 -0400 Subject: [PATCH 1/5] Fixed CLI logging options --- bin/app_cfg.rb | 9 ++++++++- bin/cli.rb | 14 ++++++++++++-- bin/cli_handler.rb | 6 +++++- bin/cli_helper.rb | 24 +++++++++++++++++++----- lib/ceedling/configurator.rb | 4 ++-- lib/ceedling/configurator_builder.rb | 4 ++-- lib/ceedling/configurator_setup.rb | 4 ++-- lib/ceedling/setupinator.rb | 17 ++--------------- 8 files changed, 52 insertions(+), 30 deletions(-) diff --git a/bin/app_cfg.rb b/bin/app_cfg.rb index 0034478e..d04a28c2 100644 --- a/bin/app_cfg.rb +++ b/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 diff --git a/bin/cli.rb b/bin/cli.rb index ec224b69..faa9d0f0 100644 --- a/bin/cli.rb +++ b/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 to true or false + method_option :log, :type => :boolean, :default => nil, :aliases => ['-l'], :desc => "Enable logging to default filepath in build directory" + method_option :logfile, :type => :string, :default => nil, :desc => "Enable logging to given filepath" 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,19 @@ 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 this for option checking) + 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 diff --git a/bin/cli_handler.rb b/bin/cli_handler.rb index bfa5533f..d21b6996 100644 --- a/bin/cli_handler.rb +++ b/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] ) diff --git a/bin/cli_helper.rb b/bin/cli_helper.rb index 1cd79f66..f6e69353 100644 --- a/bin/cli_helper.rb +++ b/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 diff --git a/lib/ceedling/configurator.rb b/lib/ceedling/configurator.rb index 590356e8..ac133807 100644 --- a/lib/ceedling/configurator.rb +++ b/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 ) diff --git a/lib/ceedling/configurator_builder.rb b/lib/ceedling/configurator_builder.rb index 13b91f90..306f3e1a 100644 --- a/lib/ceedling/configurator_builder.rb +++ b/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) ], diff --git a/lib/ceedling/configurator_setup.rb b/lib/ceedling/configurator_setup.rb index 2e60dd87..f7911d5c 100644 --- a/lib/ceedling/configurator_setup.rb +++ b/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 ) ) diff --git a/lib/ceedling/setupinator.rb b/lib/ceedling/setupinator.rb index 07c12e4a..1940e52b 100644 --- a/lib/ceedling/setupinator.rb +++ b/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 From 694d61946bc17c0cbef0a33e202e333b2bdecd91 Mon Sep 17 00:00:00 2001 From: Michael Karlesky Date: Thu, 10 Oct 2024 11:01:02 -0400 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=90=9B=20Propogated=20logging=20path?= =?UTF-8?q?=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- bin/cli.rb | 9 +++++---- bin/cli_handler.rb | 3 +++ lib/ceedling/configurator_setup.rb | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/bin/cli.rb b/bin/cli.rb index faa9d0f0..f63cc7f8 100644 --- a/bin/cli.rb +++ b/bin/cli.rb @@ -278,9 +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" - # --log defaults to nil so we can check if it's been set to true or false - method_option :log, :type => :boolean, :default => nil, :aliases => ['-l'], :desc => "Enable logging to default filepath in build directory" - method_option :logfile, :type => :string, :default => nil, :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 /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" @@ -315,7 +315,8 @@ def build(*tasks) options[:mixin].each {|mixin| _options[:mixin] << mixin.dup() } _options[:verbosity] = VERBOSITY_DEBUG if options[:debug] - # Mutually exclusive options check (Thor does not offer this for option checking) + # 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 diff --git a/bin/cli_handler.rb b/bin/cli_handler.rb index d21b6996..756ba974 100644 --- a/bin/cli_handler.rb +++ b/bin/cli_handler.rb @@ -244,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 ) @@ -273,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 ) @@ -450,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 ) diff --git a/lib/ceedling/configurator_setup.rb b/lib/ceedling/configurator_setup.rb index f7911d5c..934d3e17 100644 --- a/lib/ceedling/configurator_setup.rb +++ b/lib/ceedling/configurator_setup.rb @@ -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 ) From f8c88cd75f6b6a98c6c8504dc4cd2d0b064aca02 Mon Sep 17 00:00:00 2001 From: Michael Karlesky Date: Thu, 10 Oct 2024 11:35:55 -0400 Subject: [PATCH 3/5] =?UTF-8?q?=F0=9F=93=9D=20Updated=20CLI=20help=20for?= =?UTF-8?q?=20buid=20application=20command?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- bin/cli.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/cli.rb b/bin/cli.rb index f63cc7f8..f834a946 100644 --- a/bin/cli.rb +++ b/bin/cli.rb @@ -304,6 +304,9 @@ def upgrade(path) • `--test-case` and its inverse `--exclude-test-case` set test case name matchers to run only a subset of the unit test suite. See docs for full details. + + • The simple `--log` flag and more configurable `--logfile ` are + mutually exclusive. It is valid to use one or the other but not both. LONGDESC ) ) def build(*tasks) From fb54c9f0d400f12b53c143a4bb279ed5f8de3f56 Mon Sep 17 00:00:00 2001 From: Michael Karlesky Date: Thu, 10 Oct 2024 11:42:40 -0400 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=93=9D=20Tweaked=20CLI=20help?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- bin/cli.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/cli.rb b/bin/cli.rb index f834a946..b48b3443 100644 --- a/bin/cli.rb +++ b/bin/cli.rb @@ -305,8 +305,8 @@ def upgrade(path) • `--test-case` and its inverse `--exclude-test-case` set test case name matchers to run only a subset of the unit test suite. See docs for full details. - • The simple `--log` flag and more configurable `--logfile ` are - mutually exclusive. It is valid to use one or the other but not both. + • The simple `--log` flag and more configurable `--logfile` are mutually + exclusive. It is valid to use one or the other but not both. LONGDESC ) ) def build(*tasks) From 2e94ec60b5bab63b6ea625ae336e56e8b503b353 Mon Sep 17 00:00:00 2001 From: Michael Karlesky Date: Thu, 10 Oct 2024 14:36:30 -0400 Subject: [PATCH 5/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Simplified=20logging?= =?UTF-8?q?=20file=20CLI=20options?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- bin/cli.rb | 45 +++++++++++++++++++++++++++++++-------------- bin/cli_handler.rb | 6 ++++-- bin/cli_helper.rb | 23 +++++++++++++---------- 3 files changed, 48 insertions(+), 26 deletions(-) diff --git a/bin/cli.rb b/bin/cli.rb index b48b3443..1b56a4a0 100644 --- a/bin/cli.rb +++ b/bin/cli.rb @@ -138,6 +138,9 @@ module CeedlingTasks specified YAML file. See documentation for complete details. \x5> --mixin my_compiler --mixin my/path/mixin.yml" + # Intentionally disallowed Linux/Unix/Windows filename characters to avoid mistakenly filtering --logfile default + MISSING_LOGFILE_DEFAULT = "/<>\\||*" + class CLI < Thor include Thor::Actions extend PermissiveCLI @@ -278,9 +281,10 @@ 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" - # --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 /logs/#{DEFAULT_CEEDLING_LOGFILE}" - method_option :logfile, :type => :string, :default => nil, :desc => "Enable logging to filepath (ex. foo/bar/mybuild.log)" + # :default, :lazy_default & :check_default_type: allow us to encode 3 possible logfile scenarios (see special handling comments below) + method_option :logfile, :type => :string, :aliases => ['-l'], + :default => false, :lazy_default => MISSING_LOGFILE_DEFAULT, :check_default_type => false, + :desc => "Enables logging to /logs/#{DEFAULT_CEEDLING_LOGFILE} if blank or to specified filepath" 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" @@ -304,29 +308,41 @@ def upgrade(path) • `--test-case` and its inverse `--exclude-test-case` set test case name matchers to run only a subset of the unit test suite. See docs for full details. - - • The simple `--log` flag and more configurable `--logfile` are mutually - exclusive. It is valid to use one or the other but not both. LONGDESC ) ) 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" + # Set something to be reset below + _options[:logfile] = nil + + # Handle the 3 logging option values: + # 1. No logging (default) + # 2. Enabling logging with default log filepath + # 3. Explicitly set log filepath + case options[:logfile] + + # Match against Thor's :lazy_default for --logging flag with missing value + when MISSING_LOGFILE_DEFAULT + # Enable logging to default log filepath + _options[:logfile] = true + + # Match against missing --logging flag + when false + # No logging + _options[:logfile] = false + + # Copy in explicitly provided filepath from --logging= + else + # Filepath to explicitly set log file + _options[:logfile] = options[:logfile].dup() 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 @@ -396,6 +412,7 @@ def environment() @handler.environment( ENV, @app_cfg, _options ) end + desc "examples", "List available example projects" method_option :debug, :type => :boolean, :default => false, :hide => true long_desc( CEEDLING_HANDOFF_OBJECTS[:loginator].sanitize( diff --git a/bin/cli_handler.rb b/bin/cli_handler.rb index 756ba974..8222ec01 100644 --- a/bin/cli_handler.rb +++ b/bin/cli_handler.rb @@ -160,7 +160,9 @@ def upgrade_project(env, app_cfg, options, path) def build(env:, app_cfg:, options:{}, tasks:) @helper.set_verbosity( options[:verbosity] ) - @path_validator.standardize_paths( options[:project], options[:logfile], *options[:mixin] ) + @path_validator.standardize_paths( options[:project], *options[:mixin] ) + + @path_validator.standardize_paths( options[:logfile] ) if options[:logfile].class == String _, config = @configinator.loadinate( builtin_mixins:BUILTIN_MIXINS, filepath:options[:project], mixins:options[:mixin], env:env ) @@ -175,7 +177,7 @@ def build(env:, app_cfg:, options:{}, tasks:) ) logging_path = @helper.process_logging_path( config ) - log_filepath = @helper.process_log_filepath( options[:log], logging_path, options[:logfile] ) + log_filepath = @helper.process_log_filepath( logging_path, options[:logfile] ) @loginator.log( " > Logfile: #{log_filepath}" ) if !log_filepath.empty? diff --git a/bin/cli_helper.rb b/bin/cli_helper.rb index f6e69353..95e00331 100644 --- a/bin/cli_helper.rb +++ b/bin/cli_helper.rb @@ -184,18 +184,21 @@ def process_logging_path(config) 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?) - - # Default logfile name (to be placed in default location of logging_path) if enabled but no filename/filepath - if (enabled && filepath.empty?) + def process_log_filepath(logging_path, filepath) + case filepath + # No logging + when false + return '' + + # Default logfile path if no filename/filepath + when true filepath = File.join( logging_path, DEFAULT_CEEDLING_LOGFILE ) - end - - # Otherwise, a filename/filepath was provided that implicitly enables logging + filepath = File.expand_path( filepath ) - filepath = File.expand_path( filepath ) + # Otherwise, explcit filename/filepath provided that implicitly enables logging + else + filepath = File.expand_path( filepath ) + end dir = File.dirname( filepath )