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

Simplify target to enable config binding src generator from NuGet package, ensure it runs for all TFMs, and rename enabling property #84379

Merged
merged 5 commits into from
Apr 9, 2023

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Apr 5, 2023

@layomia layomia added this to the 8.0.0 milestone Apr 5, 2023
@layomia layomia self-assigned this Apr 5, 2023
@layomia layomia marked this pull request as ready for review April 5, 2023 20:39
@ghost
Copy link

ghost commented Apr 5, 2023

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

Issue Details
Author: layomia
Assignees: layomia
Labels:

area-Extensions-Configuration

Milestone: 8.0.0

@layomia layomia force-pushed the binder-gen-packaging-target branch from 923793c to 98917de Compare April 5, 2023 21:14
@layomia layomia force-pushed the binder-gen-packaging-target branch from 98917de to 1994914 Compare April 5, 2023 21:15
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM - just a non-blocking nit.

</PropertyGroup>

<ItemGroup>
<Content Include="buildTransitive\$(MSBuildProjectName).*"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Content Include="buildTransitive\$(MSBuildProjectName).*"
<Content Include="buildTransitive\$(PackageId).targets"

nit: I tend to avoid a wildcard unless it's needed since it hurts evaluation perf. nit: $(PackageId) should be used rather than $(MSBuildProjectName) since they can differ, but the PackageID is the one that needs to match.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could update the docs (and the other places that do this as well) to be consistent across the repo?

```xml
<ItemGroup>
<Content Include="buildTransitive\$(MSBuildProjectName).*"
PackagePath="buildTransitive\netstandard2.0\;
buildTransitive\$(NetFrameworkMinimum)\;
buildTransitive\$(NetCoreAppMinimum)\" />
</ItemGroup>
```

I believe the * is there to support both .props and .targets. But @ViktorHofer would know for sure.

@layomia
Copy link
Contributor Author

layomia commented Apr 7, 2023

The AddNETStandardCompatErrorFileForPackaging target infra to emit a warning for unsupported package-consuming TFMs causes the analyzer removal target to get dropped for unsupported TFMs (e.g. net461 and netcoreapp3.1)

<Target Name="AddNETStandardCompatErrorFileForPackaging"

This is undesirable and can lead to undefined behavior. The latest commit 9eab1ce fixes this issue, enforcing that the removal target runs for all TFMs & also emitting the compat warning for unsupported TFMs. This fix is hardcoded for now. We need to also fix it for the other source-generator targets for multi-targeting and disabling, and come up with the right templating across all the targets & packages. cc @ViktorHofer

@layomia layomia changed the title Simplify target to enable config binding src generator and rename enabling property Simplify target to enable config binding src generator from NuGet package, ensure it runs for all TFMs, and rename enabling property Apr 9, 2023
@layomia layomia merged commit e039add into dotnet:main Apr 9, 2023
</Target>

<Target Name="NETStandardCompatError_Microsoft_Extensions_Configuration_Binder"
Condition="'$(SuppressTfmSupportBuildWarnings)' == ''">
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to '$(SuppressTfmSupportBuildWarnings)' != 'true'. While at it, can you please also change that here:

Condition="'%24(SuppressTfmSupportBuildWarnings)' == ''">

</ItemGroup>
</Target>

<Target Name="NETStandardCompatError_Microsoft_Extensions_Configuration_Binder"
Copy link
Member

Choose a reason for hiding this comment

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

Please file a tracking issue to discuss how to support both the NETStandardCompatError infrastructure and target framework agnostic msbuild infrastructure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you go: #84570.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

LGTM aside from two comments.

@layomia layomia added the source-generator Indicates an issue with a source generator feature label May 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants