Skip to content
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

Remove libraries folders that don't have src/*.cs files #78978

Closed
jozkee opened this issue Nov 29, 2022 · 11 comments · Fixed by #89184
Closed

Remove libraries folders that don't have src/*.cs files #78978

jozkee opened this issue Nov 29, 2022 · 11 comments · Fixed by #89184
Assignees
Labels
area-Infrastructure-libraries question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@jozkee
Copy link
Member

jozkee commented Nov 29, 2022

Based on a conversation on https://github.com/dotnet/runtime/pull/78935/files#r1035029354.

It would be nice if we could remove folders that don't generate implementation assemblies because their types were moved to another assembly for unification.

If I browse through C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0 I can see that there are assemblies that ship with dotnet and don't have a folder or a csproj in our src\libraries. e.g: System.IO.Compression.FileSystem.dll.

Is it possible to remove more folders from libraries in a similar way?
In case is not possible, what makes System.IO.Compression.FileSystem.dll special?

In case of being possible we could be able to potentially remove all these folders (at least from the ones owned by my area pod):

  • System.IO.FileSystem.Primitives
  • System.IO.FileSystem
  • System.IO.Pipes.AccessControl
  • System.IO.UnmanagedMemoryStream
  • System.IO
  • System.Security.Cryptography.Algorithms
  • System.Security.Cryptography.Cng
  • System.Security.Cryptography.Csp
  • System.Security.Cryptography.Encoding
  • System.Security.Cryptography.OpenSsl
  • System.Security.Cryptography.Primitives
  • System.Security.Cryptography.X509Certificates
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 29, 2022
@ghost
Copy link

ghost commented Nov 29, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on a conversation on https://github.com/dotnet/runtime/pull/78935/files#r1035029354.

It would be nice if we could remove folders that don't generate implementation assemblies because their types were moved to another assembly for unification.

If I browse through C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0 I can see that there are assemblies that ship with dotnet and don't have a folder or a csproj in our src\libraries. e.g: System.IO.Compression.FileSystem.dll.

Is it possible to remove more folders from libraries in a similar way?
In case is not possible, what makes System.IO.Compression.FileSystem.dll special?

In case of being possible we could be able to potentially remove all these folders (at least from the ones owned by my area pod):

  • System.IO.FileSystem.Primitives
  • System.IO.FileSystem
  • System.IO.Pipes.AccessControl
  • System.IO.UnmanagedMemoryStream
  • System.IO
  • System.Security.Cryptography.Algorithms
  • System.Security.Cryptography.Cng
  • System.Security.Cryptography.Csp
  • System.Security.Cryptography.Encoding
  • System.Security.Cryptography.OpenSsl
  • System.Security.Cryptography.Primitives
  • System.Security.Cryptography.X509Certificates
Author: Jozkee
Assignees: -
Labels:

area-System.Security

Milestone: -

@jozkee jozkee added area-Infrastructure-libraries question Answer questions and provide assistance, not an issue with source code or documentation. and removed area-System.Security labels Nov 29, 2022
@ghost
Copy link

ghost commented Nov 29, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on a conversation on https://github.com/dotnet/runtime/pull/78935/files#r1035029354.

It would be nice if we could remove folders that don't generate implementation assemblies because their types were moved to another assembly for unification.

If I browse through C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0 I can see that there are assemblies that ship with dotnet and don't have a folder or a csproj in our src\libraries. e.g: System.IO.Compression.FileSystem.dll.

Is it possible to remove more folders from libraries in a similar way?
In case is not possible, what makes System.IO.Compression.FileSystem.dll special?

In case of being possible we could be able to potentially remove all these folders (at least from the ones owned by my area pod):

  • System.IO.FileSystem.Primitives
  • System.IO.FileSystem
  • System.IO.Pipes.AccessControl
  • System.IO.UnmanagedMemoryStream
  • System.IO
  • System.Security.Cryptography.Algorithms
  • System.Security.Cryptography.Cng
  • System.Security.Cryptography.Csp
  • System.Security.Cryptography.Encoding
  • System.Security.Cryptography.OpenSsl
  • System.Security.Cryptography.Primitives
  • System.Security.Cryptography.X509Certificates
Author: Jozkee
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@vcsjones
Copy link
Member

System.Security.Cryptography.Algorithms
System.Security.Cryptography.Cng
System.Security.Cryptography.Csp
System.Security.Cryptography.Encoding
System.Security.Cryptography.OpenSsl
System.Security.Cryptography.Primitives
System.Security.Cryptography.X509Certificates

All of these assemblies were collapsed in to System.Security.Cryptography. However, we need the ref assemblies for type forwarding to work. There is also the test consolidation work that is being tracked in #66338.

@jozkee
Copy link
Member Author

jozkee commented Nov 29, 2022

However, we need the ref assemblies for type forwarding to work.

Yes, I'm asking if there's a better way to make type forwarding and why there's no ref project/source for System.IO.Compression.FileSystem.dll.

If there's no other way, we are meant to keep forever folders of old assemblies whose types were shifted. That may not be a bad thing but it will keep piling-up over the time.

@ViktorHofer
Copy link
Member

If I browse through C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0 I can see that there are assemblies that ship with dotnet and don't have a folder or a csproj in our src\libraries. e.g: System.IO.Compression.FileSystem.dll.

These assemblies are .NETFramework/.NETStandard shims and are still live built but hidden in a subdirectory under src/libraries/shims.

Is it possible to remove more folders from libraries in a similar way?

We need to keep both the reference and the source assembly building as the ref is exposed in the targeting pack and the lib is part of the runtime pack / shared framework. Removing either one is a breaking change. In general, it would be possible to move these into a subdirectory to hide them but that would require infrastructure adjustments.

@ViktorHofer ViktorHofer added this to the Future milestone Dec 1, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Dec 1, 2022
@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 14, 2023

With c0cca33, it should now be relatively straightforward to move pure facade libraries (typeforwards in both ref and src) under src/libraries/shims. When doing that, the tests should be moved to the location that the type forwards point to.

@jozkee feel free to start with https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.FileSystem.Primitives. For that, you would just need to do the following:

  1. Move its tests to the reference source code location (System.Runtime/tests)
  2. Move the reference source project to src/libraries/shims/System.IO.FileSystem.Primitives/src/ and add the StrongNameKey from the Directory.Build.props file directory into the project file. Remove the compile items from the project file as EnableDefaultItems is enabled for shim projects. Change the P2P from ref/System.Runtime.csproj to src/System.Runtime.csproj.
  3. Move the src/libraries/System.IO.FileSystem.Primitives/ref/System.IO.FileSystem.Primitives.Forwards.cs to src/libraries/shims/System.IO.FileSystem.Primitives/src/System.IO.FileSystem.Primitives.cs
  4. Delete all the other files (sln, README.md, Directory.Build.props, src/)
  5. Make sure that no other project has a <Reference Include="System.IO.FileSystem.Primitives" /> item. A direct reference isn't necessary anymore as types exist directly in ref/System.Runtime.dll.
  6. nit: format the project file so that it looks similar to the other shim project files (layout)

If that works well, we can move the other shim projects into there as well. Please let me know if the steps above work for you.

@ViktorHofer ViktorHofer added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 14, 2023
@ghost
Copy link

ghost commented Apr 14, 2023

This issue has been marked needs-author-action and may be missing some important information.

@ghost ghost added the no-recent-activity label Apr 28, 2023
@ghost
Copy link

ghost commented Apr 28, 2023

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

@ghost
Copy link

ghost commented May 12, 2023

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@ghost ghost closed this as completed May 12, 2023
@ViktorHofer
Copy link
Member

@jozkee @bartonjs @adamsitnik is there anything else that you need from the libraries infrastructure team to make this change? Assuming there isn't any infrastructure work needed, should we break this down into smaller issues for the different area owners?

@ghost ghost removed the no-recent-activity label May 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2023
@ViktorHofer ViktorHofer reopened this Jul 19, 2023
@ViktorHofer
Copy link
Member

Let me reopen the issue as I had some time over the weekend to look into this further. The complete list of full facade assemblies (assemblies that only contain type forwards) is:

System.AppContext
System.Buffers
System.Data.DataSetExtensions
System.Diagnostics.Debug
System.Diagnostics.Tools
System.Dynamic.Runtime
System.Globalization
System.Globalization.Calendars
System.Globalization.Extensions
System.IO
System.IO.FileSystem
System.IO.FileSystem.Primitives
System.IO.UnmanagedMemoryStream
System.Reflection
System.Reflection.Extensions
System.Resources.Reader
System.Resources.ResourceManager
System.Runtime.CompilerServices.Unsafe
System.Runtime.Extensions
System.Runtime.Handles
System.Runtime.RuntimeInformation
System.Security.Cryptography.Algorithms
System.Security.Cryptography.Cng
System.Security.Cryptography.Csp
System.Security.Cryptography.Encoding
System.Security.Cryptography.OpenSsl
System.Security.Cryptography.X509Certificates
System.Security.Principal
System.Security.SecureString
System.Text.Encoding
System.Threading.Tasks
System.Threading.Tasks.Extensions
System.Threading.Timer
System.ValueTuple
System.Xml.XmlDocument

All of those should be moved into src/libraries/shims. Some of these typeforward to different assemblies between src and ref but for the ones that typeforward to the same assembly, we only need one assembly (means merging ref and src). I started working on this, here's my draft: #89184

@ViktorHofer ViktorHofer removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 19, 2023
ViktorHofer added a commit that referenced this issue Jul 19, 2023
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.
@carlossanlop carlossanlop added the in-pr There is an active PR which will close this issue when it is merged label Jul 20, 2023
ViktorHofer added a commit that referenced this issue Jul 21, 2023
* Move full facade assemblies into src/libraries/shims

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.

* Trigger CI for all legs

* Remove obsolete slns and undo CI change

* PR feedback from Eric

* Change System.ValueTuple type fowrard dest

* Collapse ref and src shims further

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.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants