Reasonable Defaults For Centrally Controlled PackageReference Version

[leusbj]

Here's a potential Approach for #46 and #49

Utilizing this SDK will now provide "reasonable" default version numbers for certain PackageReferences

• Directly setting the Version metadata when no Central management is detected (behavior existed already)
• Creating PackageReference Update statements when CentralPackageVersionSdk is enabled (and only when Packages.targets does not provide version value)
• Creating PackageVersion Include statements when CentralPackageManagement is enabled (and only when Directory.Packages.targets does not provide version value)

This Sdk will provide an inforrmational "Warning" level message during build recommending the project owner to add an entry into their package management system of choice... for those packages that do not have a centralized version entry, and the Sdk default value was applied.

@brandon-hurler and @CZEMacLeod See if this aligns with what you were looking for

[CZEMacLeod]

This looks very good and well thought out. There are some very clever bits of MSBuild stuff there with metadata and item group transforms but I like it. How much testing have you done to check these work as expected?

The only thing I think I would like to tweak is the line that controls the application of the PackageReference updates to include the version numbers - Condition=" '$(UsingMicrosoftCentralPackageVersionsSdk)' != 'true' AND '$(ManagePackageVersionsCentrally)' != 'true' "
Could we introduce a property, something like ApplySystemWebSdkPackageVersions which will be set to true if it isn't set, and the existing conditions aren't set. This means you could manually disable that section more easily.

I really like the target with the warning SystemWebSdkProvidedPackageVersionDefaultWarning - I think that is very much the issue I had with applying the defaults when using CPM. Can you explain where EnableMimicControlPackageVersion comes from? Is this just the property used to disable the default versions? Is that the best name? Should it be EnableMimicCentralPackageVersions? I don't want to nit-pick, especially as this is great work, but one thing I think is missing in this project is the documentation and making the properties have obvious names to what they do is the first step.

[leusbj]

The testing I focused on was to create 2 projects (one CPM and one CPV) and walked it through the following phases

1. Initially having "zero" entries in the respective "central" files (Directory.Packages.props and Packages.targets) -> results in the SDK default versions being injected, and 4 warnings total (1 warning for each of the 2 Packages for each of the 2 projects)
2. Adding an entry for the toolset PacakgeReference -> results in the SDK default versions being injected, and 2 warnings total (1 warning for the CompilerPlatform Packages for each of the 2 projects)
3. Adding the entry for the CompilePlatform -> results in the Sdk default versions being ignored, and zero warnings generated
   If you'd like to wait to take this until there are unit tests in place, I can probably make that happen too, as I was starting to play with that again.

For your question about tweaking an area and potentially adding a new property to govern disabling that section, were you referring to this section below? This is the part that sets the "Version" metadata when there is NO central management of package versions, so I'm thinking that we wouldn't ever turn this particular section off. But Maybe I misread your question.

    <PackageReference Update="@(PackageReference->WithMetadataValue('SystemWebSdkIncludedPackage','true')->HasMetadata('SystemWebSdkIncludedPackageVersion'))"
                      Condition=" '$(UsingMicrosoftCentralPackageVersionsSdk)' != 'true' AND '$(ManagePackageVersionsCentrally)' != 'true' ">
      <Version>%(SystemWebSdkIncludedPackageVersion)</Version>
    </PackageReference>

I would love better ideas on the EnableMimicControlPackageVersion name, as I wasn't sure exactly what to call it either.
This is the project owner configurable property that governs whether or not the Sdk will attempt to "mimic" an entry in the detected centralized package version mechanism.

• It doesn't "persist" anything to a file in the obj\ directory or do any writes to the Directory.Packages.props or Packages.props
• It doesn't duplicate or overwrite anything previously set by the project owner
• It does it for both CentralPackageVersion and CentralPackageManagement, so it seemed odd to call it specifically one of those
• It just does it's best (when not explicitly set to false) to ensure that a new project can be created with the Sdk under a directory that has enabled centralized control. So in my head it was only "mimicking" the things that the warning was reminding the project owner they should be doing

I whole heartedly agree though, even if most people will never need to set it to false, we should strive to come up with meaningful names.

[leusbj]

@CZEMacLeod I added Unit Tests for the 3 simplest of cases. If this seems worthwhile I can add coverage for the other cases as well. And if you have a unit test "Fluent Assertion" flavor preference (should be able to swap in/out whichever one)

I added a description about the Property, and the behaviors for version metadata to the documentation Sdk.md. I noticed that the RazorLibrary.md does not currently have the same table describing properties, Should I add a similar one there as well?

[CZEMacLeod]

@leusbj I've pushed a small update V4.0.88 which addresses some of the outstanding issues.
I would really like to include your cleverness from this PR around applying packages and versions - perhaps you could rebase it off the latest. Hopefully some of the changes there will simplify how this goes.

I've added some info to the documentation for the properties used.

Property	Default value	Description
ExcludeSDKDefaultPackages	false	Do not include the default packages Microsoft.Net.Compilers.Toolset and Microsoft.CodeDom.Providers.DotNetCompilerPlatform
ApplySDKDefaultPackageVersions	true*	Apply default version numbers to packages unless using a Central Package Management system

*Version numbers are not applied if you are using Microsoft.Build.CentralPackageVersions version 2.1.1 or higher. Remember to include the packages in your central package versions file.

You will see that ApplySDKDefaultPackageVersions is now set to false if either of the CPM solutions are in use.
I think these property names make sense.

I'm not sure about the unit testing stuff - I think it would need to be added to the build scripts, and the main issue I wanted to ensure was not so much that it worked in all the situations we wanted to cover, but that any other packagereferences in the main project file would not get their version numbers messed up (something I have done in the past when trying to update an item from another item).
Specifically, with

    <PackageReference Update="@(PackageReference->WithMetadataValue('SystemWebSdkIncludedPackage','true')->HasMetadata('SystemWebSdkIncludedPackageVersion'))"
                      Condition=" '$(UsingMicrosoftCentralPackageVersionsSdk)' != 'true' AND '$(ManagePackageVersionsCentrally)' != 'true' ">
      <Version>%(SystemWebSdkIncludedPackageVersion)</Version>
    </PackageReference>

to make sure that it does not overwrite all packagereferences or updates them all to the value of %(SystemWebSdkIncludedPackageVersion) of last one matched.

As I said, assuming this logic behaves correctly I really like everything done here.

[CZEMacLeod]

Can use '$(ApplySDKDefaultPackageVersions)'=='true' as the condition as of V4.0.88
This should probably be in the common files target now.

[CZEMacLeod]

This file should probably be a common file now, and use the ExcludeSDKDefaultPackages and ApplySDKDefaultPackageVersions properties instead of ExcludeDefaultRazorPackages and EnablePackageVersionDefaultsFromSdk

[leusbj]

I'll try to take a look at it in the next several days.

[brandon-hurler]

It looks good to me as well. I agree that there was some clever MSBuild work in here. I kept circling back in my free time trying to think how to solve the issues @CZEMacLeod and you had presented but I hadn't come up with a set that addressed everything.

I would like to assist in testing this after the rebase and outstanding issues are addressed. Let me know how I can help.

[leusbj]

@CZEMacLeod
I am starting to merge the recent updates down into the branch that is the source for this PR, but I think I'm to a point where I want to get some opinions merging a couple of conflicts.

1. To-Version or Not-To-Version (and if so... do it appropriately)
   a. The recent property ApplySDKDefaultPackageVersions is set to false whenever either of the two "Centralized" methods are incorporated. Then is used to prevent a build errors by preventing the Version metadata from being set by this SDK at all.
   b. The property EnablePackageVersionDefaultsFromSdk from this PR isn't based on the presence of the presence of "Centralized" methods. Instead the assumption is made that this Sdk will make every attempt to bring in the Version metada using the correct time and itemgroup, unless it is told not to... And this EnablePackageVersionDefaultsFromSdk property is the project/repo owner's opportunity to explicitly prevent this SDK from making ANY attempt to incorporate the appropriate items at the appropriate times.
   c. I would like to repurpose your property name, to take on the meaning/behavior to be the more complicated concept. But if you would like to leave that property in place, and for me to use a differently named variable for the more complicated behavior let me know.
2. The latest version of CentralPackageVersions will under certain circumstances set a EnableCentralPackageVersions property to false. and it will only perform it's validation when that property is != false. I am intending to interrogate that property to make decisions about To-Version or Not-To-Version... because in the case of a project that incorporates the SDK, but doesn't create a packages.props, it will have UsingMicrosoftCentralPackageVersionsSdk == true but it will not actually enforce anything... and our goal of setting Version whenever we can should understand that. If you disagree, let me know.
3. Please take a look at this proposed messaging, about times where this SDK has given version information, that a project/repo owner should likely be incorporating into their "Centralized" mechanism
   a. The PackageReference '{PackageName}' was implictly included defaulting to Version='{VersionAsProvided}'. Centralized management of package versions was detected, consider adding an entry to your centralized package version listing.
   b. Please suggest something different if so desired.

[CZEMacLeod]

@leusbj

1. To-Version or Not-To-Version (and if so... do it appropriately)
   a. The recent property ApplySDKDefaultPackageVersions is set to false whenever either of the two "Centralized" methods are incorporated. Then is used to prevent a build errors by preventing the Version metadata from being set by this SDK at all.
   Yes - however, it can be manually set to false to prevent the application of package versions - e.g. if you were applying them in Directory.Build.targets but not doing CPM.

b. The property EnablePackageVersionDefaultsFromSdk from this PR isn't based on the presence of the presence of "Centralized" methods. Instead the assumption is made that this Sdk will make every attempt to bring in the Version metada using the correct time and itemgroup, unless it is told not to... And this EnablePackageVersionDefaultsFromSdk property is the project/repo owner's opportunity to explicitly prevent this SDK from making ANY attempt to incorporate the appropriate items at the appropriate times.
The new property was designed to take on this role.

c. I would like to repurpose your property name, to take on the meaning/behavior to be the more complicated concept. But if you would like to leave that property in place, and for me to use a differently named variable for the more complicated behavior let me know.
It is basically the same role - so yes. If you don't manually set ApplySDKDefaultPackageVersions to false, then try to apply those versions, even with CPM, unless CPM has already set the version numbers.

2. The latest version of CentralPackageVersions will under certain circumstances set a EnableCentralPackageVersions property to false. and it will only perform it's validation when that property is != false. I am intending to interrogate that property to make decisions about To-Version or Not-To-Version... because in the case of a project that incorporates the SDK, but doesn't create a packages.props, it will have UsingMicrosoftCentralPackageVersionsSdk == true but it will not actually enforce anything... and our goal of setting Version whenever we can should understand that. If you disagree, let me know.
   I'm not aware of this behaviour, but if you can handle those circumstances, then please do.
   I don't feel that this SDK should really cover people doing silly things, but if it can JustWork™ in most circumstances, and prevent issues where e.g. it can't restore packages property and ends up in a 'broken' state then I'm all for it.

3. Please take a look at this proposed messaging, about times where this SDK has given version information, that a project/repo owner should likely be incorporating into their "Centralized" mechanism
   a. The PackageReference '{PackageName}' was implictly included defaulting to Version='{VersionAsProvided}'. Centralized management of package versions was detected, consider adding an entry to your centralized package version listing.
   Looks good to me. Do we want to add the text that a user should add (like binding redirects does)? Although that will have to have 2 versions depending on which CPM is in use...
   b. Please suggest something different if so desired.
   Does this have the ability to detect if a package downgrade is happening? I feel that if the SDK says version 5.2.9 of MVC and you have CPM set to 5.2.8 it should perhaps warn? Maybe that isn't really within the scope and in general we don't actually need a specific version of packages (other than perhaps the compilers toolset as that can cause a break on some versions of VS2022).
   Maybe we can shelve that for now and make it an option in the future, just to complete this part.

[CZEMacLeod]

I see you have moved these from the RazorLibrary specific DefaultPackages files.
As these are not used in the main SDK (currently), I'm not sure that I like having them defined without purpose.
My idea was that all common packages/versions would be in Common, and any specific to either the main SDK or RazorLibrary SDK would be in the specific one.
Is there a reason for this move that I am missing?

[leusbj]

No special purpose at the moment. I'll put them back

[CZEMacLeod]

I think you've copied them back, but not removed them here.

[leusbj]

Done

[leusbj]

@CZEMacLeod I've incorporated your feedback (and added comments to your questions).

Do you want me to take a stab at adding a "Unit Test" to the azure-pipelines.yml ?

Potentially something like

- task: VSTest@2
  displayName: 'Run Unit Tests'
  inputs:
    testAssemblyVer2: |
     **\MSBuild.SDK.SystemWeb*.UnitTests*.dll
     !**\obj\**
    codeCoverageEnabled: true
    diagnosticsEnabled: True

I don't know how/if I can experiment with that, and I don't want to break your pipeline, so I'm hesitant to include that into a PR

[CZEMacLeod]

Do you want me to take a stab at adding a "Unit Test" to the azure-pipelines.yml ?

You can certainly add that in. At the moment the pipeline doesn't run for PRs, and I can mess with it (if need be) once this is merged.
I'll look into doing some tests on running the pipeline on PRs (obviously without any signing/publishing) and maybe enable that in the future.

[CZEMacLeod]

I think these shouldn't have the condition of the version on them, as they may not have versions set if using CPM/CPV.

Would it be better to create a new item group, e.g. SDKDefaultPackageReference with these, then at the end of common do a

<PackageReference Include="@(SDKDefaultPackageReference)" 
                  Exclude="@(PackageReference)" 
                  Condition="'$(ExcludeSDKDefaultPackages)'=='false'">
  <Version /><!-- Make sure we don't apply version's here -->
  <SystemWebSdkIncludedPackage>true</SystemWebSdkIncludedPackage>
  <SystemWebSdkIncludedPackageVersion>%(Version)</SystemWebSdkIncludedPackageVersion>
</PackageReference>

or is that adding complexity that it doesn't need? (Would need to import common at the end of razor/sdk defaultpackages instead of the start.)

I just think that having simple to manage lines for the default package refs (and any custom properties such as PrivateAssets, or GeneratePathProperty) would make it simpler to read and maintain - e.g. if adding a new reference.

<ItemGroup>
    <SDKDefaultPackageReference Include="Microsoft.Net.Compilers.Toolset"
                                Version="$(MicrosoftNetCompilersToolset_Version)"
                                PrivateAssets="All"
                                GeneratePathProperty="true" />
    <SDKDefaultPackageReference Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform"
                                Version="$(MicrosoftCodeDomProvidersDotNetCompilerPlatform_Version)" />
</ItemGroup>

[leusbj]

Conditional check on named version property

That Conditional on the version property is a holdover from back before your recent updates.

Previously it looked like this

  <ItemGroup Condition="'$(ExcludeASPNetCompilers)'=='false' AND '$(UsingMicrosoftCentralPackageVersionsSdk)'!='true'">
    <PackageReference Include="Microsoft.Net.Compilers.Toolset" Version="$(MicrosoftNetCompilersToolset_Version)" PrivateAssets="All" Condition="'$(MicrosoftNetCompilersToolset_Version)'!=''"/>
    <PackageReference Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform" Version="$(MicrosoftCodeDomProvidersDotNetCompilerPlatform_Version)" Condition="'$(MicrosoftCodeDomProvidersDotNetCompilerPlatform_Version)'!=''" />
  </ItemGroup>

I had assumed that it was an intentional way of allowing project owners to exclude an individual package (clear out property for single named package), so I had included that old conditional in this rendition. But we can probably go with ExcludeSDKDefaultPackages as the sole decision point.

Separate ItemGroup

I had thought about having some itemgroup that is the list of packages we will work on, but we already have named properties for each package and that is a fairly straight forward way for project owners to tweak if they want to.

In general, I think whatever these end up being, they still needs to be "incorporated" in 2 phases though

1. Ensure that the items are added to PackageReference item group (without adding duplicates)... so after projectfile, but before Directory.Build.targets
2. Ensure appropriate Version metadata is in place... this could be PackageReference or PackageVersion depending on which mechanism is at work... so after Directory.Build.targets

I'll think on it some more

[CZEMacLeod]

Closing this PR for now, as the goals and design has moved quite a long way since this version