Skip to content

Use ProjectReferences in libs shared framework source projects #116772

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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 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 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant