Fold System.IO.FileSystem into CoreLib

[adamsitnik]

Contributes to #2138

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO, new-api-needs-documentation
Milestone:	-

[adamsitnik]

@jkotas System.Linq dependency was not a problem, as only .ToArray() was being used and I was able to reference EnumerableHelpers to get it working. It turned out that we also have a dependency to Queue<T> and Stack<T> which I had to move as well. Is that acceptable?

I followed the pattern that you have used for System.IO, but I got stuck with the following error:

/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/runtimeGroups.props(277,5): error MSB4018: The "GenerateRuntimeGraph" task failed unexpectedly. [/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj]
/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/runtimeGroups.props(277,5): error MSB4018: System.TypeLoadException: Could not load type 'System.IO.File' from assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. [/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj]
/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/runtimeGroups.props(277,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.GenerateRuntimeGraph.Execute() [/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj]
/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/runtimeGroups.props(277,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj]
/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/runtimeGroups.props(277,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [/home/adam/projects/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj]

@jkotas is there any chance that you could take a look at my changes and see what I am doing wrong?

[jkotas]

Stack<T>

The difference between Stack and List is miniscule. Both are growable arrays, with just slightly different methods exposed. The use of Stack<T> can be trivially replaced by List<T> if you would like to avoid introducing the dependency on Stack.

I think it is ok to move the Queue over.

[jkotas]

I got stuck with the following error:

I do not know what can be causing this error. It looks like a bug in the GenerateRuntimeGraph msbuild task to me. It shoud be using the live-built assets, but it is using cached assets instead. You may want to do a clean build to make sure that it is a real problem.

[adamsitnik]

@ViktorHofer @Anipik In this PR I am trying to move System.IO.File from System.IO.FileSystem to System.PrivateCoreLib. I got stuck with the following issue:

C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\runtimeGroups.props(277,5): error MSB4018: The "GenerateRuntimeGraph" task failed unexpectedly. [C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\Microsoft.NETCore.Platforms.csproj]
C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\runtimeGroups.props(277,5): error MSB4018: System.TypeLoadException: Could not load type 'System.IO.File' from assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. [C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\Microsoft.NETCore.Platforms.csproj]
C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\runtimeGroups.props(277,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.GenerateRuntimeGraph.Execute() [C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\Microsoft.NETCore.Platforms.csproj]
C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\runtimeGroups.props(277,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\Microsoft.NETCore.Platforms.csproj]
C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\runtimeGroups.props(277,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [C:\Projects\runtime\src\libraries\Microsoft.NETCore.Platforms\src\Microsoft.NETCore.Platforms.csproj]

I've verified @jkotas suggestion using a clean build:

It looks like a bug in the GenerateRuntimeGraph msbuild task to me. It shoud be using the live-built assets, but it is using cached assets instead.

and it's true that the msbuild task is using the wrong assets. In my case it's using C:\Projects\runtime\.dotnet\shared\Microsoft.NETCore.App\6.0.0-preview.3.21201.4\System.Private.CoreLib.dll but with the information that System.IO.File has been already moved to System.Private.CoreLib

@ViktorHofer @Anipik do you have any suggestions about how this could be solved (or worked around)?

[ViktorHofer]

and it's true that the msbuild task is using the wrong assets. In my case it's using C:\Projects\runtime.dotnet\shared\Microsoft.NETCore.App\6.0.0-preview.3.21201.4\System.Private.CoreLib.dll but with the information that System.IO.File has been already moved to System.Private.CoreLib

@ericstj was the one who brought the task over from arcade into dotnet/runtime so he's the right person to provide an answer. But to give you some more data meanwhile, the failing task isn't expected to run against the live shared framework as it contributes to the build itself. This is similar to msbuild and other tools that we use to build the repository which also run on a last-known-good shipped version of the shared framework (currently 6.0 Preview 3).

[jkotas]

This is similar to msbuild and other tools that we use to build the repository which also run on a last-known-good shipped version of the shared framework

That means these tools should build against the last-known-good shipped references, and not against the live references.

[ViktorHofer]

That means these tools should build against the last-known-good shipped references, and not against the live references.

Exactly. That needs to be fixed as the project currently targets the live references.

[ViktorHofer]

Had some minutes. Should be fixed with #53290. cc @ericstj

[ViktorHofer]

Curious, what's the benefit of moving System.IO.FileSystem down into CoreLib? And more generally asking what are your thoughts about which types belong into CoreLib and which don't? With time, will more and more types move down into CoreLib? I think that has already been happening over the last years?

[jkotas]

Curious, what's the benefit of moving System.IO.FileSystem down into CoreLib?

Discussed at #53168 (comment)

And more generally asking what are your thoughts about which types belong into CoreLib and which don't?

I think the generalized rule is - when other types in CoreLib need to depend on the type, the type has to be in CoreLib.

[ViktorHofer]

I think the generalized rule is - when other types in CoreLib need to depend on the type, the type has to be in CoreLib.

Doesn't that also mean that we will move more and more types down into CoreLib and it becomes an ever growing assembly? Should all low level APIs like FileSystem belong into CoreLib?

[jkotas]

it becomes an ever growing assembly?

CoreLib is evergrowing assembly because of we have a lot of people adding new APIs to it. I would say that the CoreLib growth from adding new APIs and feature is much higher than the growth from moving things around like in this PR.

We have half of the file I/O in CoreLib already, so it does not sound too concerning to move the other half of file I/O there too, especially since it reduces duplication.

If we started to move e.g. the networking stack (that is also low-level API for some definition of low-level) to CoreLib, I would be more concerned.

[jkotas]

LGTM otherwise. Thanks!

[ViktorHofer]

Given that System.IO.FileSystem only contains type forwards now, should we also remove the references to it in projects that target $(NetCoreAppCurrent) as i.e. System.IO.File is now also exposed in System.Runtime?

BTW is it actually expected that the typedefs moved from System.IO.FileSystem to System.Runtime and that the former now only contains type forwards? I'm seeing issues when referencing both assemblies at the same time as the types are overlapping.

[ViktorHofer]

cc @ericstj

[jkotas]

Given that System.IO.FileSystem only contains type forwards now, should we also remove the references to it in projects that target $(NetCoreAppCurrent) as i.e. System.IO.File is now also exposed in System.Runtime?

We cannot. It would break references to existing packages that still reference System.IO.FileSystem. (Note that we have quite refererence .dlls in netcoreapp that only contain forwarders.)

I'm seeing issues when referencing both assemblies at the same time as the types are overlapping.

If you are seeing conflicting types, I think you are using inconsistent set of references - some are from live netcoreapp6 and some are from older netcoreapp.

[ViktorHofer]

If you are seeing conflicting types, I think you are using inconsistent set of references - some are from live netcoreapp6 and some are from older netcoreapp.

This was because of a non clean build 🤦 - sorry.