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

GH-3238: Update DotNetTool module to target netstandard2.0 only #3239

Conversation

augustoproiete
Copy link
Member

Closes #3238

Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's fixed in Cake 1.1 maybe 1.0 users should just pin their version to
1.0.1 or upgrade to Cake 1.1.

Cake.DotNetTool.Module 1.1 targets Cake.Core 1.1.0, so not intended to be used with 1.0.

@augustoproiete
Copy link
Member Author

As it's fixed in Cake 1.1 maybe 1.0 users should just pin their version to
1.0.1 or upgrade to Cake 1.1.

That's certainly an option. Downside is for anyone using 1.0.0 or lower that is not yet using the DotNetTool module and decides to try out.

Also, users of @nils-a dependabot-cake-action will get PRs asking them to bump the DotNetTool module to 1.1

Cake.DotNetTool.Module 1.1 targets Cake.Core 1.1.0, so not intended to be used with 1.0.0

That's correct, except that to the end-user it might look confusing that the package that they have been using got a minor bump, and became incompatible (given the compatibility ranges we've been following)

@devlead
Copy link
Member

devlead commented Mar 6, 2021

Also, users of @nils-a dependabot-cake-action will get PRs asking them to bump the DotNetTool module to 1.1

But maybe in that case it should also look at what version of Cake.Core addin/module targets too, if it targets a newer version author has explicitly chosen higher than the contrib recommendation, i.e. for features that been added.

Cake.DotNetTool.Module 1.1 targets Cake.Core 1.1.0, so not intended to be used with 1.0.0

That's correct, except that to the end-user it might look confusing that the package that they have been using got a minor bump, and became incompatible (given the compatibility ranges we've been following)

Yes but that range in reverse, it means that something that targets Cake.Core 1.0 will work with any newer version of Cake until otherwise stated.
But it doesn't mean that something that explicitly targets a newer version of Cake.Core will work with an older version of Cake.Core.
Currently, that's why we recommend targeting Cake.Core 1.0 because that way your adding will work with any 1.x version.

We've added features in 1.1, those features aren't present in 1.0, i.e. there are no breaking changes in 1.1 so any addin that targets 1.0 and above will work with both 1.0 and 1.1.

Cake.DotNetTool.Module 1.1 targets Cake.Core 1.1 so it can't safely be recommended to be used with 1.0.

@gitfool
Copy link
Contributor

gitfool commented Mar 6, 2021

Does this mean Cake 1.1.1 is imminent, in which case I'll wait before upgrading everything to 1.1.0, or will the next release be much further out - at least a week or more?

@devlead
Copy link
Member

devlead commented Mar 6, 2021

Does this mean Cake 1.1.1 is imminent, in which case I'll wait before upgrading everything to 1.1.0, or will the next minor release be much further out - at least a week or more?

Well if we want Cake.DotNetTool.Module to support Cake.Core 1.0 then we need to change from project reference to package reference and pin Cake.Core version, not just change target framework.

<ProjectReference Include="..\Cake.Core\Cake.Core.csproj" />

@gitfool
Copy link
Contributor

gitfool commented Mar 6, 2021

When Cake.DotNetTool.Module was moved over it became embedded in the Cake.Tool NuGet package. Does this mean it doesn't need to be published post 1.0.0?

@devlead
Copy link
Member

devlead commented Mar 6, 2021

When Cake.DotNetTool.Module was moved over it became embedded in the Cake.Tool NuGet package. Does this mean it doesn't need to be published post 1.0.0?

It's needed for Cake.Frosting, or any other Cake implementation i.e. Cake.Bridge or Cake.Bridge.DependencyInjection

@gitfool
Copy link
Contributor

gitfool commented Mar 6, 2021

Couldn't that be a project reference that ends up embedded in the Cake.Frosting NuGet too?

@augustoproiete
Copy link
Member Author

augustoproiete commented Mar 6, 2021

But maybe in that case it should also look at what version of Cake.Core addin/module targets too, if it targets a newer version author has explicitly chosen higher than the contrib recommendation, i.e. for features that been added.

🤔 I think knowing what version of Cake.Core addin/module targets is valuable info, but it doesn't paint the full picture that would allow dependabot-cake-action to make a sensible decision. The dependabot-cake-action doesn't know which Cake runner is being used in a repo today... Which is the other piece of information it would need to decide whether an addin/package can be upgraded in the Cake script.

@gitfool
Copy link
Contributor

gitfool commented Mar 6, 2021

@augustoproiete another way to handle that would be for Cake to support version ranges. Normally you always want latest (IMHO), but if that becomes problematic you should be able to pin the version range to avoid issues with an incompatible Cake.Core.

@augustoproiete
Copy link
Member Author

Yes but that range in reverse, it means that something that targets Cake.Core 1.0 will work with any newer version of Cake until otherwise stated.

We're on the same page on this. My point is only that the version number of Cake.DotNetTool.Module getting a minor bump (due to the fact it's now driven by the Cake version) instead of a major bump might cause confusion.

@augustoproiete
Copy link
Member Author

@gitfool Agreed, that would be useful. We have a PR that seems to add support version ranges, which needs some TLC to move forward.

@devlead
Copy link
Member

devlead commented Mar 6, 2021

We're on the same page on this. My point is only that the version number of Cake.DotNetTool.Module getting a minor bump (due to the fact it's now driven by the Cake version) instead of a major bump might cause confusion.

It might be confusing yes, one reasoning there is that it from now on should only be outside of Cake, Cake.Tool and Cake.CoreCLR.

Any usage outside of Cake, Cake.Tool and Cake.CoreCLR will most likely use csproj or packages.config, and in those scenarios nuget will automatically sort the upgrade of Cake.Core to 1.1 as that's stated as a requirement in the nuspec.

That's why if we want to support Cake.Core 1.0 then Cake.DotNetTool.Module needs to target Cake.Core 1.0 too, we can't just change TargetFramework as that won't solve the 1.0 compatibility issue.

BTW also real fix in Cake would be for modules to respect TFMs just as we do with addins, so modules can multi target without issue.

@augustoproiete
Copy link
Member Author

BTW also real fix in Cake would be for modules to respect TFMs just as we do with addins, so modules can multi target without issue.

Agreed. We're treating symptoms instead of the cause.

So perhaps the best option to move forward is to just keep it as-is, and document on the website so we can point people to it if we get support requests? /cc @cake-build/cake-team

@augustoproiete
Copy link
Member Author

We decided we'll leave it as is and document our general recommendation:

  • If you use Cake.DotNetTool.Module already on your builds, you should either:

  • (1) Upgrade Cake to version 1.1.0, and remove the Cake.DotNetTool.Module from your build script (as it's no longer needed)

or

  • (2) Pin Cake.DotNetTool.Module to a version compatible with the Cake version that you use. Cake.DotNetTool.Module version 1.0.1 is the last version compatible with Cake 1.0.0. Starting with this release, Cake.DotNetTool.Module will always be released together with Cake and will only be compatible with the current release.

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

Successfully merging this pull request may close these issues.

Regression on DotNetTool Module: Targets multiple TFMs causing warnings
3 participants