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

Automate including the source gen projects in the transport package #91698

Open
tarekgh opened this issue Sep 6, 2023 · 3 comments
Open

Automate including the source gen projects in the transport package #91698

tarekgh opened this issue Sep 6, 2023 · 3 comments

Comments

@tarekgh
Copy link
Member

tarekgh commented Sep 6, 2023

Today it is manual process to include the source gen projects into the transport package. This issue tracks if we can improve this process.

Viktor Hofer
If the expectation is to include the entire package content, then we can just define a package dependency from Microsoft.DotNet.Runtime.AspNetCore.Transport -> i.e. Microsoft.Extensions.Configuration.Binder. That will cause troubles during servicing though. In servicing, we always create the transport packages for the partner teams but libraries packages like System.Text.Json are only created and published on-demand (when something in them changes).
Also, that would mean that we hand-off more assemblies and package assets to partner teams than today.
I.e. the aspnetcore team would then also get the netstandard2.x and net462 assemblies, all the other roslyn version targeted source generators (roslyn3.11, roslyn4.0, ...) and other potential assets from our packages. This would enlargen our current contract and I'm not sure if that's what we want.
Today, our transport packages are hand-tailored, meaning that they only include what's needed: all the reference assemblies, source assemblies and source generator assemblies (for the current roslyn version).
If we want to change that, please file an issue. Note that the current state (including how to add a source generator) is documented here: runtime/docs/coding-guidelines/libraries-packaging.md at main · dotnet/runtime (github.com)

Eric St. John
Viktor Hofer I wonder if we could drive the generator project references in the transport package off a list in NetCoreAppLibrary.props instead (EG: NetCoreAppAnalyzer, AspNetCoreAppAnalyzer).
Then when we build a package, we could see if that package includes a generator and the library is in NetCoreAppLibrary or AspNetCoreAppLibrary, but the generator is missing from the respective list NetCoreAppAnalyzer or AspNetCoreAppAnalyzer. If missing we could raise a suppressible error.

Viktor Hofer
How would you identify the generators that should be added to the transport package? Most of our source generator aware packages include multiple ones, for different compiler versions.

Eric St. John
Good point, I think it would need to be an "Any" vs "None" check.

Viktor Hofer
I only NuGet would allow a dependency to not be promoted to a package dependency without making it non transitive. That would allow the latest targeting source generator to be a transitive dependency without promoting it as a package dependency...

Eric St. John
We could create our own extension to project-reference protocol to flow transitive source generators.

Eric St. John
In metadata that only we look at.

Viktor Hofer
NuGet calculates transitives and package dependencies based on the project's project.assets.json file. AFAIK it's not possible to adjust that algorithm.

Eric St. John
But don't we already use build targets to compose those transport packages, since we're redistributing project references?

Eric St. John
So it doesn't really matter what NuGet does, all we need to do is change the content we provide from the project reference.

Viktor Hofer
Correct. But the transport package still needs the source generator project dependency. The source generator dependency is marked as PrivateAssets="all" in the source project that distributes it which results in it not being listed in the transport package's project.assets.json.

Viktor Hofer
Your proposal above works. I was saying that it would be nice to not have to list source generators dependency and automatically receive it as transitive of the referenced source project.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 6, 2023
@ghost
Copy link

ghost commented Sep 6, 2023

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

Issue Details

Today it is manual process to include the source gen projects into the transitive package. This issue tracks if we can improve this process.

Viktor Hofer
If the expectation is to include the entire package content, then we can just define a package dependency from Microsoft.DotNet.Runtime.AspNetCore.Transport -> i.e. Microsoft.Extensions.Configuration.Binder. That will cause troubles during servicing though. In servicing, we always create the transport packages for the partner teams but libraries packages like System.Text.Json are only created and published on-demand (when something in them changes).
Also, that would mean that we hand-off more assemblies and package assets to partner teams than today.
I.e. the aspnetcore team would then also get the netstandard2.x and net462 assemblies, all the other roslyn version targeted source generators (roslyn3.11, roslyn4.0, ...) and other potential assets from our packages. This would enlargen our current contract and I'm not sure if that's what we want.
Today, our transport packages are hand-tailored, meaning that they only include what's needed: all the reference assemblies, source assemblies and source generator assemblies (for the current roslyn version).
If we want to change that, please file an issue. Note that the current state (including how to add a source generator) is documented here: runtime/docs/coding-guidelines/libraries-packaging.md at main · dotnet/runtime (github.com)

Eric St. John
Viktor Hofer I wonder if we could drive the generator project references in the transport package off a list in NetCoreAppLibrary.props instead (EG: NetCoreAppAnalyzer, AspNetCoreAppAnalyzer).
Then when we build a package, we could see if that package includes a generator and the library is in NetCoreAppLibrary or AspNetCoreAppLibrary, but the generator is missing from the respective list NetCoreAppAnalyzer or AspNetCoreAppAnalyzer. If missing we could raise a suppressible error.

Viktor Hofer
How would you identify the generators that should be added to the transport package? Most of our source generator aware packages include multiple ones, for different compiler versions.

Eric St. John
Good point, I think it would need to be an "Any" vs "None" check.

Viktor Hofer
I only NuGet would allow a dependency to not be promoted to a package dependency without making it non transitive. That would allow the latest targeting source generator to be a transitive dependency without promoting it as a package dependency...

Eric St. John
We could create our own extension to project-reference protocol to flow transitive source generators.

Eric St. John
In metadata that only we look at.

Viktor Hofer
NuGet calculates transitives and package dependencies based on the project's project.assets.json file. AFAIK it's not possible to adjust that algorithm.

Eric St. John
But don't we already use build targets to compose those transport packages, since we're redistributing project references?

Eric St. John
So it doesn't really matter what NuGet does, all we need to do is change the content we provide from the project reference.

Viktor Hofer
Correct. But the transport package still needs the source generator project dependency. The source generator dependency is marked as PrivateAssets="all" in the source project that distributes it which results in it not being listed in the transport package's project.assets.json.

Viktor Hofer
Your proposal above works. I was saying that it would be nice to not have to list source generators dependency and automatically receive it as transitive of the referenced source project.

Author: tarekgh
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@tarekgh tarekgh changed the title Automate including the source gen projects in the transitive package Automate including the source gen projects in the transport package Sep 6, 2023
@ericstj
Copy link
Member

ericstj commented Sep 6, 2023

Your proposal above works. I was saying that it would be nice to not have to list source generators dependency and automatically receive it as transitive of the referenced source project.

Agreed. I think if we just flow them automatically we don't need any additional state. We just need to make sure that whatever pattern we use to include in the package will also do this flowing to transport package.

@ViktorHofer
Copy link
Member

But that requires NuGet tooling support. Today, we decorate the source generator P2Ps in the source projects with PrivateAssets=all to avoid them being promoted to package dependencies. But with that, we also stop them being treated as transitive dependencies of the source project.

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Sep 8, 2023
@ericstj ericstj added this to the 9.0.0 milestone Sep 8, 2023
@ViktorHofer ViktorHofer modified the milestones: 9.0.0, 10.0.0 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants