CZEMacLeod/MSBuild.SDK.SystemWeb#46 - Adding additional CPM support. …

[brandon-hurler]

CZEMacLeod/MSBuild.SDK.SystemWeb#46 - Adding additional CPM support for ManagePackageVersionsCentrally within RazorLibrary. CZEMacLeod/MSBuild.SDK.SystemWeb#49 - Updating RazorLibrary to support predefined PackageVersions.

[leusbj]

@brandon-hurler, and @CZEMacLeod

I think incorporation of "Default" `<PackageVersion Include...' itemgroup manipulations need to be carefully placed in the build file hierarchy.

• The reccomendation for ManagePackageVersionsCentrally does appear to be for project owners to use the Directory.Build.props and/or Directory.Packages.props to set these PackageVersion items (and this will get pulled in via the "Microsoft.NET.Sdk/Sdk.props-> Microsoft.Common.props" chain)
• Directory.Build.props and Directory.Packages.props would appear to happen before this package's DefaultPackages.props.

Potentially Consider:

• Conditionally including these PackageVersion items only if it has not already been added (since it is possible-to-likely that items will be present for anyone using ManagePackageVersionsCentrally).
• Relocating the inclusion of PackageVersion items to the DefaultPackages.targets file to ensure that the conditional include happens "late" in the process, after all the project owner's ".props" files

[brandon-hurler]

I had considered both of those - here is why I went with the current implementation.

The default package version will allow the SDK to work out of the box with CPM instances that do not have MVC already implemented.

If you already have it implemented, putting it in props allows CPM to update the entry with the "Update" attribute if you want to define a specific version. If you do not need a specific version, you can omit the Include entirely and rely on the SDK for the reference version.

If you put it in the targets file as suggested, the SDK will overwrite the CPM version, which would have even more unintended side effects.

Lastly, if you are implementing this SDK with CPM, unless you are targeting specific versions, you would be forced to maintain the dependencies and their versions rather than letting the SDK definition handle these for you.

If you know of a way to conditionally target if a PackageVersion is already defined, that would be best. However, from what I can find, there is not support for that in existing PackageVersion support.

[brandon-hurler]

Additionally, if you are worried about keeping current functionality 100% unaffected, you can change ExcludeDefaultRazorPackageVersions to true in the pull request and it will be functionally identical to the current implementation as far as PackageVersion targeting goes.

[leusbj]

@brandon-hurler I think we are looking for the same things:

1. Allow Project owners that do utilize CPM, and have not previously attempted to govern these "implicitly" included packages (like "Microsoft.Net.Compilers.Toolset") to incorporate this SDK without any additional configuration, by providing some reasonable "defaults"
2. Allow Project owners that do utilize CPM, and have already governed these "implicitly" included packages (like "Microsoft.Net.Compilers.Toolset") to not have to change any previously in place PackageVersion items (like making them changing Include to Update)
3. Allow Project owners that do utilize CPM, and have already governed these "implicitly" included packages (like "Microsoft.Net.Compilers.Toolset"), and have individual project overrides aka PackageVersion items in specific project file (overriding the repo provided defaults) to still be able to specify the version right for that project

My only concern is about where this SDK attempts to provide that default. Ideally it happens after the .props files, and after the project file

For instance, assume something like this is present in the DefaultPackages.targets

  <ItemGroup Label="Assume this is the itemgroup as defined in this Sdk"
      Condition=" ('$(ExcludeDefaultRazorPackages)' == 'false' AND '$(ExcludeDefaultRazorPackageVersions)' == 'false') AND ('$(UsingMicrosoftCentralPackageVersionsSdk)' == 'true' OR '$(ManagePackageVersionsCentrally)' == 'true')"
    >
    <PackageVersion Include="Microsoft.Net.Compilers.Toolset" 
                    Exclude="@(PackageVersion)"
              Version="$(MicrosoftNetCompilersToolset_Version)" />
  </ItemGroup>

Case 1 is covered because this SDK will appropriately adds the PackageVersion when nothing existed previously

Case 2 is covered if something like this was added into the Build.*.props, then this SDK's logic will not add a duplicate... note this snippet below actually happens first, and the SDK will happen "late"

<ItemGroup Label="Assume this itemgroup is in Directory.Packages.props ">
  <PackageVersion Include="Microsoft.Net.Compilers.Toolset" 
                  Version="$(ToolsetVersion_AsManagedByTheProjectRepo_via_Directoryprops)" />   
</ItemGroup>

Case 3 is covered if something like this was added into the Build.*.props and project file (in this case project owner already knew that entire repo was desiring version A and they have previously chosen to use a different version), then this SDK's logic will not add a duplicate... note these snippets below actually happens first, and the SDK will happen "late"

<ItemGroup Label="Assume this itemgroup is in Directory.Packages.props ">
  <PackageVersion Include="Microsoft.Net.Compilers.Toolset" 
                  Version="$(ToolsetVersion_AsManagedByTheProjectRepo_via_Directoryprops)" />   
</ItemGroup>
<ItemGroup Label="Assume this itemgroup is in project file to make a project specific version ">
  <PackageVersion Update="Microsoft.Net.Compilers.Toolset" 
                  Version="$(ToolsetVersion_AsDesiredBySpecificProject)" />   
</ItemGroup>

[brandon-hurler]

I like that solution. @CZEMacLeod What are your thoughts on that? I can give @leusbj write permissions to update the PR if needed.

[CZEMacLeod]

@brandon-hurler Yes - I think this is partly why I was reticent to do something like this - the number of cases which need to be covered - especially if a person has already got versions defined, made this feel like it was going to break something. If the mechanism @leusbj suggested will work (and I think it does although I haven't been able to test it) then I think that is a better solution. I also think we should put a property in like ExcludeSDKPackageReferenceVersions which will combine the logic of ('$(UsingMicrosoftCentralPackageVersionsSdk)' != 'true' AND '$(ManagePackageVersionsCentrally)' != 'true'). Also Microsoft.Build.CentralPackageVersions does not use PackageVersion - so it would only be relevant for ManagePackageVersionsCentrally. And finally, this would need to be done for both SDK packages - MSBuild.SDK.SystemWeb.RazorLibrary and MSBuild.SDK.SystemWeb I'm not sure, but I feel like there should be some kind of warning or info message here, that if the default package versions are used that they are not being handled and managed by CPM's Directory.Packages.props file, but by the SDK. This might be relevant if you have both SDK`s in a solution (quite valid/likely) and they are on different versions, and they therefore might have different default versions.

[brandon-hurler]

Makes sense. Would you like me to close the PR for now since the scope would be larger to include SystemWeb as a whole?

[CZEMacLeod]

@brandon-hurler Up to you. SystemWeb only has a subset of the same packages - so it would just be a copy/paste from one to the other as far as that goes. If you want to update the PR to include the mechanism @leusbj suggested that would be fine. We can factor out the additional property later on, or I can add it as part of this PR before it gets committed.

[CZEMacLeod]

Item #46 has been dealt with in the latest release V4.0.88 and the defaults will be dealt with #54 when it has been rebased on the current update. I think this covers everything that was in this PR so I am going to close it for now.

[brandon-hurler]

Sounds good, thank you!