Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jun 18, 2025

Revival of #106329

Review by commits:

Commit 1: Use P2Ps in inbox source projects
Commit 2: Additional changes due to PrivateAssets="all" for sfx source projects
Commit 3: Set PackageId for non-packable source projects to differentiate identity

Description

Use ProjectReference items in shared framework source projects instead of Reference. There are numerous benefits in using ProjectReferences consistently
in all libraries:

  1. An upfront "libs" build isn't required anymore and sfx libraries
    can now directly be built from a fresh clone (with dotnet build or inside
    VS). I.e. dotnet pack src/libraries/System.Text.Json/src/ now just works.
  2. Because of 1), we can now add a solution file for the whole sfx that
    can directly be opened and worked with from a fresh clone.
  3. Fully builds sfx projects in parallel now instead of the serial step
    (which first build all refs and then all srcs).
  4. A library's dependencies now incrementally get built together with the
    root library.
  5. Removing upfront steps to build libraries improves Copilot's ability
    to validate its proposed changes.
  6. Using P2Ps means that we now follow the common and well supported
    msbuild and SDK path instead of repo customization.

The downside of doing this is that the dependency graph gets bigger,
meaning that more projects get incrementally built when doing a "dotnet
build". This is nothing new and the SDK team recommends to pass the
"--no-dependencies" flag to "dotnet build" if incrementally (no-op)
building the additional dependency nodes is noticeable.
This is less of a concern inside VS as that has a "fast up-to-date check"
feature that doesn't even attempt to build projects that didn't change.
For VS, really the only noticeable change is that the solution explorer
now lists more projects and that when opening a solution, more projects
need to be evaluated. But, that should be fast enough when using an
up-to-date version of VS.

Measurements

I ran the following commands on my win-x64 machine. new refers to running with changes in this PR.

build.cmd libs -pack:
new - Time Elapsed 00:13:46.74
old - Time Elapsed 00:14:03.99

build.cmd libs:
new - Time Elapsed 00:07:23.85
old - Time Elapsed 00:06:54.11

The slight regression is expected as with P2Ps more work needs to be done. But that additional work directly translates into the benefits listed above.

Copilot AI review requested due to automatic review settings June 18, 2025 01:17
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 18, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates shared framework source projects to use ProjectReferences instead of assembly references. It also introduces changes to dependency resolution in various targets files and adjusts package identity for non-packable projects.

  • Converted all reference includes to ProjectReference includes in csproj files.
  • Updated build targets and generators files to support proper dependency resolution and package identity.
  • Revised VB and targets file configurations to align with the new dependency model.

Reviewed Changes

Copilot reviewed 104 out of 104 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj Replaced direct assembly references with ProjectReferences.
src/libraries/System.Data.Common/src/System.Data.Common.csproj Updated references to use ProjectReference and added additional properties for transitive behavior.
src/libraries/System.Console/src/System.Console.csproj Converted assembly references to ProjectReferences.
src/libraries/System.ComponentModel/*.csproj Changed assembly references to ProjectReferences.
src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj Updated multiple references to ProjectReferences.
src/libraries/Microsoft.VisualBasic.Core/src/Microsoft.VisualBasic.Core.vbproj Reworked VB project references and updated properties.
src/libraries/Microsoft.CSharp/src/Microsoft.CSharp.csproj Replaced Reflection.Emit and collections references with ProjectReferences.
src/libraries/Directory.Build.targets Modified condition for implicit framework reference disabling and added PackageId suffix for non-packable projects.
eng/targetingpacks.targets, eng/resolveContract.targets, eng/references.targets, eng/generators.targets Adjusted targets logic for referencing and dependency resolution.
docs/coding-guidelines/project-guidelines.md Updated guidelines to reflect the new ProjectReference usage.
Comments suppressed due to low confidence (2)

eng/generators.targets:28

  • Verify that using the 'Filename' metadata for filtering ProjectReferences is reliable across all projects, ensuring that every ProjectReference has this metadata populated as expected.
                                      '@(ProjectReference->AnyHaveMetadataValue('Filename', 'System.Runtime.InteropServices'))' == 'true' or

src/libraries/Directory.Build.targets:62

  • Confirm that the string equality check for the TargetFrameworkVersion (with a 'v' prefix) consistently matches the expected format, to avoid potential mismatches in target framework evaluations.
                                                   '$(TargetFrameworkVersion)' == 'v$(NetCoreAppCurrentVersion)' and

@teo-tsirpanis teo-tsirpanis added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 28, 2025
@dotnet-policy-service
Copy link
Contributor

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

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.

Looks good. As we discussed you've done validation by diffing the output and nuspec content to ensure we didn't change the product in a meaningful way. You also mentioned doing a VMR build to ensure we don't break this (additionally giving some coverage of usage of the built product).

@ViktorHofer
Copy link
Member Author

Failures are unrelated and Alex told me already being worked on. Merging.

@ViktorHofer ViktorHofer merged commit 910f619 into dotnet:main Jul 9, 2025
153 of 159 checks passed
@ViktorHofer ViktorHofer deleted the P2PRevival branch July 9, 2025 21:46
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2025
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