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.

Sign up for GitHub

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

Jump to bottom

Update generators.targets #68860

Merged
merged 4 commits into from
May 10, 2022
Merged

Update generators.targets #68860

merged 4 commits into from
May 10, 2022

Conversation

ViktorHofer
Copy link
Member

Changes:

  • Improve perf of LibraryImportGenerator condition and simplify it
  • Use the same msbuild code styling as in other projects in src/libraries
  • Update comments that were outdated
  • Remove the item conditions for the regex source generator which doesn't
    work well with the CPS (common project system) inside VS.
  • Remove unnecessary property

@ViktorHofer
Update generators.targets
f3ff725 
Changes:
- Improve perf of LibraryImportGenerator condition and simplify it
- Use the same msbuild code styling as in other projects in src/libraries
- Update comments that were outdated
- Remove the item conditions for the regex source generator which doesn't
  work well with the CPS (common project system) inside VS.
- Remove unnecessary property
@ViktorHofer ViktorHofer self-assigned this May 4, 2022
@ghost
Copy link

ghost commented May 4, 2022

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

Issue Details

Changes:

  • Improve perf of LibraryImportGenerator condition and simplify it
  • Use the same msbuild code styling as in other projects in src/libraries
  • Update comments that were outdated
  • Remove the item conditions for the regex source generator which doesn't
    work well with the CPS (common project system) inside VS.
  • Remove unnecessary property
Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries
Milestone: -

@ViktorHofer ViktorHofer requested review from a team May 5, 2022 15:53
@ViktorHofer
Copy link
Member Author

@carlossanlop @hoyosjs @joperezr can you please take a look? It's a trivial change that should not add any risk to the product.

bartonjs
bartonjs reviewed May 6, 2022
View reviewed changes
eng/generators.targets Outdated Show resolved Hide resolved
bartonjs
bartonjs reviewed May 6, 2022
View reviewed changes
eng/generators.targets Outdated Show resolved Hide resolved
bartonjs
bartonjs reviewed May 6, 2022
View reviewed changes
eng/generators.targets Outdated Show resolved Hide resolved
bartonjs
bartonjs reviewed May 6, 2022
View reviewed changes
eng/generators.targets
Comment on lines +44 to +45
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime.InteropServices\gen\LibraryImportGenerator\LibraryImportGenerator.csproj;
$(LibrariesProjectRoot)System.Runtime.InteropServices\gen\Microsoft.Interop.SourceGeneration\Microsoft.Interop.SourceGeneration.csproj"
Copy link
Member

Choose a reason for hiding this comment

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

Again, I'd (personally) prefer seeing this as two entries, since two things are being added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above. When you define items with the same metadata (i.e. Condition or in this case the OutputItemType and ReferenceOutputAssembly metadata), grouping them together makes more sense IMO.

@ViktorHofer @bartonjs
Update eng/generators.targets
0aec467 
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
joperezr
joperezr reviewed May 9, 2022
View reviewed changes
eng/generators.targets
OutputItemType="Analyzer"
ReferenceOutputAssembly="false" />
<ItemGroup Condition="'$(EnableRegexGenerator)' == 'true'">
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.RegularExpressions\gen\System.Text.RegularExpressions.Generator.csproj"
Copy link
Member

Choose a reason for hiding this comment

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

Don't we always try to use Unix-style folder separators everywhere?

Copy link
Member Author

@ViktorHofer ViktorHofer May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's the opposite. Under src/libraries we prefer Windows path separators.

  • runtime/src/libraries/System.Console/ref/System.Console.csproj

    Line 9 in 0533792

    <ProjectReference Include="..\..\System.Runtime\ref\System.Runtime.csproj" />

  • runtime/src/libraries/Microsoft.Extensions.Http/src/Microsoft.Extensions.Http.csproj

    Lines 29 to 32 in 0533792

    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection.Abstractions\src\Microsoft.Extensions.DependencyInjection.Abstractions.csproj" />
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Logging\src\Microsoft.Extensions.Logging.csproj" />
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Logging.Abstractions\src\Microsoft.Extensions.Logging.Abstractions.csproj" />
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Options\src\Microsoft.Extensions.Options.csproj" />

  • runtime/src/libraries/System.IO.Ports/src/System.IO.Ports.csproj

    Lines 23 to 37 in 0533792

    <Compile Include="System\IO\Ports\Handshake.cs" />
    <Compile Include="System\IO\Ports\InternalResources.cs" />
    <Compile Include="System\IO\Ports\Parity.cs" />
    <Compile Include="System\IO\Ports\SerialData.cs" />
    <Compile Include="System\IO\Ports\SerialDataReceivedEventArgs.cs" />
    <Compile Include="System\IO\Ports\SerialDataReceivedEventHandler.cs" />
    <Compile Include="System\IO\Ports\SerialError.cs" />
    <Compile Include="System\IO\Ports\SerialErrorReceivedEventArgs.cs" />
    <Compile Include="System\IO\Ports\SerialErrorReceivedEventHandler.cs" />
    <Compile Include="System\IO\Ports\SerialPinChange.cs" />
    <Compile Include="System\IO\Ports\SerialPinChangedEventArgs.cs" />
    <Compile Include="System\IO\Ports\SerialPinChangedEventHandler.cs" />
    <Compile Include="System\IO\Ports\SerialPort.cs" />
    <Compile Include="System\IO\Ports\SerialStream.cs" />
    <Compile Include="System\IO\Ports\StopBits.cs" />

jkoritzinsky
jkoritzinsky approved these changes May 10, 2022
View reviewed changes
eng/generators.targets
Comment on lines +16 to +30
Condition="'$(EnableLibraryImportGenerator)' == '' and
'$(IsSourceProject)' == 'true' and
'$(MSBuildProjectExtension)' == '.csproj' and
(
'$(TargetFrameworkMoniker)' != '$(NetCoreAppCurrentTargetFrameworkMoniker)' or
'$(DisableImplicitFrameworkReferences)' != 'true' or
(
'@(Reference)' != '' and
@(Reference->AnyHaveMetadataValue('Identity', 'System.Runtime.InteropServices'))
) or
(
'@(ProjectReference)' != '' and
@(ProjectReference->AnyHaveMetadataValue('Identity', '$(CoreLibProject)'))
)
)" />
Copy link
Member

Choose a reason for hiding this comment

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

I think we can probably simplify this more now that we aren't injecting as much source as before (only enums and an attribute type on downlevel). This can be left for a future PR though.

@ViktorHofer ViktorHofer merged commit 96db556 into main May 10, 2022
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-1 branch May 10, 2022 20:38
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joperezr joperezr joperezr left review comments

@bartonjs bartonjs bartonjs left review comments

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

Assignees

@ViktorHofer ViktorHofer

Labels
area-Infrastructure-libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ViktorHofer @jkoritzinsky @bartonjs @joperezr