Boolean arguments formatted as 'System.Object' in output

[andygjp]

Hi,

Firstly, great tool!

I get this error when I run a msbuild target on my local dev machine:

> "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\amd64\msbuild.exe" C:\Repos\build\ManagementSystem.sln /maxcpucount:4 /nodeReuse:System.Object /p:Configuration=Debug /target:ManagementSystem_Api;ManagementSystem_Api_IntegrationTests;ManagementSystem_Api_UnitTests;ManagementSystem_Application;ManagementSystem_Data;ManagementSystem_Database;ManagementSystem_Domain;ManagementSystem_Domain_Models;ManagementSystem_Integration;ManagementSystem_Processes;ManagementSystem_Test_Common;ManagementSystem_Test_Db;StateMachines
Microsoft (R) Build Engine version 15.5.180.51428 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

MSBUILD : error MSB1042: Node reuse value is not valid. String was not recognized as a valid Boolean..
Switch: System.Object

The example error, above, is from my compile target:

Target Compile => def => def
        .DependsOn(Restore)
        .Executes(() => MSBuild(_ => DefaultMSBuildCompile));

I think the issue is down to the thing that generates the command. NodeReuse is a nullable boolean that is null by default, but is set to true if I use the default msbuild settings, via NukeBuild.Instance.IsLocalBuild. If I explicitly set it false:

Target Compile => def => def
        .DependsOn(Restore)
        .Executes(() => MSBuild(_ => DefaultMSBuildCompile.SetNodeReuse(false)));

The error goes away. The output is:

> "C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin\amd64\msbuild.exe" C:\Repos\build\ManagementSystem.sln /maxcpucount:4 /p:Configuration=Debug /target:ManagementSystem_Api;ManagementSystem_Api_IntegrationTests;ManagementSystem_Api_UnitTests;ManagementSystem_Application;ManagementSystem_Data;ManagementSystem_Database;ManagementSystem_Domain;ManagementSystem_Domain_Models;ManagementSystem_Integration;ManagementSystem_Processes;ManagementSystem_Test_Common;ManagementSystem_Test_Db;StateMachines
Microsoft (R) Build Engine version 15.5.180.51428 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

I think Arguments.cs should change from:

        public Arguments Add(string argument, bool? condition = true)
        {
            return Add(argument, condition.HasValue && condition.Value ? new object() : null);
        }

to:

        public Arguments Add(string argument, bool? condition = true)
        {
            return Add(argument, condition.HasValue && condition.Value ? bool.TrueString : null);
        }

However, I still don't think its right.

nodeReuse is on by default, but my potential fix doesn't allow me to turn it off (see the last output - /nodeReuse:false is missing). I think it should actually be:

        public Arguments Add(string argument, bool? condition = true)
        {
            return Add(argument, condition.HasValue ? (condition.Value ? bool.TrueString : bool.FalseString) : null);
        }

That could, potentially, be a change to functionality.

What do you think - is that a change we could make? I could raise a pull request if you like?

Cheers,
Andy

[matkoch]

@andygjp thanks for the kind words and for raising this issue 😄 Handling boolean arguments was indeed a little incomplete until now. This commit should fix it. Could you review it?

Btw, for resetting NodeReuse you can call ResetNodeReuse. Then neither true or false are passed.

[andygjp]

Thanks for the quick turn response.

The change looks good. I've not tried it out, however. Would you like me to try it out before you push a new version to NuGet?

And thanks for the tip 😀

Cheers,
Andy

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.