search

CZEMacLeod / MSBuild.SDK.SystemWeb

This MSBuild SDK is designed to allow for the easy creation and use of SDK (shortform) projects targeting ASP.NET 4.x using System.Web.
MIT License
151 stars 8 forks source link

Central Package Management - Default Package Restore Failure #49

Closed brandon-hurler closed 1 year ago

brandon-hurler commented 1 year ago

When using Central Package Management, the default package restores fail if an explicit PackageVersion does not exist for the dependent packages the SDK is using.

e.g., I have an explicit PackageVersion set for:

However, I get the error for the two packages the SDK uses that I did not set references for.

error NU1010: The PackageReference items Microsoft.Net.Compilers.Toolset;RazorGenerator.MsBuild do not have corresponding PackageVersion.

It looks like there needs to be additional checks to see if there is an existing PackageVersion defined for each dependent package.

CZEMacLeod commented 1 year ago

@brandon-hurler I'm not quite sure why you think there needs to be an additional check, or what it would do? The existing error message from CPM is pretty clear as to what is happening/what the problem is, and how to solve it.

The RazorGenerator.MsBuild package is included if you are using the MSBuild.SDK.SystemWeb.RazorLibrary SDK to build razor pages (ensuring it will correctly update any dependent code without having any installed extensions etc. and works in all VS versions and build servers). The Microsoft.Net.Compilers.Toolset package allows using newer C# language features in said razor pages (and ensures that the same version of the compilers are used on a build server.)

The list of packages is shown in the DefaultPackages file for each SDK - e.g. MSBuild.SDK.SystemWeb.RazorLibrary.DefaultPackages.targets and the default version numbers (if you are not using CPM) are shown in MSBuild.SDK.SystemWeb.RazorLibrary.DefaultPackages.props.

You can set the property ExcludeDefaultRazorPackages to true if you don't want the default packages included (or ExcludeASPNetCompilers in the case of the MSBuild.SDK.SystemWeb SDK)

I guess it might be possible to check if the PackageVersion is defined and if missing set it as per the defaults, but I am not aware of any other SDK or package doing this kind of logic, and I feel it would mean 'magic' was happening inside the SDK that was not clear to the user. e.g. if there were 2 projects referencing different SDK versions, they might end up with different package versions (which kind of negates the point of CPM).

brandon-hurler commented 1 year ago

If you have CPM enabled but do not have an existing dependency on the packages included in the SDK, then you won't have any version for the project / solution likely at all. Using PackageReference in combination with PackageVersion, you do not define any version on the PackageReference directly.

For that use-case, if someone using CPM were to install the SDK and not already have a dependency on all the packages the SDK would need, it would break the restore.

It's not that I can't fix it on my end, that would be pretty easy to do. But without checking for the PackageVersion as well, the SDK can get out of state with its own dependencies.

CZEMacLeod commented 1 year ago

I am aware of how CPM works w.r.t. defining the version numbers (there is an open issue #46 about the new CPM included in nuget vs. the version in Microsoft.Build.CentralPackageVersions which is already supported)

If you add a project using the template for the SDK, then you will need to modify/add the code from that issue at present if you are using the new CPM.

The package restore will fail in this case if any of the packages do not have a central package version defined, correct, however, there is a sensible error message provided and appropriate versions can be added (as a one off for the solution) to your central package versions file.

If you have an example of another SDK that has this additional logic you are talking about, please let me know so I can look at it.

I'm not quite sure what you mean by 'if someone were to install the SDK' - it is not a nuget pacakge you install - see the How can I use these SDKs? section.

Also, what you mean about the SDK getting out of sync with its dependencies? Do you mean, that if it requires a specific minimum version of one of these it could cause a problem? The default versions can be overridden and set if required (for instance to a newer release of the compilers) without a need for a new version of the SDK as it stands, but there are no 'checks' if the version will work as intended in that case either.

I don't believe that any of the packages will fail if they are downgraded, or upgraded as it stands - they are mainly there to quickly add the main references required, in a similar way to the net6+ SDK, such that you don't need any additional package references. They can also be disabled as I said before, if you want to manually add all the package references (and package versions) to you project/solution manually.

If there are any specific checks that would make this easier to use, feel free to create a PR with them, and we can look at merging that (along with the general changes to resolve #46). I think it might just be a case of improving the documentation for the project - again I welcome PRs to flesh out any of the functionality/documentation.

A lot of the SDK is designed around ease of use in the basic case, along with the ability to override functions and setting for an advanced user. I don't think CPM is a basic case, and people using it would know how to resolve the issue - but I could be wrong about that.

brandon-hurler commented 1 year ago

I added a PR to address this issue and #46 for the RazorLibrary. Here's what I wanted to address:

julealgon commented 1 year ago

Is there a reason this is still open? Hasn't it been solved by #54 above?

CZEMacLeod commented 1 year ago

@julealgon #54 has been closed without merging as the design and structure of the solution moved quite a way in the discussions.

62 / #59 has an update for the default version used, and the 'workaround' of defining the versions in the CPM packages file should work.

However, there are no 'default versions' applied when using a CPM system for now. Once the latest PRs are pulled in, I'll try and build something from the discussion in #56, although, as I pointed out in the comments, I don't actually think this is a bug as such, much more an enhancement, with a fairly limited use case.

julealgon commented 1 year ago

@julealgon #54 has been closed without merging

Oh I completely missed that fact, my bad.

Thanks for the clarifications @CZEMacLeod . The reason I went over some of these issues now is that we'll potentially begin a substantial migration towards SDK-style projects on our side here and we do need to make sure a few old webforms and MVC Razor projects will not only retain full compatibility but also that we know about any challenges coming up. Central package management is also something I'd like to introduce ASAP.

Will apply the workaround in the meantime but will also keep track of this.

Appreciate your work on this library very much.

willbush commented 11 months ago

I just ran into an CPM issue with 4.0.88 of this library due to default packages Microsoft.Net.Compilers.Toolset and Microsoft.CodeDom.Providers.DotNetCompilerPlatform getting included. The error said the version was not specified. I worked around the issue by simply setting ExcludeSDKDefaultPackages true.

CZEMacLeod commented 11 months ago

@willbush You can either set ExcludeSDKDefaultPackages to true, or include versions for those two packages for now.

<ItemGroup>
  <PackageVersion Include="Microsoft.Net.Compilers.Toolset" Version="$(MicrosoftNetCompilersToolset_Version)" />
  <PackageVersion Include="Microsoft.CodeDom.Providers.DotNetCompilerPlatform" Version="$(MicrosoftCodeDomProvidersDotNetCompilerPlatform_Version)" />
</ItemGroup>

Should work