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

Recommended changes resulting from automated audit #21

Closed
2 tasks done
Jericho opened this issue Jun 5, 2018 · 9 comments
Closed
2 tasks done

Recommended changes resulting from automated audit #21

Jericho opened this issue Jun 5, 2018 · 9 comments

Comments

@Jericho
Copy link
Member

Jericho commented Jun 5, 2018

We performed an automated audit of your Cake addin and found that it does not follow all the best practices.

We encourage you to make the following modifications:

  • You are currently referencing Cake.Core 0.26.0. Please upgrade to 0.28.0
  • Your addin should target netstandard2.0. Please note that there is no need to multi-target, netstandard2.0 is sufficient.

Apologies if this is already being worked on, or if there are existing open issues, this issue was created based on what is currently published for this package on NuGet.org and in the project on github.

pascalberger added a commit that referenced this issue Jun 24, 2018
@pascalberger
Copy link
Member

With recent discussions multi-targetting is again way to go. So this addin now multi-targets full framework 4.6.1 and .NET standard 2.0

@Jericho
Copy link
Member Author

Jericho commented Oct 2, 2018

I was not aware the recommendation had changed. What’s the reasoning for this change?

Was this discussed on Gitter? I must have missed this discussion.

@pascalberger
Copy link
Member

@Jericho I cannot find something "official" from Microsoft, but there were recent discussion about this on Twitter and they also mentioned it on a .NET Conf talk. The issue is that the support for .NET Standard in 4.6.1 was added retroactively and unfortunately can lead to issues. Also 4.7.1 has issues. Which basically means that you're required to have 4.7.2 installed to get .NET Standard assemblies running. As Cake itself can run on 4.6.1 best is therefore to multi-target 4.6.1 and .NET Standard 2.0.

I just created a PR to update the guidelines: cake-contrib/Home#55

@Jericho
Copy link
Member Author

Jericho commented Oct 2, 2018

Have you had a discussion about this with the Cake team? I know that @mholo65 has done a lot of work on this, does he agree with your conclusion?

@Jericho
Copy link
Member Author

Jericho commented Oct 2, 2018

To be clear, I’m not questioning your reasoning. I just want to make sure everybody is on same page and agrees on this change before I modify the Audit tool to enforce this new requirement.

(By the way thanks for the PR on the recommendation document)

@pascalberger
Copy link
Member

Yes, we discussed this inside the Cake team, also if we want to target .NET 4.7.1 with Cake, but we decided to stay on 4.6.1 for Cake and suggest to multi-target for addins. The thing is that this is something which we cannot fix inside Cake, as it is an issue with .NET Framework, and also Microsoft cannot fix easily without introducing a whole bunch of other issues.

I also just created a PR for adding a note about this on the Cake website: cake-build/website#579, and we would also like to do a blog post.

@pascalberger
Copy link
Member

Unfortunately there's no official documentation about this form Microsoft to easily link to.

But they mention it on this .NET Conf presentation starting at 44:48: https://channel9.msdn.com/Events/dotnetConf/2018/S107
And talk about the issue in this tweet: https://twitter.com/terrajobst/status/1031999730320986112

@Jericho
Copy link
Member Author

Jericho commented Oct 2, 2018

I just read the twitter thread you linked and I now understand the situation: Cake currently is built with .net 4.6.1 which is not 100% compatible with netstandard2.0 (despite some Microsoft documentation). Therefore addins shouldn’t target netstandard2.0 exclusively.

I guess it’s time to modify the audit tool and notify the 200+ addins authors!!!

@Jericho
Copy link
Member Author

Jericho commented Oct 2, 2018

Just thinking out loud: for the last several months we have been telling addin authors they only need to target netstandard2.0, now we are telling them they need to multi-target and when Cake upgrades to net 4.7.x, we will again tell them to only target netstandard2.0

I'm concerned addin authors will get fatigued that we keep changing the recommendation and will stop listening to our recommendations.

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

No branches or pull requests

2 participants