GH4991: Read verbosity from env in ICakeLog registration

[paulomorgado]

Follow up from #4947.

PR Classification

Dependency injection behavior update to support configurable logging verbosity.

PR Summary

The registration of ICakeLog in the DI container now uses a factory that reads verbosity from the CAKE_SETTINGS_VERBOSITY environment variable, defaulting to Normal if parsing fails.

• Changed DI registration for ICakeLog from a direct singleton to a factory method.
• Factory method parses CAKE_SETTINGS_VERBOSITY for Verbosity enum, with fallback to Verbosity.Normal.
• CakeBuildLog is instantiated with the resolved verbosity using ActivatorUtilities.

@devlead, what tests should I add?

[devlead]

For Frosting, the Verbosity is set as part of execution here

cake/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs

Line 55 in 59eb19d

log.Verbosity = settings.Verbosity;

So setting it in IoC isn't going to change it, as it's a singleton and set before execution.

The same pattern is used in Cake Tool

cake/src/Cake/Commands/DefaultCommand.cs

Line 66 in 59eb19d

_log.Verbosity = settings.Verbosity;

cake/src/Cake/Features/Bootstrapping/BootstrapFeature.cs

Line 75 in 59eb19d

log.Verbosity = settings.Verbosity;

cake/src/Cake/Features/Building/BuildFeature.cs

Line 121 in 59eb19d

log.Verbosity = settings.Verbosity;

And Cake SDK
https://github.com/cake-build/generator/blob/2cac5f5c6fb4b2e53e43352059b2278e3957a0d7/src/Cake.Generator.Core/CakeGenerator.Generate.Helper.Services.PostBuildServiceProvider.cs#L44

Also, using System.Environment.GetEnvironmentVariable makes it untestable; for configuration in Cake, we use ICakeConfiguration as it comes from Cake IOC; it's testable (uses Cake environment and args and replaceable by Cake Modules. ICakeConfiguration also handles configuration via files, arguments, and environment variables. So it's idiomatic with how other settings are handled within Cake. And if we introduce a new configuration setting, we want, as far as possible and where it makes sense, to make it available for all Cake runners.

So a more correct change would be something roughly like below: a common helper method that lets you override verbosity via configuration and then falls back to settings (default command line) if no config is available

diff --git a/src/Cake.Core/Constants.cs b/src/Cake.Core/Constants.cs
index 7a202ca2..f2d1edf6 100644
--- a/src/Cake.Core/Constants.cs
+++ b/src/Cake.Core/Constants.cs
@@ -20,6 +20,7 @@ namespace Cake.Core
             public const string NoMonoCoersion = "Settings_NoMonoCoersion";
             public const string ShowProcessCommandLine = "Settings_ShowProcessCommandLine";
             public const string UnifiedDependencyGraphForMultipleTargets = "Settings_UnifiedDependencyGraphForMultipleTargets";
+            public const string Verbosity = "Settings_Verbosity";
         }
 
         public static class Paths
diff --git a/src/Cake.Core/Extensions/CakeConfigurationExtensions.cs b/src/Cake.Core/Extensions/CakeConfigurationExtensions.cs
index 0462334d..9bf32c3f 100644
--- a/src/Cake.Core/Extensions/CakeConfigurationExtensions.cs
+++ b/src/Cake.Core/Extensions/CakeConfigurationExtensions.cs
@@ -1,6 +1,7 @@
 using System;
 
 using Cake.Core.Configuration;
+using Cake.Core.Diagnostics;
 using Cake.Core.IO;
 
 // ReSharper disable once CheckNamespace
@@ -64,5 +65,20 @@ namespace Cake.Core
             var toolPath = configuration.GetToolPath(defaultRoot, environment).ExpandShortPath();
             return toolPath.Combine("Modules").Collapse();
         }
+
+        /// <summary>
+        /// Gets the verbosity level from the configuration.
+        /// </summary>
+        /// <param name="configuration">The Cake configuration.</param>
+        /// <returns>The configured verbosity level, or <paramref name="defaultValue"/> if not found or invalid.</returns>
+        public static Verbosity GetVerbosity(this ICakeConfiguration configuration, Verbosity defaultValue)
+        {
+            var verbosity = configuration.GetValue(Constants.Settings.Verbosity);
+            if (!string.IsNullOrWhiteSpace(verbosity) && Enum.TryParse<Verbosity>(verbosity, true, out var result))
+            {
+                return result;
+            }
+            return defaultValue;
+        }
     }
 }
diff --git a/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs b/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs
index b1de7d14..4e7ae75c 100644
--- a/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs
+++ b/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs
@@ -7,6 +7,7 @@ using System.Linq;
 using Cake.Cli;
 using Cake.Cli.Infrastructure;
 using Cake.Core;
+using Cake.Core.Configuration;
 using Cake.Core.Diagnostics;
 using Cake.Core.IO;
 using Cake.Core.Packaging;
@@ -52,7 +53,8 @@ namespace Cake.Frosting.Internal
 
                 // Set the log verbosity
                 var log = provider.GetRequiredService<ICakeLog>();
-                log.Verbosity = settings.Verbosity;
+                var configuration = provider.GetRequiredService<ICakeConfiguration>();
+                log.Verbosity = configuration.GetVerbosity(settings.Verbosity);
 
                 // Set the working directory
                 SetWorkingDirectory(provider, settings);
diff --git a/src/Cake/Commands/DefaultCommand.cs b/src/Cake/Commands/DefaultCommand.cs
index 4082aa68..d431b700 100644
--- a/src/Cake/Commands/DefaultCommand.cs
+++ b/src/Cake/Commands/DefaultCommand.cs
@@ -7,6 +7,7 @@ using System.Collections.Generic;
 using Cake.Cli;
 using Cake.Cli.Infrastructure;
 using Cake.Core;
+using Cake.Core.Configuration;
 using Cake.Core.Diagnostics;
 using Cake.Features.Bootstrapping;
 using Cake.Features.Building;
@@ -25,6 +26,7 @@ namespace Cake.Commands
         private readonly ICakeInfoFeature _info;
         private readonly IConsole _console;
         private readonly ICakeLog _log;
+        private readonly ICakeConfiguration _configuration;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="DefaultCommand"/> class.
@@ -35,18 +37,21 @@ namespace Cake.Commands
         /// <param name="info">The info feature.</param>
         /// <param name="console">The console.</param>
         /// <param name="log">The log.</param>
+        /// <param name="configuration">The configuration.</param>
         public DefaultCommand(
             IBuildFeature builder,
             IBootstrapFeature bootstrapper,
             ICakeVersionFeature version,
             ICakeInfoFeature info,
             IConsole console,
-            ICakeLog log)
+            ICakeLog log,
+            ICakeConfiguration configuration)
         {
             _builder = builder;
             _bootstrapper = bootstrapper;
             _version = version;
             _info = info;
+            _configuration = configuration;
             _console = console;
             _log = log;
         }
@@ -63,7 +68,7 @@ namespace Cake.Commands
             try
             {
                 // Set log verbosity.
-                _log.Verbosity = settings.Verbosity;
+                _log.Verbosity = _configuration.GetVerbosity(settings.Verbosity);
 
                 if (settings.ShowVersion)
                 {
@@ -82,7 +87,7 @@ namespace Cake.Commands
                 // Run the bootstrapper?
                 if (!settings.SkipBootstrap || settings.Bootstrap)
                 {
-                    int bootstrapperResult = PerformBootstrapping(context, settings, host);
+                    int bootstrapperResult = PerformBootstrapping(context, settings, host, _log.Verbosity);
                     if (bootstrapperResult != 0 || settings.Bootstrap)
                     {
                         return bootstrapperResult;
@@ -125,7 +130,7 @@ namespace Cake.Commands
             return BuildHostKind.Build;
         }
 
-        private int PerformBootstrapping(CommandContext context, DefaultCommandSettings settings, BuildHostKind host)
+        private int PerformBootstrapping(CommandContext context, DefaultCommandSettings settings, BuildHostKind host, Verbosity verbosity)
         {
             if (host != BuildHostKind.Build && host != BuildHostKind.DryRun)
             {
@@ -137,7 +142,7 @@ namespace Cake.Commands
             return _bootstrapper.Run(arguments, new BootstrapFeatureSettings
             {
                 Script = settings.Script,
-                Verbosity = settings.Verbosity
+                Verbosity = verbosity
             });
         }
 
diff --git a/src/Cake/Features/Bootstrapping/BootstrapFeature.cs b/src/Cake/Features/Bootstrapping/BootstrapFeature.cs
index f42f2cf6..281526e8 100644
--- a/src/Cake/Features/Bootstrapping/BootstrapFeature.cs
+++ b/src/Cake/Features/Bootstrapping/BootstrapFeature.cs
@@ -6,6 +6,7 @@ using System;
 using System.Linq;
 using Autofac;
 using Cake.Core;
+using Cake.Core.Configuration;
 using Cake.Core.Diagnostics;
 using Cake.Core.IO;
 using Cake.Core.Scripting;
@@ -72,7 +73,7 @@ namespace Cake.Features.Bootstrapping
 
                 // Set log verbosity for log in new scope.
                 var log = scope.Resolve<ICakeLog>();
-                log.Verbosity = settings.Verbosity;
+                log.Verbosity = configuration.GetVerbosity(settings.Verbosity);
 
                 // Get the root directory.
                 var root = settings.Script.GetDirectory();
diff --git a/src/Cake/Features/Building/BuildFeature.cs b/src/Cake/Features/Building/BuildFeature.cs
index 7988d6b0..bc1fa62b 100644
--- a/src/Cake/Features/Building/BuildFeature.cs
+++ b/src/Cake/Features/Building/BuildFeature.cs
@@ -118,7 +118,7 @@ namespace Cake.Features.Building
 
                 // Set log verbosity for log in new scope.
                 var log = scope.Resolve<ICakeLog>();
-                log.Verbosity = settings.Verbosity;
+                log.Verbosity = configuration.GetVerbosity(settings.Verbosity);
 
                 // Create the script host.
                 var host = CreateScriptHost(settings, scope);

[paulomorgado]

Got it!

[paulomorgado]

@devlead, thinking deeper about this, this would have the Settings_Verbosity be the override and not the default, and I think it should be the other way around.

Could DefaultCommandSettings.Verbosity have a default of default(Verbosity?) instead of Verbosity.Normal?

[devlead]

Could DefaultCommandSettings.Verbosity have a default of default(Verbosity?) instead of Verbosity.Normal?

Probably, but changing a type is generally a breaking change, and needs more thought.
Verbosity.Normal is the default verbosity, so one could argue if it's not normal, then it has changed.

That said other configuration settings override default behavior by being present, so would follow how other configuration works.
https://cakebuild.net/docs/running-builds/configuration/set-configuration-values

You can always override a environmental config value with a argument.

[paulomorgado]

You can always override a environmental config value with a argument.

That's not the case for verbosity, because, by the time you are setting it, you don't know if the verbosity was set as an argument or is the default for DefaultCommandSettings.Verbosity.

The best I could get, so far, is:

if (Enum.TryParse<Verbosity>(Environment.GetEnvironmentVariable("CAKE_SETTINGS_VERBOSITY"), true, out var verbosity)
    && !args.Any(arg =>
        arg.StartsWith("--verbosity=", StringComparison.OrdinalIgnoreCase)
        || arg.Equals("--verbosity", StringComparison.OrdinalIgnoreCase)
        || arg.Equals("-v", StringComparison.OrdinalIgnoreCase)))
{
    args = [..args, $"--verbosity={verbosity}"];
}

return new CakeHost()
    ...
    .Run(args);

[devlead]

Thanks for kicking this off, @paulomorgado, and you were right about default(Verbosity?).

Digging in, the environment read on its own couldn't take effect: DefaultCommand assigns
log.Verbosity = settings.Verbosity immediately after resolving the log, and with
[DefaultValue(Verbosity.Normal)] an omitted --verbosity is indistinguishable from an explicit
--verbosity normal, so the value from the environment was overwritten with Normal a moment later.
Making the option nullable, exactly as you suggested, is what makes "not specified" expressible, and it means none of the argument sniffing from your snippet is needed.

I've pushed to your branch, rebased on latest develop and squashed to a single commit with full attribution to you.

The main changes
on top of your original:

• Read through ICakeConfiguration rather than System.Environment, under a new
  Settings_Verbosity key. That picks up cake.config and arguments as well as
  CAKE_SETTINGS_VERBOSITY, follows the documented configuration precedence, and goes via
  ICakeEnvironment so it's testable.
• --verbosity is now Verbosity? on both hosts, with the merge in a new
  ICakeConfiguration.GetVerbosity(...): command line first, then configuration, then Normal.
  On the breaking-change concern, this is confined to the Cake.Tool assembly and Frosting's
  internal settings type, so nothing referenceable changes shape.
• Cake.Tool as well as Frosting. BuildFeature and BootstrapFeature apply the merged
  verbosity to the host log as well as the script scope log, so host-level output (top-level
  exceptions, assembly resolution) honours configuration too, not just what the script logs.
• Frosting resolves configuration after the working directory is set, so a cake.config under
  --working / UseWorkingDirectory is honoured. Command line verbosity is applied before
  SetWorkingDirectory, so failures there still log at the level you asked for.
• Shared alias parsing. The lookup in VerbosityConverter moved out to
  Cake.Core.Diagnostics.VerbosityParser, so configuration accepts the same q/m/n/v/d
  spellings as the CLI. Enum.TryParse would also have accepted numerics like 2, which this rejects.

On your question about what tests to add: unit coverage for VerbosityParser and GetVerbosity in
Cake.Core.Tests, precedence tests in ProgramTests, BuildFeatureTests and CakeHostTests
(including cake.config inside vs. outside the working directory), plus integration tests
exercising both the script runner and Frosting end to end.

One cosmetic regression I noticed from the nullable option: an invalid value now doesn't report the underlying type by its CLR name, this is likely best a upstream Sprectre.Console.Cli fix, so something that can be chased down separately.

So all in all PR LGTM and if CI passes it'll be merged.

[devlead]

@paulomorgado your changes have been merged, thanks for your contribution 👍