Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
FrankRay78 opened this issue May 23, 2023 · 13 comments
Closed
2 tasks done

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

FrankRay78 opened this issue May 23, 2023 · 13 comments
Milestone

Comments

@FrankRay78
Copy link
Contributor

FrankRay78 commented May 23, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched issues to ensure it has not already been reported

Cake runner

Cake .NET Tool

Cake version

Most recent develop branch (b55239f)

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: #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: bbb2324)

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, #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 #4156, in order to try and identify exactly what change might be causing the cake build failure.

@devlead
Copy link
Member

devlead commented Jul 8, 2023

@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
Copy link
Member

devlead commented Jul 8, 2023

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
Copy link
Member

devlead commented Jul 8, 2023

Suspect it's this change in behavior
spectreconsole/spectre.console@714cf17?diff=unified#diff-4a7ee1042c6d7b131733b2395f43b1a7e80562c80151c5aa49bcedc9095a0bc8L361-R380

image

That's breaking Cake.

@devlead
Copy link
Member

devlead commented Jul 9, 2023

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

@FrankRay78
Copy link
Contributor Author

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
Copy link
Member

devlead commented Sep 29, 2023

@FrankRay78 any luck getting this addressed upstream?

@FrankRay78
Copy link
Contributor Author

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

@FrankRay78
Copy link
Contributor Author

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

@devlead
Copy link
Member

devlead commented Oct 18, 2023

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

Excellent 👍

@FrankRay78
Copy link
Contributor Author

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
Copy link
Contributor Author

FrankRay78 commented Oct 20, 2023

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: #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

// 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

@FrankRay78
Copy link
Contributor Author

I'm closing this issue as #4236 was successfully merged, fully addressing it.

@cake-build-bot
Copy link

🎉 This issue has been resolved in version v3.2.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants