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

We need a repo convention to ship source generators as part of the ref pack #61321

Closed
ericstj opened this issue Nov 8, 2021 · 9 comments · Fixed by #69069
Closed

We need a repo convention to ship source generators as part of the ref pack #61321

ericstj opened this issue Nov 8, 2021 · 9 comments · Fixed by #69069

Comments

@ericstj
Copy link
Member

ericstj commented Nov 8, 2021

I believe we want to ship the Regex source generator by default in .NET 7.0

I checked https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet7&package=Microsoft.NETCore.App.Ref&protocolType=NuGet&version=7.0.0-alpha.1.21558.2
And noticed that it wasn't present.

I believe because it is not built as part of the normal build that produces the refpack. This is because there is no AnalyzerReference from the src project to the generator project and we have no convention to find and build generator projects as part of src.proj or ref.proj.

We should update the docs to indicate what steps are required (if any additional steps are necessary):
https://github.com/dotnet/runtime/blob/331823f4046da1530dd950ac3ad8a87b3cdd584d/docs/coding-guidelines/libraries-packaging.md#netcoreapp-shared-framework

Possible fixes here are:

  1. Require some building src project add an AnalyzerReference to the shipping source generator.
  2. Add some build convention to src.proj to build the generator.
  3. Some other build convention, eg generators.proj.

@stephentoub @joperezr @eerhardt @safern

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner labels Nov 8, 2021
@ghost
Copy link

ghost commented Nov 8, 2021

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

Issue Details

I believe we want to ship the Regex source generator by default in .NET 7.0

I checked https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=dotnet7&package=Microsoft.NETCore.App.Ref&protocolType=NuGet&version=7.0.0-alpha.1.21558.2
And noticed that it wasn't present.

I believe because it is not built as part of the normal build that produces the refpack. This is because there is no AnalyzerReference from the src project to the generator project and we have no convention to find and build generator projects as part of src.proj or ref.proj.

We should update the docs to indicate what steps are required (if any additional steps are necessary):
https://github.com/dotnet/runtime/blob/331823f4046da1530dd950ac3ad8a87b3cdd584d/docs/coding-guidelines/libraries-packaging.md#netcoreapp-shared-framework

Possible fixes here are:

  1. Require some building src project add an AnalyzerReference to the shipping source generator.
  2. Add some build convention to src.proj to build the generator.
  3. Some other build convention, eg generators.proj.

@stephentoub @joperezr @eerhardt @safern

Author: ericstj
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@ericstj
Copy link
Member Author

ericstj commented Nov 8, 2021

cc @AaronRobinsonMSFT @elinor-fung in case they're also expecting the DllImport generator as well, since it's not currently in the ref pack.

@elinor-fung
Copy link
Member

Thanks @ericstj.
Yeah, we will need to do something here as well when we get to productization (added an item to our team's tracking issue).

@joperezr
Copy link
Member

joperezr commented Nov 8, 2021

Thanks for logging this. I'll follow your first suggestion so that we can get the source generator into the ref pack as soon as possible so we can start getting feedback on it from more people. I'll also log a separate infrastructure issue to talk about conventions, so that we can use that issue to update all of the existing generators once/if a convention is decided upon.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 8, 2021
@kasperk81
Copy link
Contributor

net6 comes with two generators, json and logging. not important for ref pack?

@ericstj
Copy link
Member Author

ericstj commented Nov 9, 2021

Those are already in the ref-packs (JSON in the NetCore.App ref pack, Logging in ASP.NET). They both also ship packages and reference the source generator as part of their src project, which is why they aren't missing.

This is about a new 7.0 generator for RegEx that was not included in the ref-pack.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 9, 2021
@safern safern removed the untriaged New issue has not been triaged by the area owner label Nov 9, 2021
@ericstj
Copy link
Member Author

ericstj commented Nov 30, 2021

Can this be closed now, @joperezr?

@joperezr
Copy link
Member

mm not really, if you see this comment It was decided that this issue would track coming up with a better convention of how to automatically add source generators to the ref pack so that the issue we had in regex wouldn't happen again. I don't think we have done that just yet, we only fixed the individual regex case. We should probably rename this one to match what it is being tracked, or close this and open a new one that tracks specifically the convention work.

@safern safern changed the title Regex Source generator is not currently part of the .NETCore ref-pack We need a repo convention to ship source generators as part of the ref pack Nov 30, 2021
@safern
Copy link
Member

safern commented Nov 30, 2021

I've renamed the issue (despite breaking email threads 😆) to reflect what we are tracking.

@ViktorHofer ViktorHofer self-assigned this May 9, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 9, 2022
ViktorHofer added a commit to ViktorHofer/runtime that referenced this issue May 10, 2022
Fixes dotnet#61321

Until now we required source libraries to define ProjectReferences when
an analyzer should be part of the shared framework. That strategy causes
analyzer projects to leak into the ProjectReference closure and by that
into a solution file.

As an example:
When another library references the source library that references the
analyzer, the analyzer is part of the dependency closure even though it
might not be required.

This change makes it possible to define the shared framework analyzer
projects in the NetCoreAppLibrary.props file for both the .NETCoreApp,
and the AspNetCoreApp shared framework.

Out-of-band projects which ship analyzers inside their produced package,
continue to reference the analyzers via the `AnalyzerProject` item.
ViktorHofer added a commit that referenced this issue May 12, 2022
* Define convention to include analyzers in ref pack

Fixes #61321

Until now we required source libraries to define ProjectReferences when
an analyzer should be part of the shared framework. That strategy causes
analyzer projects to leak into the ProjectReference closure and by that
into a solution file.

As an example:
When another library references the source library that references the
analyzer, the analyzer is part of the dependency closure even though it
might not be required.

This change makes it possible to define the shared framework analyzer
projects in the NetCoreAppLibrary.props file for both the .NETCoreApp,
and the AspNetCoreApp shared framework.

Out-of-band projects which ship analyzers inside their produced package,
continue to reference the analyzers via the `AnalyzerProject` item.

* Use AnalyzerReference consistently

* Don't reference analyzer when its packaged

* Fix P2P reference

* Fix multi target roslyn component target condition
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants