Skip to content

Simplify ref/ assembly generation #24136

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

Merged
merged 2 commits into from
Jul 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 1 addition & 3 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,7 @@
Include="$(RepoRoot)src\Analyzers\Internal.AspNetCore.Analyzers\src\Internal.AspNetCore.Analyzers.csproj"
ReferenceOutputAssembly="false"
OutputItemType="Analyzer"
PrivateAssets="All"
Version="$(InternalAspNetCoreAnalyzersPackageVersion)"
IsImplicitlyDefined="true" />
PrivateAssets="All" />
</ItemGroup>

<!-- Compilation options which apply to all languages. Language-specific options should be set in eng/targets/$(lang).Common.props -->
Expand Down
8 changes: 0 additions & 8 deletions eng/Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@
<!-- Project selection can be overridden on the command line by passing in -projects. -->
<When Condition="'$(ProjectToBuild)' != ''">
<ItemGroup>
<!-- Include RTMVersions.csproj unless this invocation is building RepoTasks.csproj or won't compile C#. -->
<ProjectToBuild Include="$(RepoRoot)eng\RTMVersions\RTMVersions.csproj"
Exclude="@(ProjectToExclude)"
Condition=" $(ProjectToBuild.EndsWith('.csproj')) AND !$(ProjectToBuild.EndsWith('RepoTasks.csproj')) " />
<ProjectToBuild Include="$(ProjectToBuild)" Exclude="@(ProjectToExclude);$(RepoRoot)**\bin\**\*;$(RepoRoot)**\obj\**\*">
<RestoreInParallel Condition="'%(Extension)' == '.npmproj'">false</RestoreInParallel>
</ProjectToBuild>
Expand Down Expand Up @@ -125,11 +121,8 @@
<!--
Use caution to avoid deep recursion. If the globbing pattern picks up something which exceeds MAX_PATH,
the entire pattern will silently fail to evaluate correctly.

Include RTMVersions.csproj when building any managed projects.
-->
<DotNetProjects Include="
$(RepoRoot)eng\RTMVersions\RTMVersions.csproj;
$(RepoRoot)src\Framework\App.Ref\src\Microsoft.AspNetCore.App.Ref.csproj;
$(RepoRoot)src\Framework\App.Runtime\src\Microsoft.AspNetCore.App.Runtime.csproj;
$(RepoRoot)src\Framework\App.Ref.Internal\src\Microsoft.AspNetCore.App.Ref.Internal.csproj;
Expand Down Expand Up @@ -178,7 +171,6 @@
$(RepoRoot)**\obj\**\*;"
Condition=" '$(BuildMainlyReferenceProviders)' != 'true' " />
<DotNetProjects Include="
$(RepoRoot)eng\RTMVersions\RTMVersions.csproj;
$(RepoRoot)src\DefaultBuilder\**\src\*.csproj;
$(RepoRoot)src\Features\JsonPatch\**\src\*.csproj;
$(RepoRoot)src\DataProtection\**\src\*.csproj;
Expand Down
2 changes: 2 additions & 0 deletions eng/Dependencies.props
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ 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 RTMVersions.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
6 changes: 0 additions & 6 deletions eng/RTMVersions/Directory.Build.props

This file was deleted.

1 change: 0 additions & 1 deletion eng/RTMVersions/Directory.Build.targets

This file was deleted.

28 changes: 0 additions & 28 deletions eng/RTMVersions/RTMVersions.csproj

This file was deleted.

5 changes: 5 additions & 0 deletions eng/Version.Details.xml
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,11 @@
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>f37dd6fc8595e130909dcb3085a56342d04aa20c</Sha>
</Dependency>
<!-- System.Security.AccessControl should only be referenced in Dependencies.props and RTMVersions.csproj. -->
<Dependency Name="System.Security.AccessControl" Version="5.0.0-preview.8.20361.2">
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>f37dd6fc8595e130909dcb3085a56342d04aa20c</Sha>
</Dependency>
<Dependency Name="System.Security.Cryptography.Cng" Version="5.0.0-preview.8.20361.2">
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>f37dd6fc8595e130909dcb3085a56342d04aa20c</Sha>
Expand Down
13 changes: 10 additions & 3 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@
<SystemReflectionMetadataPackageVersion>5.0.0-preview.8.20361.2</SystemReflectionMetadataPackageVersion>
<SystemResourcesExtensionsPackageVersion>5.0.0-preview.8.20361.2</SystemResourcesExtensionsPackageVersion>
<SystemRuntimeCompilerServicesUnsafePackageVersion>5.0.0-preview.8.20361.2</SystemRuntimeCompilerServicesUnsafePackageVersion>
<!-- System.Security.AccessControl should only be referenced in Dependencies.props and RTMVersions.csproj. -->
<SystemSecurityAccessControlPackageVersion>5.0.0-preview.8.20361.2</SystemSecurityAccessControlPackageVersion>
<SystemSecurityCryptographyCngPackageVersion>5.0.0-preview.8.20361.2</SystemSecurityCryptographyCngPackageVersion>
<SystemSecurityCryptographyPkcsPackageVersion>5.0.0-preview.8.20361.2</SystemSecurityCryptographyPkcsPackageVersion>
<SystemSecurityCryptographyXmlPackageVersion>5.0.0-preview.8.20361.2</SystemSecurityCryptographyXmlPackageVersion>
Expand Down Expand Up @@ -163,6 +165,14 @@
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 RTMVersions.csproj. Because
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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericstj when I updated only $(SystemSecurityAccessControlV0PackageVersion) in testing, the newer (non-RTM) copy of System.Security.AccessControl.dll ended up in our targeting pack. I believe the newer assembly was also used in most compilations. It was difficult to tell exactly why but my guess is the newer System.Security.Principal.Windows.dll was somehow considered incompatible and conflict resolution chose the newer version of both. (The System.Security.Principal.Windows package is a direct dependency of a few of our packages.)

Will we hit something similar if dotnet/runtime ships System.Security.AccessControl 5.0.1 but leaves System.Security.Principal.Windows at 5.0.0❔ Put another way, will dotnet/runtime build but not ship a newer System.Security.Principal.Windows.dll and use that when compiling System.Security.AccessControl.dll❔ If yes, we'll need to be really careful here (perhaps adding further workarounds) and I shouldn't include the "not be necessary" words.

Copy link
Member

Choose a reason for hiding this comment

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

There’s nothing fancy here. It just chooses the newer version. You’ll only see things go app local if you use a newer package than what’s in the shared framework you target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't generally use our own part of the shared Fx when building and the example assemblies are in Microsoft.AspNetcore.App.Ref, not Microsoft.NETCore.App.Ref. So, does "nothing fancy" help us with the corner case I describe❔

I'd like to know we can rev one without the other reliably before we get to 5.0.1. (And, these packages / assemblies have a few other inter-dependencies.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless someone objects, I'm going to assume the worst case here would be silently compiling against 5.0.x (newer) versions of these packages when servicing. Will file an issue about testing that %(RTMVersion) metadata is respected. That test will likely be a bit painful since we won't be building our targeting pack when servicing and checks will require looking at all references in our implementation assemblies for the relevant assemblies then checking their [AssemblyInformationalVersion()] attributes against the expected package version.

-->
<PropertyGroup Condition=" '$(IsServicingBuild)' == 'true' ">
<MicrosoftWin32RegistryV0PackageVersion>$(MicrosoftWin32RegistryPackageVersion.Split('.')[0]).$(MicrosoftWin32RegistryPackageVersion.Split('.')[1]).0</MicrosoftWin32RegistryV0PackageVersion>
Expand Down Expand Up @@ -252,9 +262,6 @@
<MonoCecilPackageVersion>0.11.2</MonoCecilPackageVersion>
<NewtonsoftJsonBsonPackageVersion>1.0.2</NewtonsoftJsonBsonPackageVersion>
<NewtonsoftJsonPackageVersion>12.0.2</NewtonsoftJsonPackageVersion>
<!-- Begin: STOP!!! Razor need to reference the version of JSON that our HOSTS support. -->
<Razor_NewtonsoftJsonPackageVersion>9.0.1</Razor_NewtonsoftJsonPackageVersion>
<!-- End: STOP!!! Razor need to reference the version of JSON that our HOSTS support. -->
<NSwagApiDescriptionClientPackageVersion>13.0.4</NSwagApiDescriptionClientPackageVersion>
<SeleniumSupportPackageVersion>3.12.1</SeleniumSupportPackageVersion>
<SeleniumWebDriverMicrosoftDriverPackageVersion>17.17134.0</SeleniumWebDriverMicrosoftDriverPackageVersion>
Expand Down
12 changes: 7 additions & 5 deletions eng/targets/ResolveReferences.targets
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
</Target>

<!--
This target resolves remaining Referene items to Packages, if possible. If not, they are left as Reference items fo the SDK to resolve.
This target resolves remaining Reference items to Packages, if possible. If not, they are left as Reference items fo the SDK to resolve.
This executes on NuGet restore and during DesignTimeBuild. It should not run in the outer, cross-targeting build.
-->
<Target Name="ResolveCustomReferences"
Expand Down Expand Up @@ -205,21 +205,23 @@
</Target>

<!--
Muck with @(ResolvedCompileFileDefinitions) items between generation and use in order to compile against RTM lib/
Change @(ResolvedCompileFileDefinitions) items between generation and use in order to compile against RTM lib/
Copy link
Member

Choose a reason for hiding this comment

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

Good change 😆

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.

Condition checks for RTMVersions.csproj.nuget.g.props file only to ensure restore operation is complete.

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=" '$(EnableCustomReferenceResolution)' == 'true' AND EXISTS('$(RepoRoot)eng\RTMVersions\obj\RTMVersions.csproj.nuget.g.props') "
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"
Expand Down
10 changes: 10 additions & 0 deletions eng/tools/RepoTasks/RepoTasks.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,15 @@
<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>
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,10 @@
<Compile Include="$(SharedSourceRoot)CommandLineUtils\**\*.cs" />
</ItemGroup>

<ItemGroup>
<Reference Include="Newtonsoft.Json" Version="$(Razor_NewtonsoftJsonPackageVersion)" />
<Reference Include="Microsoft.Css.Parser" Version="$(MicrosoftCssParserPackageVersion)" />
</ItemGroup>

<ItemGroup>
<Reference Include="Microsoft.CodeAnalysis.Razor" />
<Reference Include="Microsoft.Css.Parser" />
<Reference Include="Newtonsoft.Json" />
</ItemGroup>

<!-- This makes it so that the runtimeconfig.json is included as part of the build output of the project that references this project. -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
</PropertyGroup>

<ItemGroup>
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.2.1" Version="$(MicrosoftAspNetCoreAzureAppServicesSiteExtension21PackageVersion)" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.2.2" Version="$(MicrosoftAspNetCoreAzureAppServicesSiteExtension22PackageVersion)" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.3.1.x64" Version="$(MicrosoftAspNetCoreAzureAppServicesSiteExtension31PackageVersion)" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.3.1.x86" Version="$(MicrosoftAspNetCoreAzureAppServicesSiteExtension31PackageVersion)" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.2.1" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.2.2" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.3.1.x64" PrivateAssets="All" />
<Reference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.3.1.x86" PrivateAssets="All" />
<!-- When updating this add the previous SiteExtension(s) to the list above -->
<PackageReference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.5.0.x86" Version="$(PackageVersion)" PrivateAssets="All" />
<PackageReference Include="Microsoft.AspNetCore.AzureAppServices.SiteExtension.5.0.x64" Version="$(PackageVersion)" PrivateAssets="All" />
Expand Down