Added support for logger parameters to MSBuild.

[alphaleonis]

Added ConsoleLogger and FileLogger to MSBuildSettings, allowing fluent API specification of parameters for file loggers and the console logger when running MSBuild.

Example:

            MSBuild(s => s
                .SetTargetPath(Solution)
                .SetTargets("Rebuild")
                .SetConfiguration(Configuration)
                .AddFileLogger(l => l
                     .SetLogFile("D:\\mylog.log")
                     .DisableAppend()
                     .SetVerbosity(MSBuildVerbosity.Normal)
                     .EnableShowTimestamp()
                     .EnablePerformanceSummary()
                )
                .SetMaxCpuCount(Environment.ProcessorCount)
                .SetNodeReuse(IsLocalBuild));

Fixes nuke-build/nuke#314

[alphaleonis]

I have no idea what the check failure on macOS is about unfortunately. Is there something I did wrong?

[matkoch]

This is exactly the part I want the code generation to take over

[alphaleonis]

Yeah, I agree that that would be nice. Question is if this case is common enough to warrant the effort to write code generation for it? If it is, then do you have any ideas for how the specifications would best be extended to support this?

[alphaleonis]

If you don't have any initial input, I'll sit down and try to sketch up a suggestion about how the json schema could look to support this, and get back to you to see what you think.

[matkoch]

Also this should be done by code generation

[matkoch]

Error on macOS is unrelated.

[matkoch]

FYI, it will take a while until I can get into this. In the next release, I plan to do some work around CI system integration. Only after that iteration, I will get to merge this (unless it would look all perfect and shiny).

[alphaleonis]

No worries. I'll try to come up with a suggestion on how this could be handled by code generation in the meantime, and post a suggestion once I come up with something I feel good about.

[alphaleonis]

I have now spent some time thinking about how code generation in Nuke could be extended to support this scenario, and I've come up with the following five enhancements that I think would make sense and could be useful also outside of this limited case.

I would be more than happy to implement some or all of these and submit a new PR if you like. But first of course I would like your input on these ideas @matkoch, when you've got the time to review them:

1. Conditional Formatting

This is a feature that allows the selection of the format of an argument based on the value of that argument, similar to a switch statement in C#. This could be useful outside of this particular feature for any CLI tool that has mutually exclusive parameters to turn a feature on or off. For example -EnableMPLogging and -DisableMPLogging. Based on how the configuration API in Nuke is generated, it would be nice to have a simple property named MPLogging, which would generate the configurator methods EnableMPLogging() and DisableMPLogging(), instead of having two properties, that would generate the kind of weird looking EnableEnableMPLogging(), DisableEnableMPLogging() etc.

The idea is to extend the "format" specifiers of the properties in json specifications to allow a complex format specifiecation in addition to a simple string.

The various format alternatives would be specified in an array like the following:

[
    {
        "case": true,
        "then": "/ShowSummary"
    },
    {
        "case": false,
        "then": "/NoSummary"
    },
    {
        "case": null,
        "then": "/UseNull"
    },
    {
        "default": "/DoNothing"
    }
]

The value after a case could be a bool value, null or a string. If no case matches, then the default is used if specified, otherwise nothing is output.

Alternative 1 - Array format

The format accepts also an array of cases, and a new property switchOn is introduced to indicate the value to switch on.

The advantage of this variant is that the format specifier is kept fairly compact, while allowing to decide which value to switch on. (The default would be {value} unless specified, but {key} could also be specified here if desired).

Example:

{
    "name": "ShowSummary",
    "type": "bool",
    "switchOn": "{value}",
    "format": [
        {
            "case": true,
            "then": "/ShowSummary"
        },
        {
            "case": false,
            "then": "/NoSummary"
        },
        {
            "default": "/Dummy"
        }
    ]
}

Alternative 2 - Object format

In this alternative the format specifier is an object instead of an array, encapsulating all information about the format specifier inside that object. The on property here decides which value to switch on in the subsequent cases. Again, the default if not specified would be {value}.

The advantage of this variant is that the format is more encapsulated than in alternative 1 above, in that we no longer have the "switchOn" property a level above the format, but instead everything related to the format is inside that object. It also allows for easier extension of other types of formatting in the future, by just adding a new keyword in addition to switch.

A possible disadvantage is that it's a bit more verbose to write.

 {
    "name": "ShowSummary",
    "type": "bool",
    "format": {
        "switch": {
            "on": "{value}",
            "cases": [
                {
                    "case": true,
                    "then": "/ShowSummary"
                },
                {
                    "case": false,
                    "then": "/NoSummary"
                },
                {
                    "default": "/Dummy"
                }
            ]
        }
    }
 }

Personally I am in favor of the 2:nd alternative, object format here.

2. Index of array item in format specifier

Introduce the {index} format specifier to include the index of the item in the list. In the MSBuild scenario we need this index to be 1-based, but in general there could be scenarios where one would want this 0-based instead, so also introduce a way to select the base of the index, or include this in the format specifier.

Alternative 1 - indexBase property

Add the property indexBase with possible values of either 0 or 1to the Property type. This would change the behavior of the index specifier to be either 1-based or 0-based.

Example:

 {
    "name": "FileLoggers",
    "type": "List<string>",
    "indexBase": 1,
    "format": "/flp{index}:{value}"
 }

Alternative 2 - Include base in specifier

The specifier could include the base, which may be a bit more concise. For example {index0} and {index1}, or {index(0)} and {index(1)} . {index+1} is another option to produce a one-based index if {index} is the zero based index, and the one that probably is the easiest to understand.

This is the option I would personally prefer.

3. MaxItems specifier on array Properties

In the case of MSBuild, at most 9 file loggers are supported. I'd like to introduce a maxItems property to the Property class, to allow setting a limit to the number of items that could be added to the list without having an error generated.

Example:

 {
    "name": "FileLoggers",
    "type": "List<string>",
    "maxItems": 9,    
    "format": "/flp{index}:{value}"
 }

4. ToString() generation in DataClasses

To allow for a dataclass to produce a single value for a property, as is the case of the file logger parameters in MSBuild, we could add the option to generate a ToString() method on a data class that takes the format of each item, just as if they were normal arguments, and joins them together with a separator. We should also allow for custom ToString() overloads in the partial class (which is the same as simply not generating a ToString() method I guess)

Suggest adding the toStringMethod property to a data class, allowing the specification of a separator.

Note: If any property today contains a format, an override of ConfigureArguments is generated which leads to a compile time error. We probably should not do this, but instead generate just the ToString in this case.

Example (containing a bunch of the previous suggestions as well).

"dataClasses": [
    {
        "name": "MSBuildFileLogger",
        "toStringMethod": {
            "separator": ";"
        },
        "properties": [
            {
                "name": "Summary",
                "type": "bool",
                "format": [
                    {
                        "when": false,
                        "then": "NoSummary"
                    },
                    {
                        "when": true,
                        "then": "ShowSummary"
                    }
                ]
            },
            {
                "name": "PerformanceSummary",
                "type": "bool",
                "format": "PerformanceSummary"
            },
            {
                "name": "LogFile",
                "type": "string",
                "format": "LogFile={value}"
            }
        ]
    }
]

5. Additional extension method generation

Add support for generating fluent type configuration methods on single properties as well (Methods accepting a configurator function that is). In the MSBuild example we have a property of type List<MSBuildFileLogger> and we want to have the following extension methods generated:

   public static MSBuildSettings AddFileLogger(this MSBuildSettings toolSettings, params Func<MSBuildFileLogger, MSBuildFileLogger>[] configurators)
   {
      var instance = new MSBuildFileLogger();
      return toolSettings.AddFileLogger(configurators.Select(configurator => configurator(instance)));
   }

   public static MSBuildSettings SetFileLogger(this MSBuildXSettings toolSettings, params Func<MSBuildFileLogger, MSBuildFileLogger>[] configurators)
   {
      var instance = new MSBuildFileLogger();
      return toolSettings.SetFileLogger(configurators.Select(configurator => configurator(instance)));
   }

Suggest adding support for the extensionMethods property also to individual properties to have these generated.

Example:

{
    "name": "FileLoggers",
    "type": "List<MSBuildFileLogger>",
    "extensionMethods": true,
    "format": ...
}

[david-driscoll]

Would also be possible to add in support for binary logging as well? Or I can do that as a different PR if that makes more sense.

[alphaleonis]

@david-driscoll That should be doable as well. We just need to come to an agreement on how to implement the support for this with the code generation first I think. Probably no point in implementing it before that.