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

Source Generator projects don't need to be included in other library's sln files #58879

Closed
eerhardt opened this issue Sep 9, 2021 · 8 comments
Labels
area-Infrastructure-libraries needs-author-action An issue or pull request that requires more info or actions from the author.
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Sep 9, 2021

Any library that references System.Text.Json is getting the Json Source Generator project in their .sln file:

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Text.Json.SourceGeneration", "..\System.Text.Json\gen\System.Text.Json.SourceGeneration.csproj", "{160C3D6B-D90A-40B1-A695-81DB79EB24C4}"
EndProject

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Text.Json.SourceGeneration", "..\System.Text.Json\gen\System.Text.Json.SourceGeneration.csproj", "{6699E51A-8DC5-4DBA-A06B-B4A04144E4FA}"
EndProject

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Text.Json.SourceGeneration", "..\System.Text.Json\gen\System.Text.Json.SourceGeneration.csproj", "{D120D5A7-69D9-44A2-B7F7-EE0E48CC0238}"
EndProject

These are unnecessary, bloating the .sln file. With #58446, there are now 2 .csprojs for each source generator - each targeting a different version of Roslyn. Which will make it even worse.

We only really need the Source Generator projects in the .sln files of the libraries that build source generators: Json and Logging.Abstractions.

cc @ViktorHofer

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Sep 9, 2021
@ghost
Copy link

ghost commented Sep 9, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Any library that references System.Text.Json is getting the Json Source Generator project in their .sln file:

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Text.Json.SourceGeneration", "..\System.Text.Json\gen\System.Text.Json.SourceGeneration.csproj", "{160C3D6B-D90A-40B1-A695-81DB79EB24C4}"
EndProject

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Text.Json.SourceGeneration", "..\System.Text.Json\gen\System.Text.Json.SourceGeneration.csproj", "{6699E51A-8DC5-4DBA-A06B-B4A04144E4FA}"
EndProject

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "System.Text.Json.SourceGeneration", "..\System.Text.Json\gen\System.Text.Json.SourceGeneration.csproj", "{D120D5A7-69D9-44A2-B7F7-EE0E48CC0238}"
EndProject

These are unnecessary, bloating the .sln file. With #58446, there are now 2 .csprojs for each source generator - each targeting a different version of Roslyn. Which will make it even worse.

We only really need the Source Generator projects in the .sln files of the libraries that build source generators: Json and Logging.Abstractions.

cc @ViktorHofer

Author: eerhardt
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer ViktorHofer added this to the 7.0.0 milestone Sep 9, 2021
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Sep 9, 2021
@ViktorHofer
Copy link
Member

That is by design as solution files need to be dependency closure complete. If library A depends on System.Text.Json then the source generators of System.Text.Json need to be available in the solution as otherwise System.Text.Json isn't buildable.

In other terms, VS requires all ProjectReferences (which represent the build graph) to be part of the solution.

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

ghost commented May 6, 2022

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

@eerhardt
Copy link
Member Author

eerhardt commented May 6, 2022

then the source generators of System.Text.Json need to be available in the solution as otherwise System.Text.Json isn't buildable.

I think this is where the disconnect lies. The System.Text.Json src project doesn't need the STJ source generator to be buildable. The only reason it has a "project reference" to it is for packing purposes. Because we want the source generator to be packed in the STJ nuget package. But to just build the System.Text.Json src project, the source generator isn't necessary.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels May 6, 2022
@ViktorHofer
Copy link
Member

I think this is where the disconnect lies. The System.Text.Json src project doesn't need the STJ source generator to be buildable. The only reason it has a "project reference" to it is for packing purposes. Because we want the source generator to be packed in the STJ nuget package. But to just build the System.Text.Json src project, the source generator isn't necessary.

Just to double check, you are saying the System.Text.Json source generator isn't an analyzer input to the System.Text.Json source project? If not, then I question the existence of the ProjectReference (AnalyzerReference is a P2P) from S.T.J to the analyzer. That seems wrong. We should find a better way to express that an analyzer should be part of the shared framework.

@eerhardt
Copy link
Member Author

eerhardt commented May 9, 2022

Just to double check, you are saying the System.Text.Json source generator isn't an analyzer input to the System.Text.Json source project?

Correct. Here are the analyzers that are passed during a net7.0 build of the System.Text.Json source project:
image

@ViktorHofer
Copy link
Member

ViktorHofer commented May 9, 2022

I took a closer look. Even though I was able to tackle the underlying problem with #69069, the change won't help with this specific issue. System.Text.Json ships as a package and includes the analyzers in it. Even though the analyzer is not a compile dependency, it's a package dependency which is expressed via the AnalyzerRefernence item(s) in the project file.

When I remove the projects from the solution and delete the obj and bin folders of the two source generator projects I hit the following issue:

8>------ Build started: Project: System.Text.Json (src\System.Text.Json), Configuration: Debug Any CPU ------
8>System.Text.Json -> C:\git\runtime4\artifacts\bin\System.Text.Json\Debug\net7.0\System.Text.Json.dll
8>System.Text.Json -> C:\git\runtime4\artifacts\bin\System.Text.Json\Debug\net6.0\System.Text.Json.dll
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018: The "GenerateDepsFile" task failed unexpectedly.
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018: System.NullReferenceException: Object reference not set to an instance of an object.
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.ReferenceInfo.GetVersion(ITaskItem referencePath)
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.ReferenceInfo.CreateReferenceInfo(ITaskItem referencePath)
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.ReferenceInfo.CreateReferenceInfos(IEnumerable`1 referencePaths)
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.GenerateDepsFile.WriteDepsFile(String depsFilePath)
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.TaskBase.Execute()
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.NET.Build.Tasks.TaskWithAssemblyResolveHooks.Execute()
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
8>C:\git\runtime4\.dotnet\sdk\6.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.targets(172,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()
8>Done building project "System.Text.Json.csproj" -- FAILED.
8>System.Text.Json -> C:\git\runtime4\artifacts\bin\System.Text.Json\Debug\net462\System.Text.Json.dll
========== Build: 1 succeeded, 1 failed, 6 up-to-date, 0 skipped ==========

I have been seeing this happen whenever a solution's dependency graph isn't complete. As said, even though the analyzers aren't a compile dependency, they are dependencies in the graph (an AnalyzerReference is a ProjectReference).

@ViktorHofer ViktorHofer added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels May 10, 2022
@ghost
Copy link

ghost commented May 10, 2022

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

@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

No branches or pull requests

2 participants