Skip to content

Remove package version pinning for ref/ assemblies in servicing #25851

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 13, 2020
Merged

Remove package version pinning for ref/ assemblies in servicing #25851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e31c1e6
Remove package version pinning for ref/ assemblies in servicing
dougbu Sep 12, 2020
0c4774f
Merge branch 'release/5.0-rc2' into dougbu/unpin.package.versions
dougbu Sep 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions eng/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ and are generated based on the last package release.
<LatestPackageReference Include="System.Reflection.Metadata" />
<LatestPackageReference Include="System.Runtime.CompilerServices.Unsafe" />
<LatestPackageReference Include="System.Runtime.InteropServices.RuntimeInformation" />
<!-- System.Security.AccessControl should only be referenced in Dependencies.props and RepoTasks.csproj. -->
<LatestPackageReference Include="System.Security.AccessControl" />
<LatestPackageReference Include="System.Security.Cryptography.Cng" />
<LatestPackageReference Include="System.Security.Cryptography.Pkcs" />
<LatestPackageReference Include="System.Security.Cryptography.Xml" />
Expand Down Expand Up @@ -196,11 +194,6 @@ and are generated based on the last package release.
<!-- Get versions. -->
<LatestPackageReference Update="@(LatestPackageReference)">
<Version>$(%(VersionName)PackageVersion)</Version>
<RTMVersion>$(%(VersionName)V0PackageVersion)</RTMVersion>

<!-- Remove excess metadata. -->
<RTMVersion Condition=" '%(Version)' == '%(RTMVersion)' " />
<VersionName />
</LatestPackageReference>

<!--
Expand Down
5 changes: 0 additions & 5 deletions eng/Version.Details.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,6 @@
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>54dfce73caac716251ff90e1099c4232f5587e35</Sha>
</Dependency>
<!-- System.Security.AccessControl should only be referenced in Dependencies.props and RepoTasks.csproj. -->
<Dependency Name="System.Security.AccessControl" Version="5.0.0-rc.2.20462.5">
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>54dfce73caac716251ff90e1099c4232f5587e35</Sha>
</Dependency>
<Dependency Name="System.Security.Cryptography.Cng" Version="5.0.0-rc.2.20462.5">
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>54dfce73caac716251ff90e1099c4232f5587e35</Sha>
Expand Down
31 changes: 0 additions & 31 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,6 @@
<SystemReflectionMetadataPackageVersion>5.0.0-rc.2.20462.5</SystemReflectionMetadataPackageVersion>
<SystemResourcesExtensionsPackageVersion>5.0.0-rc.2.20462.5</SystemResourcesExtensionsPackageVersion>
<SystemRuntimeCompilerServicesUnsafePackageVersion>5.0.0-rc.2.20462.5</SystemRuntimeCompilerServicesUnsafePackageVersion>
<!-- System.Security.AccessControl should only be referenced in Dependencies.props and RepoTasks.csproj. -->
<SystemSecurityAccessControlPackageVersion>5.0.0-rc.2.20462.5</SystemSecurityAccessControlPackageVersion>
<SystemSecurityCryptographyCngPackageVersion>5.0.0-rc.2.20462.5</SystemSecurityCryptographyCngPackageVersion>
<SystemSecurityCryptographyPkcsPackageVersion>5.0.0-rc.2.20462.5</SystemSecurityCryptographyPkcsPackageVersion>
<SystemSecurityCryptographyXmlPackageVersion>5.0.0-rc.2.20462.5</SystemSecurityCryptographyXmlPackageVersion>
Expand Down Expand Up @@ -162,36 +160,7 @@
-->
<MicrosoftNETCoreAppRuntimeVersion>$(MicrosoftNETCoreAppRuntimewinx64PackageVersion)</MicrosoftNETCoreAppRuntimeVersion>
</PropertyGroup>
<!--
We ship ref/ assemblies for runtime packages in our targeting targeting pack. When servicing that targeting pack,
these assemblies must not change. Must also compile our assemblies against the initial ref/ assemblies for runtime
packages. But, need to test against the latest implementation assemblies and ship them in our shared framework.
Upshot is we need Major.Minor.0 runtime packages for compilation and the targeting pack and Major.Minor.Latest
runtime packages for everything else. This is not an issue for assemblies available in Microsoft.NETCore.App.Ref or
Microsoft.Extensions.Internal.Transport because it is next to impossible we would service those packages.

System.Security.AccessControl should only be referenced in Dependencies.props and RepoTasks.csproj. Because
Copy link
Member

@Pilchie Pilchie Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What changed to make this no longer necessary?

Copy link
Contributor Author

@dougbu dougbu Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the description, "dotnet/runtime now keeps $(AssemblyVersion) values consistent", specifically for the ref/ assemblies packaged outside Microsoft.NETCore.App.Ref and Microsoft.Extensions.Internal.Transport (where versions were already pinned). Their change is checked in for RC2 and so we no longer need any of these overrides.

Did I miss anything @ericstj @Anipik❔

Copy link
Member

@Pilchie Pilchie Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But from the comment we used to include it in our ref pack, even though the Core ref didn't. Don't we still need to do that?

Copy link
Contributor Author

@dougbu dougbu Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll grab the ref/ assemblies from the current (latest) packages rather than the .0 packages. System.Security.AccessControl in particular comes in due to transitive references; we don't need to reference it directly because we don't need to pin the ref/ assembly.

Copy link
Member

@Pilchie Pilchie Sep 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks.

it's a transitive reference, we reship the ref/ assembly in Microsoft.AspNetCore.App.Ref. dotnet/runtime ships
the implementation assemblies in Microsoft.NETCore.App.Runtime.* packages.

If testing this configuration prior to servicing, update the versions of dependencies too. E.g. change
`$(SystemSecurityPrincipalWindowsV0PackageVersion)` if you change `$(SystemSecurityAccessControlV0PackageVersion)`
because System.Security.AccessControl will otherwise be loadable. This should not be necessary in servicing.
-->
<PropertyGroup Condition=" '$(IsServicingBuild)' == 'true' ">
<MicrosoftWin32RegistryV0PackageVersion>$(MicrosoftWin32RegistryPackageVersion.Split('.')[0]).$(MicrosoftWin32RegistryPackageVersion.Split('.')[1]).0</MicrosoftWin32RegistryV0PackageVersion>
<MicrosoftWin32SystemEventsV0PackageVersion>$(MicrosoftWin32SystemEventsPackageVersion.Split('.')[0]).$(MicrosoftWin32SystemEventsPackageVersion.Split('.')[1]).0</MicrosoftWin32SystemEventsV0PackageVersion>
<SystemDiagnosticsEventLogV0PackageVersion>$(SystemDiagnosticsEventLogPackageVersion.Split('.')[0]).$(SystemDiagnosticsEventLogPackageVersion.Split('.')[1]).0</SystemDiagnosticsEventLogV0PackageVersion>
<SystemDrawingCommonV0PackageVersion>$(SystemDrawingCommonPackageVersion.Split('.')[0]).$(SystemDrawingCommonPackageVersion.Split('.')[1]).0</SystemDrawingCommonV0PackageVersion>
<SystemIOPipelinesV0PackageVersion>$(SystemIOPipelinesPackageVersion.Split('.')[0]).$(SystemIOPipelinesPackageVersion.Split('.')[1]).0</SystemIOPipelinesV0PackageVersion>
<SystemSecurityAccessControlV0PackageVersion>$(SystemSecurityAccessControlPackageVersion.Split('.')[0]).$(SystemSecurityAccessControlPackageVersion.Split('.')[1]).0</SystemSecurityAccessControlV0PackageVersion>
<SystemSecurityCryptographyCngV0PackageVersion>$(SystemSecurityCryptographyCngPackageVersion.Split('.')[0]).$(SystemSecurityCryptographyCngPackageVersion.Split('.')[1]).0</SystemSecurityCryptographyCngV0PackageVersion>
<SystemSecurityCryptographyPkcsV0PackageVersion>$(SystemSecurityCryptographyPkcsPackageVersion.Split('.')[0]).$(SystemSecurityCryptographyPkcsPackageVersion.Split('.')[1]).0</SystemSecurityCryptographyPkcsV0PackageVersion>
<SystemSecurityCryptographyXmlV0PackageVersion>$(SystemSecurityCryptographyXmlPackageVersion.Split('.')[0]).$(SystemSecurityCryptographyXmlPackageVersion.Split('.')[1]).0</SystemSecurityCryptographyXmlV0PackageVersion>
<SystemSecurityPermissionsV0PackageVersion>$(SystemSecurityPermissionsPackageVersion.Split('.')[0]).$(SystemSecurityPermissionsPackageVersion.Split('.')[1]).0</SystemSecurityPermissionsV0PackageVersion>
<SystemSecurityPrincipalWindowsV0PackageVersion>$(SystemSecurityPrincipalWindowsPackageVersion.Split('.')[0]).$(SystemSecurityPrincipalWindowsPackageVersion.Split('.')[1]).0</SystemSecurityPrincipalWindowsV0PackageVersion>
<SystemWindowsExtensionsV0PackageVersion>$(SystemWindowsExtensionsPackageVersion.Split('.')[0]).$(SystemWindowsExtensionsPackageVersion.Split('.')[1]).0</SystemWindowsExtensionsV0PackageVersion>
</PropertyGroup>
<PropertyGroup Label="Manual">
<!-- DiagnosticAdapter package pinned temporarily until migrated/deprecated -->
<MicrosoftExtensionsDiagnosticAdapterPackageVersion>5.0.0-preview.4.20180.4</MicrosoftExtensionsDiagnosticAdapterPackageVersion>
Expand Down
39 changes: 0 additions & 39 deletions eng/targets/ResolveReferences.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,45 +253,6 @@
Text="Could not resolve this reference. Could not locate the package or project for &quot;%(Reference.Identity)&quot;. Did you update baselines and dependencies lists? See docs/ReferenceResolution.md for more details." />
</Target>

<!--
Change @(ResolvedCompileFileDefinitions) items between generation and use in order to compile against RTM lib/
or ref/ assemblies. The approach works for all TFMs because it happens after a specific assembly is chosen for
compilation; no need to restrict this to (say) the default TFM.

This target could get confused if the layout changes for one of the dozen special-cased packages during servicing.
E.g. ResolvePackageAssets picks a compatible assembly from the 5.0.1 package and _UseRTMReferenceAssemblies
changes the path to one not present in the 5.0.0 package. Fortunately, this will break the build with complaints
about the "invalid strong name".
-->
<Target Name="_UseRTMReferenceAssemblies"
Condition=" '$(MSBuildProjectName)' != 'RepoTasks' "
AfterTargets="ResolvePackageAssets"
BeforeTargets="GenerateBuildDependencyFile;GeneratePublishDependencyFile;ILLink;ResolveLockFileReferences"
DependsOnTargets="ResolvePackageAssets">
<Error Condition=" !EXISTS('$(RepoRoot)artifacts\obj\RepoTasks\RepoTasks.csproj.nuget.g.props') "
Text="The eng/tools/RepoTasks project must be restored before building other projects." />

<JoinItems Left="@(ResolvedCompileFileDefinitions)"
Right="@(LatestPackageReference->HasMetadata('RTMVersion'))"
LeftKey="Filename"
ItemSpecToUse="Left"
LeftMetadata="*"
RightMetadata="RTMVersion;Version">
<Output TaskParameter="JoinResult" ItemName="_ResolvedCompileFileDefinitionsToChange" />
</JoinItems>

<ItemGroup>
<ResolvedCompileFileDefinitions Remove="@(_ResolvedCompileFileDefinitionsToChange)" />

<!-- Ignore %(NuGetPackageVersion) when doing substitution because some projects use downlevel packages. -->
<_ResolvedCompileFileDefinitionsToChange
HintPath="$([System.String]::new('%(Identity)').Replace('\%(Version)\', '\%(RTMVersion)\').Replace('/%(Version)/', '/%(RTMVersion)/'))" />
<ResolvedCompileFileDefinitions Include="@(_ResolvedCompileFileDefinitionsToChange -> '%(HintPath)')" />

<_ResolvedCompileFileDefinitionsToChange Remove="@(_ResolvedCompileFileDefinitionsToChange)" />
</ItemGroup>
</Target>

<PropertyGroup>
<_CompileTfmUsingReferenceAssemblies>false</_CompileTfmUsingReferenceAssemblies>
<_CompileTfmUsingReferenceAssemblies
Expand Down
10 changes: 0 additions & 10 deletions eng/tools/RepoTasks/RepoTasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,5 @@
<Reference Include="Microsoft.Deployment.WindowsInstaller.Package">
<HintPath>$(WiXSdkPath)\Microsoft.Deployment.WindowsInstaller.Package.dll</HintPath>
</Reference>

<!--
Gather project references for compilation against RTM packages. %(RTMVersion) is set for about a dozen packages
in all servicing builds. Cannot reference two versions of a package, mandating this separation from projects
using the relevant packages.
-->
<PackageReference Include="@(LatestPackageReference->HasMetadata('RTMVersion'))"
IncludeAssets="None"
PrivateAssets="All"
Version="%(RTMVersion)" />
</ItemGroup>
</Project>