-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
CZEMacLeod/MSBuild.SDK.SystemWeb#46 - Adding additional CPM support. … #50
Conversation
…ng RazorLibrary to support predefined PackageVersions.
…with Version ItemGroup.
@brandon-hurler, and @CZEMacLeod I think incorporation of "Default" `<PackageVersion Include...' itemgroup manipulations need to be carefully placed in the build file hierarchy.
Potentially Consider:
|
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. |
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. |
@brandon-hurler I think we are looking for the same things:
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
Case 1 is covered because this SDK will appropriately adds the 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"
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"
|
I like that solution. @CZEMacLeod What are your thoughts on that? I can give @leusbj write permissions to update the PR if needed. |
@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. |
Makes sense. Would you like me to close the PR for now since the scope would be larger to include SystemWeb as a whole? |
@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. |
Sounds good, thank you! |
#46 - Adding additional CPM support for ManagePackageVersionsCentrally within RazorLibrary.
#49 - Updating RazorLibrary to support predefined PackageVersions.