Move to central package pinning #10716
Conversation
This should make it much easier for us to respond to CG alerts in the future. All that will need to be done is add an entry in Directory.Packages.props and it will automatically impact all consumers of it. Consider this example in Roslyn for how to respond to a CG issue dotnet/roslyn#74653
| @@ -21,6 +21,9 @@ | |||
|
|
|||
| <PackageReference Include="Newtonsoft.Json" /> | |||
| <PackageReference Include="Moq" /> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" VersionOverride="$(MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion)" /> | |||
There was a problem hiding this comment.
Just for my own understanding, why are these neccessary?
There was a problem hiding this comment.
I had the same question. Does arcade do something to these packages? Because it seems like the override version would otherwise always match the real version.
There was a problem hiding this comment.
I assume it's because these projects depend on these packages implicitly, and if we didn't set the version explicitly here, they'd be set to the version in the root project, not the overridden version in the compiler projects.
There was a problem hiding this comment.
What is going on here is that there are three sources of two sources of references for MS.CA.CSharp (and the other projects):
- The project ref to Microsoft.CodeAnalysis.Razor.Compiler.csproj which uses version 4.9.2
- The project ref to Microsoft.AspNetCore.Razor.Test.Common.csproj which uses version 4.12.0-1.24379.11
Standard MSBuild logic will say the 4.12.0-1.24379.11 reference is higher therefore lets use that one. But in this case we're using central transitive pinning which means that the version in Directory.Packages.props wins over standard MSBuild logic. For this project it uses src\Compiler\Directory.Packages.props which has MS.CA.CSharp at 4.9.2. Without this VersionOverride we'd end up with un-runnable binaries.
The good news here is that an essential feature of central transitive package pinning is that it will warn aggressively on any silent downgrades like this. That is essential because central transitive pinning is somewhat spooky action at a distance. It's saying any transitive dependency that matches an entry in Directory.Packages.props will be done at the version specified in that file. Hence it aggressively warns if this ever results in a silent downgrade because that is almost certainly what you did not want.
That is exactly what happened here and why I was lead to make the correct override notations.
There was a problem hiding this comment.
Thank you for the explanation. I didn't realise there still was a Directory.Packages.props in the Compiler folder, so I couldn't think of a place where a different version could possibly come from.
There was a problem hiding this comment.
Yeah it's still there because we haven't fully gotten the torn state solution into the .NET SDK. Once we get to say .NET 9 SDK RC1 we don't have to pin the compiler backwards any more and we can start undoing this nest of weirdness.
| @@ -21,6 +21,9 @@ | |||
|
|
|||
| <PackageReference Include="Newtonsoft.Json" /> | |||
| <PackageReference Include="Moq" /> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" VersionOverride="$(MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion)" /> | |||
There was a problem hiding this comment.
I had the same question. Does arcade do something to these packages? Because it seems like the override version would otherwise always match the real version.
…enizer * upstream/main: (53 commits) Move to central package pinning (dotnet#10716) Try to fix rename tests Unskip rename tests I spent 10 minutes looking up cool Mr Freeze catch phrases for this commit message, and I didn't like any of them. Clean up CompilationTagHelperResolver Clean up all ITagHelperDescriptorProviders a bit (and found a bug!) Make ExcludeHidden and IncludeDocumentation init-only properties Swap TagHelperDescriptorProviderContext.Create methods for constructors Remove TagHelperDescriptorProviderContext.Items property Make TargetSymbol a TagHelperDescriptorProviderContext property Make Compilation a TagHelperDescriptorProviderContext property Merge TagHelperDescriptorProviderContext and DefaultContext Don't pass code document and source text around in diagnostics translator, plus some cleanup Remove unnecessary parameter, because it can be trivially retrieved Find razor document correctly in RemoveDocumentMappingService Add extension methods that convert URIs to Roslyn file paths Use Uri.LocalPath rather than GetAbsoluteOrUNCPath() Move MapToHostDocuementUriAndRangeAsync to extension methods Fix small typo in comment Remove unused DocumentState method ...
This should make it much easier for us to respond to CG alerts in the future. All that will need to be done is add an entry in Directory.Packages.props and it will automatically impact all consumers of it.
Consider this example in Roslyn for how to respond to a CG issue
dotnet/roslyn#74653
### Summary of the changes
Fixes: