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

Change the calculation of the TFM for Cake addins #116

Closed
nils-a opened this issue Mar 13, 2021 · 14 comments · Fixed by #125
Closed

Change the calculation of the TFM for Cake addins #116

nils-a opened this issue Mar 13, 2021 · 14 comments · Fixed by #125
Assignees
Labels
bug Something isn't working
Milestone

Comments

@nils-a
Copy link
Contributor

nils-a commented Mar 13, 2021

In versions up until Cake 1.0.0 we has only one rule for TargetFramework.

In current development, there is three of them:

  • addins referencing Cake < 1.0.0: netstandard2.0 (required) and net461 (suggested)
  • addins referencing Cake ≥ 1.0.0: netstandard2.0 (required) and net461 (suggested) and net5.0 (suggested)
  • modules: netstandard2.0 (required)
  • recipes: no required or suggested references

Currently for addins not referencing Cake (Core or Common) the same rule as for Cake < 1.0.0 is selected.

@Jericho asked in #107

Why are the targeting rules different depending on whether an addin targets Cake.Core or not? Shouldn't we be consistent and always make the same recommendation?

And my reply was

Currently the rule defaults to the "lowest" requirement - in this case that is the "Cake < 1.0.0"-rule. We could argue, that since Cake 1.0.0 is now the default, the default-rule should be the "Cake ≥ 1.0.0"-rule.

The question is: Does someone have other ideas, or opinions on this? I really don't have a strong opinion on this, the way it is currently implemented was only the first thing that came to my mind when implementing this.

@augustoproiete
Copy link
Member

Do we have examples of Cake addins that don't reference Cake.Core nor Cake.Common but are still considered true Cake addins? 🤔

In fact, what is the definition of a true Cake addin?

To me, it's a class library that has tailor-made aliases specifically to be used in Cake builds, by adding extension methods to ICakeContext. So if this definition is correct, then it wouldn't be possible for a true Cake addin not to reference Cake.Core (directly or transitively).

Similarly, true Cake modules need to implement the ICakeModule interface and have the CakeModule attribute in the assembly... So like addins, it wouldn't be possible for a true Cake module not to reference Cake.Core (directly or transitively).

The fact Cake can leverage/load any assembly via the #addin directive doesn't make them true Cake addins, so I wouldn't expect, say, Polly to ever install the CakeContrib.Guidelines packages.

Finally my understanding is that the TFM rules exist to help ensure that an addin/module will be compatible with Cake runners. If a Cake-Contrib project is not "loaded" by the Cake runners (and therefore doesn't need Cake.Core nor Cake.Common), then I think CakeContrib.Guidelines shouldn't care about what TFMs are being targeted, as it doesn't affect Cake at all... It's at the discretion of the project maintainer.

@nils-a
Copy link
Contributor Author

nils-a commented Mar 13, 2021

The point came up when adding CakeContrib.Guidelines to Cake.Email and Cake.Email.Common.

And while I do agree to the "not caring about not-addins", I think there is probably a reason to reference CakeContrib.Guidlines and that it is probably because a use in a Cake context is intended.

So, looking at it that way, for a "common" project that should be used in other cake related projects - shouldn't that "common" project target the same targetFramworks (or a superset) as the intended using packages?

@devlead
Copy link
Member

devlead commented Mar 13, 2021

If it has no direct reference Core, Common nor runners then it's more a case of Cake supporting it than the other way around. Plus what @augustoproiete said.

@Jericho
Copy link
Member

Jericho commented Mar 13, 2021

We are trying to encourage addin authors to upgrade their addins to target Cake 1.0.0, therefore in my mind it makes sense that the "default" target rule would be "Cake > 1.0.0".

@nils-a
Copy link
Contributor Author

nils-a commented Mar 13, 2021

I kind of agree to both "positions" currently.
I feel the way I implemented the default-rule currently is wrong. I propose the following change:

  • projects named *.cake (addins): Cake based rules, as above with the default being Cake ≥ 1.0.0
  • projects named *.module (modules): Rule as above.
  • projects named *.recipe (recipes): Rule as above.
  • projects not named *.cake, *.module, *.recipe : Do not require any specific TargetFramework

Additionally, to address what @devlead said I propose to extend #98 to raise a warning if the project is named either *.cake (addin) or *.module (module) but does not reference Cake.

@augustoproiete
Copy link
Member

So, looking at it that way, for a "common" project that should be used in other cake related projects - shouldn't that "common" project target the same targetFramworks (or a superset) as the intended using packages?

A human looking at Cake.Email.Common would say "YES", but a machine would say "I DON'T KNOW" as it doesn't have enough information to decide which TFM(s) should be targeted because it's a dependency package... - The machine can't know where it's going to be used...

The entry-point is the Cake.Email addin, so it owns the responsibility to choose the right dependencies that it can use/take to satisfy its implementation.

Now if you wanted to - as part of analyzing a true Cake addin such as Cake.Email - you could check its referenced assemblies and detect if any of the referenced assemblies don't match the TFMs being used by the addin - That would be the right place to issue warnings... Not in the dependency package. Of course this would be complex as you'd have to walk the whole dependency tree.

@augustoproiete
Copy link
Member

  • projects named *.cake (addins): Cake based rules, as above with the default being Cake ≥ 1.0.0
  • projects named *.module (modules): Rule as above.
  • projects named *.recipe (recipes): Rule as above.
  • projects not named *.cake, *.module, *.recipe : Do not require any specific TargetFramework

Hmm I don't think rules should be based on the project name. I think we have better ways to determine if the project is an addin/module/recipe, to then recommend the right TFM and Cake.Core/Cake.Common version to depend on if it's a Cake extension.

@nils-a
Copy link
Contributor Author

nils-a commented Mar 13, 2021

I think we have better ways to determine if the project is an addin/module/recipe

Any ideas that work from msbuild are highly welcome 😄

@augustoproiete
Copy link
Member

augustoproiete commented Mar 13, 2021

Any ideas that work from msbuild are highly welcome 😄

Would it work if it was an MSBuild task that runs after the build, and analyzes the generated assemblies via Mono.Cecil (or similar)?

@nils-a
Copy link
Contributor Author

nils-a commented Mar 13, 2021

Would it work if it was an MSBuild task that runs after the build, and analyzes the generated assemblies via Mono.Cecil (or similar)?

Probably. I have created #120 to track that.

@augustoproiete
Copy link
Member

Hmm I don't think rules should be based on the project name.

I thought more about this and realized that my concern about with using project names to detect what kind of Cake extension it is was mainly because it's hard to differentiate between Cake addins and other Cake supporting projects just based on project names - which is not the case for Modules and Recipes where using the project name would prob. be fine.

If we check for Cake.Core and/or Cake.Common first before applying any rules, I think using project names might be fine.

  • Project references Cake.Core and/or Cake.Common

    • and project named Cake.**.Module (assume it's a module): Apply TFM rule for modules
    • and project is named Cake.**.Recipe (assume it's a recipe): Apply TFM rule for recipes
    • and project name starts with Cake. (assume it's an addin): Apply TFM rule for addins
    • none of the above: Do not apply TFM rule
  • Project don't reference Cake.Core or Cake.Common

    • Do not apply TFM rule

Would that be easier to implement with the current codebase, without going the path I suggested with Mono.Cecil, etc?

@gep13
Copy link
Member

gep13 commented Mar 14, 2021

This seems like a sensible approach to me! We can always enhance the detection rules later, but this would be a solid first pass.

@nils-a
Copy link
Contributor Author

nils-a commented Mar 14, 2021

Would that be easier to implement with the current codebase,

Yes, that's only a real small change compared to the current codebase. I'll proceed with that change and leave #120 open to be revisited later.

@nils-a nils-a changed the title What should the default targetFrameworks be for addins not referencing Cake? Change the calculation of the TFM for Cake addins Mar 14, 2021
@nils-a nils-a added the bug Something isn't working label Mar 14, 2021
@nils-a nils-a added this to the 1.0.0 milestone Mar 14, 2021
@nils-a nils-a self-assigned this Mar 14, 2021
nils-a added a commit to nils-a/CakeContrib.Guidelines that referenced this issue Mar 15, 2021
This is mainly a restructured calculation of the
project type (addin/module/recipe) and a small
change to the default TFM.
nils-a added a commit to nils-a/CakeContrib.Guidelines that referenced this issue Mar 15, 2021
This is mainly a restructured calculation of the
project type (addin/module/recipe) and a small
change to the default TFM.
nils-a added a commit to nils-a/CakeContrib.Guidelines that referenced this issue Mar 16, 2021
nils-a added a commit that referenced this issue Mar 16, 2021
(GH-116) updated the calculation for TFM
cake-contrib-bot pushed a commit that referenced this issue Mar 16, 2021
Merge pull request #125 from nils-a/feature/GH-116

(GH-116) updated the calculation for TFM
@cake-contrib-bot
Copy link

🎉 This issue has been resolved in version 1.0.0 🎉
The release is available on:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants