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

Clean-up generators.targets #82304

Closed
wants to merge 2 commits into from
Closed

Clean-up generators.targets #82304

wants to merge 2 commits into from

Conversation

ViktorHofer
Copy link
Member

From what I can see, the two targets in generators.targets aren't used anymore. The LibraryImportGenerator_UseMarshalType setting was already defined in LibraryImportGenerator.props. Also, remove that props file and inline the values into generators.targets.

From what I can see, the two targets in generators.targets aren't used anymore. The `LibraryImportGenerator_UseMarshalType` setting was already defined in LibraryImportGenerator.props. Also, remove that props file and inline the values into generators.targets.
@ghost
Copy link

ghost commented Feb 17, 2023

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

Issue Details

From what I can see, the two targets in generators.targets aren't used anymore. The LibraryImportGenerator_UseMarshalType setting was already defined in LibraryImportGenerator.props. Also, remove that props file and inline the values into generators.targets.

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure

Milestone: -

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the props file so we have a close relationship between which options the LibraryImportGenerator supports and the MSBuild infrastructure to expose the options. Can we add a comment around the CompilerVisibleProperty items saying to keep in sync with the OptionsHelper.cs file in LibraryImportGenerator?

@elinor-fung
Copy link
Member

Also, remove that props file and inline the values into generators.targets.

Is that the general best practice for libraries generators? I find it nice to have the separate props right next to the generator itself. That way it is easy to see what settings there are for the generator.

@ViktorHofer
Copy link
Member Author

A props file adds value when it is imported by another props file, i.e. Directory.Build.props. It's benefit is questionable if a props file is imported by targets file. That said, I'm fine with keeping it if that's desirable.

@jkoritzinsky
Copy link
Member

We can create a generators.props file that just includes the props files for generator options if that makes it more worthwhile.

@ViktorHofer
Copy link
Member Author

Closing as this doesn't seem necessary.

@ViktorHofer ViktorHofer closed this Mar 9, 2023
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-1 branch March 9, 2023 14:38
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants