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

Ingest and import Windows Forms analyzer props #4605

Closed
wants to merge 2 commits into from

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Jun 1, 2021

Description

This work relates to Streamline Windows Forms application configuration and bootstrap design proposal, and acts as a workaround until Proposal to add Framework specific inbox source generators design proposal is approved and implemented.

Provide a mechanism to ingest Windows Forms specific analyzer props into Microsoft.NET.Sdk.WindowsDesktop from Windows Forms transport package, and copy the props file into targets folder of the SDK, so that the props file is resolved and imported when a developer builds a Windows Forms app.

In the end the new props file will end up in a location similar to this: C:\Program Files\dotnet\sdk\6.0.<version>\Sdks\Microsoft.NET.Sdk.WindowsDesktop\targets\.

The build process looks something like this:

  • import props and targets
    image
    image
  • execute the task that resolves and references Windows Forms analyzers:
    image

Testing

A lot of manual testing

  1. Create a Windows Forms transport package, e.g.: winforms> .\build.cmd -pack
  2. Copy the build transport package from .\winforms\artifacts\packages\Debug\NonShipping over the currently referenced by WPF microsoft.private.winforms package in C:\Users\<user>\.nuget\packages\microsoft.private.winforms
  3. Build and pack WPF: wpf> .\build -pack
  4. Inspect .\wpf\artifacts\packaging\Debug\Microsoft.NET.Sdk.WindowsDesktop.Debug\targets folder and see System.Windows.Forms.Analyzers.props and System.Windows.Forms.Analyzers.targets present

@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 1, 2021
@ghost ghost requested review from fabiant3 and SamBent June 1, 2021 02:49
@RussKie RussKie force-pushed the dev/igveliko/winforms_analyzer_props branch from 6d46dd9 to 443a26e Compare June 2, 2021 11:37
RussKie added a commit to dotnet/windowsdesktop that referenced this pull request Jun 2, 2021
Provide a mechanism to import Windows Forms specific analyzers from
Windows Forms transport package, and and package those into
Microsoft.WindowsDesktop.App.Ref bundle, so that they are available in
Windows Forms app out of the box.

The analyzers are referenced via props and targets located in
`C:\Program Files\dotnet\sdk\6.0.<version>\Sdks\Microsoft.NET.Sdk.WindowsDesktop\targets\`,
which is facilitated by dotnet/wpf#4605.

This work relates to dotnet/designs#223, and acts
as a workaround until dotnet/designs#181 is
approved and implemented.
</ItemGroup>

<Target Name="_EnsureWindowsFormsPackagingContent" BeforeTargets="IdentifyPackageAssets">
Copy link
Member

@vatsan-madhavan vatsan-madhavan Jun 2, 2021

Choose a reason for hiding this comment

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

This is an antipattern. Foo.*ArchNeutral.csproj, Foo.csproj etc. have certain conventions, and they weren't intended to be spots for additional domain-specific targets. Our domain-specific targets - in fact all of this repos' build-related intelligence - lives under a certain hierarchy under eng\ that I've already alluded to. There are rare well-considered exceptions to this rule - and this does [edit] not [/edit] strike me as one.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't read the docs you've shared, but my sense/guess here is that analyzer related props/targets is a new category of payload that's being introduced. If this is right... then consider introducing a new Item name, something like AnalyzerPackagingContent that's used to track these items. These would become agnostic of WPF/Winforms etc. far as build-logic goes.

Then in Packaging.targets, add logic specific to validation of these. For e.g., anything that's been added to AnalyzerPackagingContent - ensure that they exist, make sure they're added to PackagingContent etc.. Then all you'd do in *.WindowsDesktop.ArchNeutral.csproj is adding your props/targets to the newly introduced AnalyzerPackagingContent Item and let the backend targets do its work as before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this target purely to perform verifcations as you suggested earlier.
All work is done by these targets https://github.com/RussKie/winforms/blob/project_configuration/src/System.Windows.Forms.Analyzers/src/System.Windows.Forms.Analyzers.targets

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I added this target in this project and not in Directory.Build.targets at other levels is these validations is only relevant to this project. I'm happy to move it out to a separate targets file under eng\ folder and reference it in this project.

Copy link
Member

Choose a reason for hiding this comment

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

A different architecture is fine, but this project is not the right place for adding targets, imports etc. - if you need to extend or hook the build system, it has to be done elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does 9c0757e look better?

Provide a mechanism to ingest Windows Forms specific analyzer props and
targets into Microsoft.NET.Sdk.WindowsDesktop from Windows Forms transport
package, and copy these files into `targets` folder of the SDK, so that
the props files are resolved and imported when a developer builds a Windows
Forms app.

In the end the new props file will end up in a location similar to this:
`C:\Program Files\dotnet\sdk\6.0.<version>\Sdks\Microsoft.NET.Sdk.WindowsDesktop\targets\`.

This work relates to dotnet/designs#223, and acts
as a workaround until dotnet/designs#181 is
approved and implemented.
@RussKie RussKie force-pushed the dev/igveliko/winforms_analyzer_props branch from 3d07448 to 9c0757e Compare June 7, 2021 10:56
@RussKie
Copy link
Member Author

RussKie commented Jun 8, 2021

Tested using an experimental pipeline:
image

@RussKie RussKie closed this Jul 29, 2021
@RussKie RussKie deleted the dev/igveliko/winforms_analyzer_props branch July 29, 2021 04:58
@ghost ghost added the draft label Apr 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
draft PR metadata: Label to tag PRs, to facilitate with triage WindowsDesktop SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants