Move full facade assemblies into src/libraries/shims #89184
Conversation
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsFixes #78978
This minimizes the build graph (makes the libs build faster) and moves the focus away from full facade assemblies that need to be kept for compat reasons.
|
ViktorHofer
force-pushed
the
MoveFullFacadeAssembliesToShimsDir
branch
from
2792523
to
97c25d5
Compare
Fixes #78978 1. Move all full facade assemblies (which only contain type forwards) into src/libraries/shims. 2. Merge assemblies (ref+src) which typeforward to the same destination. 3. They inherently now don't produce a documentation file anymore (as shims only contain type forwards). This minimizes the build graph (makes the libs build faster) and moves the focus away from full facade assemblies that need to be kept for compat reasons.
ViktorHofer
force-pushed
the
MoveFullFacadeAssembliesToShimsDir
branch
from
97c25d5
to
261c760
Compare
There was a problem hiding this comment.
The change looks straight-forward and reasonable. Things that might go wrong are assembly names changing, or the set of forwards changing. This could happen in ref or src facades (sometimes we forward more in src facades for serialization purposes, for example).
It would be good to do some manual check to double check that we're getting coverage of those. Either by forcing a break in each unique scenario and ensuring API compat catches it / or doing a diff of ildasm'ed output of the changed assemblies.
It looks to me like https://github.com/dotnet/runtime/blob/main/src/libraries/shims/README.md could also use an update.
Assuming these steps are taken, I'm OK with this change.
| @@ -7,8 +7,6 @@ | |||
| <CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies> | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <DefaultReferenceExclusion Include="System.Diagnostics.Debug" /> | |||
| <DefaultReferenceExclusion Include="System.Runtime.Extensions" /> | |||
| <DefaultReferenceExclusion Include="System.Collections" /> | |||
There was a problem hiding this comment.
What's the reason for these and the change? Is it just to avoid duplicate types?
There was a problem hiding this comment.
Correct. The default reference exclusion was necessary when these assemblies still contained types and weren't pure facade assemblies because of duplicate types.
| @@ -1,17 +0,0 @@ | |||
| # System.Threading.Tasks | |||
There was a problem hiding this comment.
Seems reasonable to remove this, but would like to get ack from @dotnet/area-system-threading-tasks
src/libraries/System.Diagnostics.TraceSource/src/System.Diagnostics.TraceSource.csproj
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.AccessControl/src/System.IO.FileSystem.AccessControl.csproj
Show resolved
Hide resolved
...eading.Thread.WebAssembly.Threading/ref/System.Threading.Thread.WebAssembly.Threading.csproj
Show resolved
Hide resolved
src/libraries/shims/System.ValueTuple/ref/System.ValueTuple.csproj
Outdated
Show resolved
Hide resolved
src/libraries/shims/System.Threading.Timer/ref/System.Threading.Timer.cs
Outdated
Show resolved
Hide resolved
src/libraries/shims/System.Security.Cryptography.Csp/src/System.Security.Cryptography.Csp.cs
Show resolved
Hide resolved
Sure. So the basic validation is provided by APICompat which validates all the reference assemblies' type forwards. In addition to that, to also check assembly references, I did invoke ildasm on all the produced assemblies and diffed them with their previous state (from the P6 shared framework). I did collect those and invoked ildasm via a small C# wrapper project. The diff is here, which I then looked at with windiff (which allows to diff directories): shims-diff-new.zip |
As discussed with Eric offline, typeforwarding to System.Runtime and System.Xml.ReaderWriter makes it possible to collapse the following ref and src projects into just src: System.AppContext System.Buffers System.Diagnostics.Debug System.Diagnostics.Tools System.Globalization System.Globalization.Calendars System.IO.UnmanagedMemoryStream System.Reflection System.Resources.ResourceManager System.Runtime.CompilerServices.Unsafe System.Runtime.Extensions System.Security.Principal System.Text.Encoding System.Threading.Timer System.Xml.XmlDocument The destination then handles the type fowarding to the private assembly implementation.
|
This affected my workflow: |
|
The System.Reflection tests will probably be moved under System.Runtime/tests, tracked via #89230. When that's completed, you can achieve what you had previously, but with System.Runtime instead of System.Reflection as the argument name. |
|
@ViktorHofer since there is no .sln for System.Reflection any longer, I need to manually compile the dependencies for System.Reflection.Tests (S.R.TestExe, S.R.Tests.Assembly_*) which is probably easiest done by If we have one large runtime sln in the future to contains all of the runtime tests, we have to have a way to run a given subset quickly; otherwise, I'd likely need to fall back to manually unloading projects in VS to speed it up (which I need to do in other large solutions today, like M.Extensions.Hosting). |
ghost
locked as resolved and limited conversation to collaborators
Fixes #78978
The change itself is quite simple. Move full partial facade projects into src/libraries/shims. The projects that had identical type forward destination in the ref and src are converged into a single assembly. The others move to a single assembly as well, by introducing an additional type forward hop from System.Runtime -> System.Private.CoreLib and System.Private.Uri or System.Xml.ReaderWriter -> System.Privatem.Xml.
This minimizes the build graph (makes the libs build faster), avoids using the
IsPartialFacadeAssemblyinfrastructure feature (which speeds up the build further) and moves the focus away from full facade assemblies that need to be kept for compat reasons.Filed #89230 to track moving tests as well.