Fix VS complaining about missing projects in slns (continued) #68543

ViktorHofer · 2022-04-26T10:23:39Z

Contributes to #65552
Continuation of #68488

This PR updates the remaining out-of-band projects to fix the broken Visual Studio experience by making sure that the projects correctly build on-top of the targeting pack.

The changes are grouped by commits:

Infra clean-up to simplify auto references and general clean-up in generators.targets
Ref project file updates
Src project file updates
Test project file updates (2 projects were unnecessarily manually referencing targeting pack assemblies)

Remaining changes that will be submitted via separate pull requests:

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.
Update all solution files to finally fix Visual Studio restore is broken as projects are missing from the dependency closure #65552

cc @ericstj (just FYI)

ghost · 2022-04-26T10:23:47Z

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

Issue Details

Contributes to #65552
Continuation of #68488

This PR updates the remaining out-of-band projects that need to be updated to fix the broken Visual Studio experience by making sure that the projects correctly build on-top of the targeting pack.

The changes are grouped by commits:

Infra clean-up to simplify auto references and general clean-up in generators.targets
Ref project file updates
Src project file updates
Test project file updates (2 projects were unnecessarily manually referencing targeting pack assemblies)

Remaining changes that will be submitted via separate pull requests are:

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 build faster.
Update all solution files to finally fix Visual Studio restore is broken as projects are missing from the dependency closure #65552

cc @ericstj (just FYI)

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`area-Infrastructure`
Milestone:	-

eerhardt · 2022-04-26T14:26:57Z

eng/targetingpacks.targets

-          Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
-    <!-- Point MicrosoftNetCoreAppRefPackRefDir to the acquired targeting pack to use it later for AssemblySearchPaths resolution. -->
-    <PropertyGroup Condition="'$(TargetFrameworkVersion)' != 'v$(NetCoreAppCurrentVersion)'">
-      <MicrosoftNetCoreAppRefPackRefDir>%(ResolvedFrameworkReference.TargetingPackPath)\ref\net$(TargetFrameworkVersion.TrimStart('v'))\</MicrosoftNetCoreAppRefPackRefDir>


How does this work now without this?

That property was previously set for older .NETCoreApp tfms but that isn't necessary anymore as these now just use the targeting pack's references.

But what if an OOB that targets say net6.0 references another OOB that also targets net6.0? Wouldn't you want to build against the live version of it?

In such cases you use ProjectReferences which is the current behavior in main already. Conflict resolution (an SDK task) makes sure that ProjectReferences are preferred over targeting pack assets.

You only use <Reference /> items for NetCoreAppCurrent to reference inbox assets. Everything else is accomplished via ProjectReferences. Eventually the inbox references will be converted to ProjectReferences as well but that's not possible yet without regressing the local innerloop perf.

src/libraries/System.Threading.Tasks.Dataflow/src/System.Threading.Tasks.Dataflow.csproj

eerhardt · 2022-04-26T14:30:50Z

src/libraries/System.Threading.RateLimiting/src/System.Threading.RateLimiting.csproj

  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
    <PackageReference Include="System.Threading.Tasks.Extensions" Version="$(SystemThreadingTasksExtensionsVersion)" />
-  </ItemGroup>
-  <ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netstandard2.1'))">


Why is this being removed? I would think we only want the dependency on netstandard2.0 and netfx.

The project doesn't target netstandard2.1, therefore this condition can be merged with the "TFI != .NetCoreApp" condition above.

src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.csproj

...aries/System.Reflection.MetadataLoadContext/src/System.Reflection.MetadataLoadContext.csproj

src/libraries/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj

…fer/runtime into AutoReferencesContinued

joperezr · 2022-04-26T18:44:12Z

eng/generators.targets

@@ -16,17 +16,12 @@
                        and '$(IsSourceProject)' == 'true'
                        and '$(MSBuildProjectExtension)' == '.csproj'
                        and (
-                          ('@(Reference)' != ''
+                          '$(TargetFrameworkMoniker)' != '$(NetCoreAppCurrentTargetFrameworkMoniker)'
+                          or '$(DisableImplicitFrameworkReferences)' != 'true'


NIT: can we please update the comment above to explain the condition in more detail given it is a complex one?

We can simplify these conditions vastly and I originally had that change in this PR but then moved it out as it's unrelated to my change. I will submit a follow-up PR that does the clean-up and update the comment.

joperezr · 2022-04-26T18:52:14Z

src/libraries/System.ComponentModel.Composition/ref/System.ComponentModel.Composition.csproj

  <ItemGroup>
    <Compile Include="System.ComponentModel.Composition.cs" />
    <Compile Include="System.ComponentModel.Composition.Forwards.cs" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
-    <ProjectReference Include="$(LibrariesProjectRoot)System.Linq.Expressions\ref\System.Linq.Expressions.csproj" />


If we are removing P2Ps from netcoreappcurrent build, how are we supposed to consume the live build of S.L.Expressions?

The targeting pack that is resolved is the actual live built one (for NetCoreAppCurrent) which resides under artifacts\bin\microsoft.netcore.app.ref\.

This is already the current behavior in main. Let me explain it in more details:

The shared framework libraries (both ref and src) get built as part of the libs.sfx subset.

These projects binplace their built assets into the live targeting pack and runtime pack folders.

NetCoreAppCurrent out-of-band libraries build on top of the live targeting pack and/or runtime pack and receive that pack's references.

ProjectReference items are used to reference live built assets that aren't part of the targeting pack (OOBs) or when the library is available in the targeting pack but ships via a package as well, i.e. System.Text.Json. In the case of such special libraries like STJ, we reference them via ProjectReferences so that the library has a package dependency on STJ.

ViktorHofer · 2022-04-26T21:41:22Z

Logging off now. Would be fantastic to get an approval so that I can continue my work tomorrow, on top of these changes. Thanks everyone.

bartonjs · 2022-04-26T22:22:08Z

src/libraries/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj

  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
    <Compile Include="Polyfills\System.Numerics.BitOperations.netstandard20.cs" />
    <Compile Include="Polyfills\System.Text.Rune.netstandard20.cs" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
+
+  <ItemGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', '$(NetCoreAppCurrent)'))">


Is there a way that it'd be compatible with NetCoreAppCurrent without actually being NetCoreAppCurrent? It feels like the compare-to-current is usually an ==...

@eerhardt was asking the same question previously. Today these are the possible ways to condition on NetCoreAppCurrent (both platform agnostic and platform specific) that I know of:

'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(TargetFrameworkVersion)' == 'v$(NetCoreAppCurrentVersion)'

'$(TargetFrameworkVersion)' == 'v$(NetCoreAppCurrentVersion)'

'$(TargetFrameworkMoniker)' == '$(NetCoreAppCurrentTargetFrameworkMoniker)'

$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', '$(NetCoreAppCurrent)')) <-- that one works on properties as well

$([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '$(NETCoreAppCurrentVersion)'))

I chose the TargetFrameworkCompatible function as it works on both properties and items. In the two projects that I updated to use that function, a simple equality check couldn't be performed as the TargetFramework property doesn't evaluate to net7.0 but to net7.0-platform, i.e. in the case of System.Text.Encodings.Web net7.0-Browser.

eerhardt · 2022-04-27T15:09:07Z

src/libraries/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj

  <ItemGroup>
    <ProjectReference Include="$(LibrariesProjectRoot)System.Collections.Immutable\src\System.Collections.Immutable.csproj" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
+
+  <ItemGroup Condition="'$(TargetFrameworkVersion)' == 'v$(NetCoreAppCurrentVersion)'">


Can this be

'$(TargetFramework)' == '$(NetCoreAppCurrent)'

?

You are right. Let me update that line a follow-up PR.

eerhardt

One last minor comment. Other than that, LGTM.

ViktorHofer added 4 commits April 26, 2022 10:39

Infra clean-up to simplify auto references

4d04ab8

Ref project file updates

4df15ef

Src project file updates

07cdbe1

Test project file updates

46c99be

ViktorHofer added the area-Infrastructure label Apr 26, 2022

ViktorHofer requested a review from eerhardt April 26, 2022 10:23

ViktorHofer self-assigned this Apr 26, 2022

ViktorHofer added 3 commits April 26, 2022 13:17

Update targetingpacks.targets

4a0afd3

Update targetingpacks.targets

c7bc8dc

Update generators.targets

7e3e14a

eerhardt reviewed Apr 26, 2022

View reviewed changes

src/libraries/System.Threading.Tasks.Dataflow/src/System.Threading.Tasks.Dataflow.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Apr 26, 2022

View reviewed changes

src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Apr 26, 2022

View reviewed changes

...aries/System.Reflection.MetadataLoadContext/src/System.Reflection.MetadataLoadContext.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Apr 26, 2022

View reviewed changes

src/libraries/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj Show resolved Hide resolved

ericstj requested review from a team and joperezr April 26, 2022 16:56

ViktorHofer added 2 commits April 26, 2022 20:41

PR feedback

656a07b

Merge branch 'AutoReferencesContinued' of https://github.com/ViktorHo…

de1684c

…fer/runtime into AutoReferencesContinued

joperezr reviewed Apr 26, 2022

View reviewed changes

bartonjs reviewed Apr 26, 2022

View reviewed changes

eerhardt reviewed Apr 27, 2022

View reviewed changes

eerhardt approved these changes Apr 27, 2022

View reviewed changes

ViktorHofer merged commit b54561a into dotnet:main Apr 27, 2022

ViktorHofer deleted the AutoReferencesContinued branch April 27, 2022 15:28

ViktorHofer mentioned this pull request Apr 27, 2022

Minimize the build graph by deleting unnecessary references #68603

Merged

ViktorHofer mentioned this pull request Apr 28, 2022

Visual Studio restore is broken as projects are missing from the dependency closure #65552

Closed

ghost locked as resolved and limited conversation to collaborators May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VS complaining about missing projects in slns (continued) #68543

Fix VS complaining about missing projects in slns (continued) #68543

ViktorHofer commented Apr 26, 2022 •

edited

Loading

ghost commented Apr 26, 2022

eerhardt Apr 26, 2022

ViktorHofer Apr 26, 2022

joperezr Apr 26, 2022

ViktorHofer Apr 26, 2022 •

edited

Loading

eerhardt Apr 26, 2022

ViktorHofer Apr 26, 2022

joperezr Apr 26, 2022

ViktorHofer Apr 26, 2022 •

edited

Loading

joperezr Apr 26, 2022

ViktorHofer Apr 26, 2022 •

edited

Loading

ViktorHofer Apr 26, 2022 •

edited

Loading

ViktorHofer commented Apr 26, 2022

bartonjs Apr 26, 2022

ViktorHofer Apr 27, 2022

eerhardt Apr 27, 2022

ViktorHofer Apr 27, 2022

eerhardt left a comment

Fix VS complaining about missing projects in slns (continued) #68543

Fix VS complaining about missing projects in slns (continued) #68543

Conversation

ViktorHofer commented Apr 26, 2022 • edited Loading

The changes are grouped by commits:

Remaining changes that will be submitted via separate pull requests:

ghost commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

ViktorHofer Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

ViktorHofer commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

ViktorHofer commented Apr 26, 2022 •

edited

Loading

ViktorHofer Apr 26, 2022 •

edited

Loading

ViktorHofer Apr 26, 2022 •

edited

Loading

ViktorHofer Apr 26, 2022 •

edited

Loading

ViktorHofer Apr 26, 2022 •

edited

Loading