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

Minimize the build graph by deleting unnecessary references #68603

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

ViktorHofer
Copy link
Member

Contributes to #65552

Minimizing projects' dependency graph. There are tons of unnecessary
Reference and ProjectReference items which aren't required anymore as
the referenced projects became full façade assemblies. Removing those
from a leaf's graph makes the graph smaller and therefore the project's
evaluation and build faster.

Examples of full facade projects are:

  • System.Runtime.CompilerServices.Unsafe.csproj
  • System.Runtime.Extensions.csproj

The changes were performed via:

  • Find & Replace of:
    • <Reference Include="full-facade-assembly" />
    • <ProjectReference Include="full-facade-project" />
  • Manually checking if the compiled assembly actually requires the
    defined ProjectReference/Reference items and removing them if not.

Unrelated changes that I applied in the projects that I already touched:

  • In a handful of projects, adding missing primary references that were
    previously brought in transitively.
  • Adding empty lines between property and/or item groups to make
    project files more readable. Most projects in src/libraries already
    follow that style.
  • Moving a few Compile items into their own ItemGroup to improve
    readability of project files.

Minimizing projects' dependency graph. There are tons of unnecessary
Reference and ProjectReference items which aren't required anymore as
the referenced projects became full facade assemblies. Removing those
from a leaf's graph makes the graph smaller and therefore the project's
evaluation and build faster.
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Apr 27, 2022

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

Issue Details

Contributes to #65552

Minimizing projects' dependency graph. There are tons of unnecessary
Reference and ProjectReference items which aren't required anymore as
the referenced projects became full façade assemblies. Removing those
from a leaf's graph makes the graph smaller and therefore the project's
evaluation and build faster.

Examples of full facade projects are:

  • System.Runtime.CompilerServices.Unsafe.csproj
  • System.Runtime.Extensions.csproj

The changes were performed via:

  • Find & Replace of:
    • <Reference Include="full-facade-assembly" />
    • <ProjectReference Include="full-facade-project" />
  • Manually checking if the compiled assembly actually requires the
    defined ProjectReference/Reference items and removing them if not.

Unrelated changes that I applied in the projects that I already touched:

  • In a handful of projects, adding missing primary references that were
    previously brought in transitively.
  • Adding empty lines between property and/or item groups to make
    project files more readable. Most projects in src/libraries already
    follow that style.
  • Moving a few Compile items into their own ItemGroup to improve
    readability of project files.
Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

Comment on lines +38 to 40
<!-- error : CannotChangeAttribute : Attribute 'System.Diagnostics.SwitchLevelAttribute' on 'System.Diagnostics.BooleanSwitch' changed
from '[SwitchLevelAttribute(typeof(bool))]' in the implementation to '[SwitchLevelAttribute(typeof(Boolean))]' in the reference. -->
<Reference Include="System.IO.FileSystem" />
Copy link
Member Author

@ViktorHofer ViktorHofer Apr 27, 2022

Choose a reason for hiding this comment

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

@tannergooding do you think the above mentioned ApiCompat error will be fixed with your recent change to use primitive types always?

Copy link
Member

Choose a reason for hiding this comment

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

This one I'm not sure is impacted. The logic I changed was only impacting Boolean itself using bool vs Boolean (and same for other primitives).

I wouldn't have expected it to also impact attribute decisions like this one. So there may still be another GenApi fix needed to ensure typeof here is allowing the keyword where possible.

@@ -260,7 +260,7 @@ System.Reflection.PortableExecutable.ManagedPEBuilder</PackageDescription>
<ProjectReference Include="$(LibrariesProjectRoot)System.Collections.Immutable\src\System.Collections.Immutable.csproj" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkVersion)' == 'v$(NetCoreAppCurrentVersion)'">
<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
Copy link
Member Author

Choose a reason for hiding this comment

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

@eerhardt I'm applying your feedback here.

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Assuming it builds, LGTM.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Apr 27, 2022

Requesting feedback from a few folks as this is general clean-up / build optimization and not specific to infrastructure.

EDIT: As Jeremy already approved the PR I don't think additional reviews are needed (basically because this should be a non controversial change).

@ghost
Copy link

ghost commented Apr 27, 2022

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

Issue Details

Contributes to #65552

Minimizing projects' dependency graph. There are tons of unnecessary
Reference and ProjectReference items which aren't required anymore as
the referenced projects became full façade assemblies. Removing those
from a leaf's graph makes the graph smaller and therefore the project's
evaluation and build faster.

Examples of full facade projects are:

  • System.Runtime.CompilerServices.Unsafe.csproj
  • System.Runtime.Extensions.csproj

The changes were performed via:

  • Find & Replace of:
    • <Reference Include="full-facade-assembly" />
    • <ProjectReference Include="full-facade-project" />
  • Manually checking if the compiled assembly actually requires the
    defined ProjectReference/Reference items and removing them if not.

Unrelated changes that I applied in the projects that I already touched:

  • In a handful of projects, adding missing primary references that were
    previously brought in transitively.
  • Adding empty lines between property and/or item groups to make
    project files more readable. Most projects in src/libraries already
    follow that style.
  • Moving a few Compile items into their own ItemGroup to improve
    readability of project files.
Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Meta

Milestone: -

@ViktorHofer
Copy link
Member Author

Assuming it builds, LGTM.

@bartonjs that was fast 😄. And I agree that if the changes builds successfully, they should be correct.

@@ -1,5 +0,0 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

@GrabYourPitchforks sees "delete SecureString.cs" and starts to celebrate 🥳

@ViktorHofer ViktorHofer changed the title Minimize the build graph by deleting references Minimize the build graph by deleting unnecessary references Apr 27, 2022
@ViktorHofer ViktorHofer merged commit ba055bd into dotnet:main Apr 28, 2022
@ViktorHofer ViktorHofer deleted the DeleteUnnecessaryReferences branch April 28, 2022 13:04
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2022
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.

4 participants