cake-build / cake

:cake: Cake (C# Make) is a cross platform build automation system.
https://cakebuild.net
MIT License
3.85k stars 724 forks source link

Upgrading to spectre.console 0.47.0 breaks the cake build #4157

Closed FrankRay78 closed 8 months ago

FrankRay78 commented 1 year ago

Prerequisites

Cake runner

Cake .NET Tool

Cake version

Most recent develop branch (b55239f748f921edc303d644839183e9727e00a4)

Operating system

Linux, Windows, macOS

Operating system architecture

64-Bit

CI Server

GitHub action on the main cake-build/cake repo

What are you seeing?

Something in spectre.console 0.47.0 breaks the Cake build, I've verified it conclusively on the following PR: https://github.com/cake-build/cake/pull/4156

I took the latest upstream develop branch and pushed it - the build passes. I then upgrade the spectre.console NuGet packages to 0.47.0 and re-push - the build fails with timing issues

The failure is originating in tests\integration\Cake\ScriptCache.cake, namely:

139            Assert.True(data.CompileResult.Elapsed > data.ExecuteResult.Elapsed, $"Compile time {data.CompileResult.Elapsed} should be greater than execute time  {data.ExecuteResult.Elapsed}.");
140            Assert.Equal(data.CompileResult.Hash, data.ExecuteResult.Hash);

And also

152        Assert.True(data.ReCompileResult.Elapsed> data.ExecuteResult.Elapsed, $"ReCompileTime time {data.ReCompileResult.Elapsed} should be greater than execute time  {data.ExecuteResult.Elapsed}.");
153        Assert.NotEqual(data.CompileResult.Hash , data.ReCompileResult.Hash);

Even when I commented out the first Assert statement, the NotEqual Hash assert still fails (see commit: https://github.com/cake-build/cake/pull/4156/commits/bbb2324259f17b18d0a89d44db8f51f6594fe5a0)

What is expected?

The build to pass

Steps to Reproduce

  1. Create a branch from the most recent cake develop branch
  2. Open the solution and upgrade both spectre.console NuGet packages to 0.47.0
  3. Build and run tests locally (should all pass)
  4. Check-in and push, create PR
  5. Build then fails

Output log

https://github.com/cake-build/cake/actions/runs/5055159519 https://github.com/cake-build/cake/actions/runs/5055159519/jobs/9070991328#step:5:1035

image


cc: @patriksvensson (nb. I've spent a bit of time pushing different changes to the test PR, https://github.com/cake-build/cake/pull/4156, and still don't understand what's causing this.)

Next steps

One possibility is to locally build a NuGet package, for each spectre.console commit since 0.46.0, pushing them one by one to the test PR https://github.com/cake-build/cake/pull/4156, in order to try and identify exactly what change might be causing the cake build failure.

devlead commented 1 year ago

@FrankRay78 issue seems to be that flags are no longer available as arguments for some reason after upgrading to 0.47.0 i.e. passing --invalidate-script-cache argument isn't available here anymore https://github.com/cake-build/cake/blob/6a334f30ec989c44d6578a7e73843ed9f1167ca3/src/Cake/Infrastructure/Scripting/RoslynScriptSession.cs#L64C106-L64C106

            _regenerateCache = host.Context.Arguments.HasArgument(Constants.Cache.InvalidateScriptCache);

with 0.47.0 image

If one reverts to 0.46.0 image

So something has changed in Spectre.Console in this regard.

devlead commented 1 year ago

Checking the Spectre.Console command earlier

https://github.com/cake-build/cake/blob/6a334f30ec989c44d6578a7e73843ed9f1167ca3/src/Cake/Commands/DefaultCommand.cs#L73C55-L73C57

                return _builder.Run(context.Remaining, new BuildFeatureSettings(host)

one can see by inspecting command / context

new { settings.Recompile, context.Remaining.Parsed.Count }

in 0.47.0 on gets recompile but it's not present in context.Remaining

{{ Recompile = True, Count = 1 }}
    Count: 1
    Recompile: true

but in 0.46.0 it's both parsed and available in Remaining

{{ Recompile = True, Count = 2 }}
    Count: 2
    Recompile: true
devlead commented 1 year ago

Suspect it's this change in behavior https://github.com/spectreconsole/spectre.console/commit/714cf179cb349597a8b775287ac8299584b18617?diff=unified#diff-4a7ee1042c6d7b131733b2395f43b1a7e80562c80151c5aa49bcedc9095a0bc8L361-R380

image

That's breaking Cake.

devlead commented 1 year ago

Wondering for Cake's sake if there were something like context.All.Parsed that contained all arguments passed to the build feature.

FrankRay78 commented 1 year ago

Thank you so much for this @devlead, a thoroughly useful investigation that was beyond my means. I'll pick this up from here and see if I can get it fixed in spectre.console

devlead commented 9 months ago

@FrankRay78 any luck getting this addressed upstream?

FrankRay78 commented 9 months ago

Thanks for the ping @devlead, life got in the way but I will move this to the top of my stack.

FrankRay78 commented 9 months ago

Update: Looking into this now @devlead, I'm keen to get this sorted out.

devlead commented 9 months ago

Update: Looking into this now @devlead, I'm keen to get this sorted out.

Excellent đź‘Ť

FrankRay78 commented 9 months ago

Ok, I can see what's going on. Basically 0.47.0 has tightened up the parsing. If an argument matches something in the settings class, then it's populated there and no longer appears in the Remaining.Parsed args collection. Whereas in 0.46.0 and earlier, it would be populated in the settings class but also appear in the args collection. This visual demonstrates the difference in behaviour, although is nothing new to the investigations already done above.

image

I'm checking with the other spectre.console maintainers whether 0.47.0 is the intended behaviour going forward, and once I hear back, I will be able to determine the correct course of action.

FrankRay78 commented 9 months ago

It's looking like the 0.47.0 behaviour is the correct one, so I will start to examine the Cake codebase for the required changes to support this updated behaviour (/breaking change).

nb. Looks very similar to this Cake.Frosting issue, which was previously fixed: https://github.com/cake-build/cake/issues/3291

// Fixes #3291, We have to add arguments manually which are defined within the DefaultCommandSettings type. Those are not considered "as remaining" because they could be parsed

https://github.com/cake-build/cake/blob/c3cd2af8d73b4f270d6abe585c41f049ea92a0d7/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs#L95

FrankRay78 commented 8 months ago

I'm closing this issue as https://github.com/cake-build/cake/pull/4236 was successfully merged, fully addressing it.

cake-build-bot commented 8 months ago

:tada: This issue has been resolved in version v3.2.0 :tada:

The release is available on:

Your GitReleaseManager bot :package::rocket: