-
-
Notifications
You must be signed in to change notification settings - Fork 726
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
GH2886/2904: Bring Frosting up to speed #2946
GH2886/2904: Bring Frosting up to speed #2946
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frosting Integration test needs to be updated to cater for the breaking changes.
Think this is already fixed by param Lines 120 to 123 in 11dc243
and build.csproj is poked cake/src/Cake.Frosting.Template/Cake.Frosting.Template.csproj Lines 26 to 28 in 11dc243
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After adjusted the following in integration tests
using Cake.Core;
using Cake.Frosting;
+using Microsoft.Extensions.DependencyInjection;
public class Program : IFrostingStartup
{
public static int Main(string[] args)
- {
- // Create the host.
- var host = new CakeHostBuilder()
- .WithArguments(args)
+ => new CakeHost()
.UseStartup<Program>()
- .Build();
-
- // Run the host.
- return host.Run();
- }
+ .Run(args);
- public void Configure(ICakeServices services)
+ public void Configure(IServiceCollection services)
{
services.UseContext<Context>();
services.UseLifetime<Lifetime>();
services.UseWorkingDirectory("..");
}
}
CI seems to be happier.
@patriksvensson as I commented earlier by the looks of it template versioning seems to work already?
Was there anything else you wanted to add before taking out of WIP?
Other than that this LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment for a smaller issue. Beside that we should create issues for the breaking changes introduced with this PR so that they end up in release notes.
<Compile Remove="**\*" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<None Remove="build.ps1" /> | ||
<None Remove="build.sh" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these two lines required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
@@ -0,0 +1,2 @@ | |||
dotnet run --project build/Build.csproj -- $args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer want to download .NET as part of the bootstrapper? What's the reason for this?
Otherwise #2960 would be a solution using global.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the standard bootstrappers install .NET?
Maybe a good thing to keep these as simple as possible.
The template itself already requires .NET Core / .NET 5 to be installed.
I kinda like the simplicity of what's suggested here.
Maybe better to document dotnet new globaljson
and have more advanced bootstrappers in our resources repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was never team "Install .NET in bootstrapper" 😉 It's just what most people seem to prefer nowadays, and I wanted to bring it up to discussion, since it's quite a hidden change. Template requiring .NET to be installed probably isn't an argument since people / machine running build in most cases is not the same which did run the template.
@gep13 Did you already though about how bootstrappers in resources repo should look like for .NET tool and .NET Core regarding installation of .NET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pascalberger I haven't come to a final decision about how these should look yet. I am still thinking that installing .Net locally into the repository folder, based on what is in the global.json is a good idea though. I liked what was done with the jsonfile
parameter, and I thought about putting an if statement round that to either pass it in or not, based on the global.json file being present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the frosting bootstrapper should install .NET. It should just be a easy way to run a build which exists in another directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it maybe should push / change / pop directory so it's executed in script directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not needed since the working directory already is the script directory. Working directory should imo not be set by a script but be explicitly stated by the person calling the script
Contains some unavoidable breaking changes.
src/Cake.Cli/Cake.Cli.csproj
Outdated
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<AssemblyName>Cake.Cli</AssemblyName> | ||
<TargetFrameworks>netstandard2.0;net5.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is also used in Cake.Core it should also target net46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used in Cake.Core
, it references Cake.Core
though.
For net46
won't work because of dependencies, it could and probably should target net461
too as Cake.exe
does.
I can add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly introduced Cake.Cli library needs to be added to
Line 107 in a2f339b
new [] { |
Good catch I'll add that. |
@pascalberger Added Cake.Cli NuGet & net461 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't go through every single change, but overall it LGTM.
I still want to have proper documentation out for breaking changes in release notes and on website before we push the release out.
@patriksvensson your changes have been merged, thanks for your contribution 👍 |
Contains some unavoidable breaking changes.