Improve project reference consistency by dougbu · Pull Request #37338 · dotnet/aspnetcore

dougbu · 2021-10-06T23:38:35Z

Improve project reference consistency

use Private and SkipGetTargetFrameworkProperties w/ ReferenceOutputAssemblies="false"
- correct meaningless $(ReferenceOutputAssemblies) settings; no such metadata
- Private="false" avoids unnecessary file copies
- SkipGetTargetFrameworkProperties="true" should reduce useless TFM negotiation
  - that said, it can't be used everywhere due to multi-targeting restrictions
do not use PrivateAssets="All" in @(ProjectReference) items or `@(Reference) items that become them
- has no meaning in @(ProjectReference) items
add warnings about mixed-up application of %(Private) and %(PrivateAssets)

Remove most UndefineProperties use

should not be necessary

- use `Private` and `SkipGetTargetFrameworkProperties` w/ `ReferenceOutputAssemblies="false"` - correct meaningless `$(ReferenceOutputAssemblies)` settings; no such metadata - `Private="false"` avoids unnecessary file copies - `SkipGetTargetFrameworkProperties="true"` should reduce useless TFM negotiation - that said, it can't be used everywhere due to multi-targeting restrictions - do not use `PrivateAssets="All"` in `@(ProjectReference)` items or `@(Reference) items that become them - has no meaning in `@(ProjectReference)` items - add warnings about mixed-up application of `%(Private)` and `%(PrivateAssets)` nit: make references _look_ consistent too e.g. use consistent attribute order and wrap lines

- should not be necessary

dougbu · 2021-10-06T23:46:43Z

This may help w/ #32219 though consistency doesn't hurt in general 😄

@rainersigwald I'd appreciate your thoughts on the changes here and potential unintended consequences. For one thing, does %(UndefineProperties) metadata on our @(Reference) items that turn into @(ProjectReference) items and "regular" @(ProjectReference) items work at all and potentially need to be restored in some of these cases❔

dougbu · 2021-10-06T23:47:47Z

At least we got the spelling right, even if the metadata was ignored 😁

dougbu · 2021-10-06T23:51:11Z

ReferenceOutputAssembly="false" tells msbuild and NuGet not to include the reference in a generated .nuspec i.e. the PrivateAssets="All" intention was already in the references where I removed PrivateAssets="All"

dougbu · 2021-10-06T23:57:11Z

It's a nit but I'm tempted to make Microsoft.AspNetCore.App.Ref and Microsoft.AspNetCore.App.Runtime project reference providers and add errors when they're @(Reference)d w/o ReferenceOutputAssembly="false". That would get rid of some of these non-@(Reference) items but may require another special case or two. Thoughts❔

dougbu · 2021-10-07T00:00:19Z

Similarly, could make the .npmproj projects project reference providers for the same consistency reason as App.Ref and App.Runtime. Would be slightly less useful because these references are a bit less common and don't need additional special cases or warnings / errors.

- remove `SkipGetTargetFrameworkProperties="true"` from App.Ref reference to App.CodeFixes - need TFM negotiation when App.Ref builds on at least some platforms - firm up `net7.0` ➡️ `netstandard2.0` transition - fix typo in Rpm.TargetingPack.rpmproj - align Microsoft.AspNetCore.AzureAppServices.SiteExtension.csproj w/ Microsoft.AspNetCore.Runtime.SiteExtension.pkgproj - use same Microsoft.Web.Xdt.Extensions reference approach nit: remove extra Microsoft.AspNetCore.App.Analyzers references; brought in by App.CodeFixes

dougbu · 2021-10-07T17:03:16Z

For one thing, does %(UndefineProperties) metadata on our @(Reference) items that turn into @(ProjectReference) items and "regular" @(ProjectReference) items work at all and potentially need to be restored in some of these cases❔

For another, is %(ProjectReference.Private) metadata ignored when %(ReferenceOutputAssembly) is false❔

dougbu

We normally use SkipGetTargetFrameworkProperties="true" only when referring to .npmproj from .csproj projects, to .csproj from .debproj or .rpmproj projects, or between .csproj projects with identical (and single) TFMs. Highlighting the few exceptions…

Do any of these raise red flags for you @rainersigwald❔

dougbu · 2021-10-07T18:20:37Z

+    <Reference Include="Microsoft.AspNetCore.Mvc.Testing.Tasks"
+      Private="false"
+      ReferenceOutputAssembly="false"
+      SkipGetTargetFrameworkProperties="true" />


TFM here is net7.0 while that in Microsoft.AspNetCore.Mvc.Testing.Tasks is netstandard2.0

dougbu · 2021-10-07T18:44:04Z

    <ProjectReference Include="..\..\Framework\App.Ref\src\Microsoft.AspNetCore.App.Ref.csproj"
-        Condition=" $(IsTargetingPackBuilding) ">
-      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
-      <SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties>
-    </ProjectReference>
+      Condition=" $(IsTargetingPackBuilding) "
+      Private="false"
+      ReferenceOutputAssembly="false"
+      SkipGetTargetFrameworkProperties="true" />
    <ProjectReference Include="..\..\Framework\App.Runtime\src\Microsoft.AspNetCore.App.Runtime.csproj"
-        Condition=" !$(IsTargetingPackBuilding) ">
-      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
-      <SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties>
-    </ProjectReference>
+      Condition=" !$(IsTargetingPackBuilding) "
+      Private="false"
+      ReferenceOutputAssembly="false"
+      SkipGetTargetFrameworkProperties="true" />
  </ItemGroup>


TFM here is net462 while that in the App projects is net7.0

dougbu · 2021-10-07T18:45:33Z

-    <ProjectReference Include="..\..\Framework\App.Runtime\src\Microsoft.AspNetCore.App.Runtime.csproj">
-      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>
-      <SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties>
-    </ProjectReference>
+    <ProjectReference Include="..\..\Framework\App.Runtime\src\Microsoft.AspNetCore.App.Runtime.csproj"
+      Private="false"
+      ReferenceOutputAssembly="false"
+      SkipGetTargetFrameworkProperties="true" />


TFM here is net462 while that in the App.Runtime project is net7.0

dougbu · 2021-10-07T19:56:34Z

+    <Reference Include="GetDocument.Insider"
+      Private="false"
+      ReferenceOutputAssembly="false"
+      SkipGetTargetFrameworkProperties="true">
+        <Targets Condition=" '$(TargetFramework)' == 'netcoreapp2.1' ">Publish</Targets>


This project and GetDocument.Insider both multi-target but their TFM sets are identical. But, this is the only case where this PR adds SkipGetTargetFrameworkProperties="true" to an "unusual" project and it seems to be working fine.

rainersigwald · 2021-10-07T20:50:50Z

For one thing, does %(UndefineProperties) metadata on our @(Reference) items that turn into @(ProjectReference) items and "regular" @(ProjectReference) items work at all and potentially need to be restored in some of these cases❔

UndefineProperties is one of the metadata that is handled natively in the MSBuild task. In this case I think they were useful, preventing inner-build TFs from propagating to the references. For instance in one of the binlogs from this PR build I see an .npmproj building with TargetFramework=net7.0. Might be best to leave it anywhere you have SkipGetTargetFrameworkProperties="true" ReferenceOutputAssembly="false".

dougbu · 2021-10-07T21:00:30Z

UndefineProperties is one of the metadata that is [handled natively in the MSBuild task][native_task].

Alright I'll revert some or all of my second commit. To confirm my understanding, "handled natively" also means <MSBuild /> picks up and uses %(UndefineProperties) metadata from @(ProjectReference) items, @(_MSBuildProjectReferenceExistent) items, and similar❔

- keep using this metadata when referring to .npmproj projects

dougbu · 2021-10-07T22:12:00Z

Unless I hear the following cases also need %(UndefineProperties) restored, I'm leaving those bits from 6acca56 alone

M       src/Framework/App.Ref/src/Microsoft.AspNetCore.App.Ref.csproj
M       src/Mvc/Mvc.Testing/src/Microsoft.AspNetCore.Mvc.Testing.csproj
M       src/Tools/Extensions.ApiDescription.Server/src/Microsoft.Extensions.ApiDescription.Server.csproj

dougbu · 2021-10-07T23:46:39Z

/ping reviewers

Bringing general questions to the top

I'm tempted to make Microsoft.AspNetCore.App.Ref and Microsoft.AspNetCore.App.Runtime project reference providers and add errors when they're @(Reference)d w/o ReferenceOutputAssembly="false". That would get rid of some of these non-@(Reference) items but may require another special case or two. Thoughts❔
Similarly, could make the .npmproj projects project reference providers for the same consistency reason as App.Ref and App.Runtime. Would be slightly less useful because these references are a bit less common and don't need additional special cases or warnings / errors.

dougbu · 2021-10-07T23:54:27Z

And, sorry, @rainersigwald but I had a couple more questions for you:

is %(ProjectReference.Private) metadata ignored when %(ReferenceOutputAssembly) is false❔

I'm wondering if I should remove Private="false" everywhere we use ReferenceOutputAssembly="false" (and would if it's just excess verbiage)
Also curious whether Private="true" ReferenceOutputAssembly="false" on an item would mean "copy output assembly to my bin/ folder but don't reference it when compiling"

[on SkipGetTargetFrameworkProperties="true" exceptions from the single TFM norm] Do any of these raise red flags for you @rainersigwald❔

dougbu · 2021-10-08T23:45:36Z

Note that while, at a glance, this change looks cosmetic, the typos (%(PrivateAssets) and especially @(ReferenceOutputAssemblies)) are important and may be causing some part of our file-locking problems. Looking forward to reviews as well as thoughts on the questions above and not quite as far above

rainersigwald · 2021-10-11T21:17:34Z

is %(ProjectReference.Private) metadata ignored when %(ReferenceOutputAssembly) is false❔

I think so, because most of the ResolveProjectReference machinery doesn't pay attention to it. Mostly it just gets inherited by the eventual @(Reference) item that gets created, which then respects it. If you have ReferenceOutputAssembly=false that won't get created. However it's still considered here when copying files around, so it might have some impact. https://github.com/dotnet/msbuild/blob/bbb9655b007be6d079985f3a7ec14f5d82a18f64/src/Tasks/Microsoft.Common.CurrentVersion.targets#L4920

[on SkipGetTargetFrameworkProperties="true" exceptions from the single TFM norm] Do any of these raise red flags for you @rainersigwald

It's definitely unusual but nothing scares me too much.

dougbu · 2021-10-11T22:57:40Z

I think so

😀 much appreciated @rainersigwald❕

/ping reviewers (especially the many code owners tagged). This PR is ready to go.

Pilchie

Our build is complicated enough that I don't understand everything this is doing but the general idea seems sound.

However, one concern is that I thought MSBuild didn't block for ProjectReferences to build if they had ReferenceOutputAssembly="false". Since this sets that in more places, is it likely to actually introduce more build races? @rainersigwald do you know about the scheduling aspect?

dougbu · 2021-10-13T03:03:44Z

Our build is complicated enough

Well, consistency should reduce the concept count a bit 😺

I thought MSBuild didn't block for ProjectReferences to build if they had ReferenceOutputAssembly="false"

Hmm, I don't recall problems in this area except w.r.t. our .npmproj projects. We use ReferenceOutputAssembly="false" all over the place, mostly to ensure build order. The problem, if any, should get worse only in the ~5 projects where I fixed ReferenceOutputAssembl**ies**="false".

rainersigwald · 2021-10-13T13:40:42Z

However, one concern is that I thought MSBuild didn't block for ProjectReferences to build if they had ReferenceOutputAssembly="false".

It's actually the opposite: we continue to block on the build of the referenced projects in ResolveProjectReferences, but don't do anything with the output (if you don't specify OutputItemType).

From the perspective of the project, the MSBuild task doesn't offer a fire-and-forget mode; it's always a synchronous wait. The node will yield the project and do other stuff, but the calling project is frozen until the requested projects/targets are done.

Pilchie · 2021-10-13T15:41:54Z

It's actually the opposite: we continue to block on the build of the referenced projects in ResolveProjectReferences, but don't do anything with the output (if you don't specify OutputItemType).

I have vague memories of us trying this in the early days of Roslyn.sln and running into problems with it, but I definitely don't remember the details, and you know this space better than I.

dougbu added 2 commits October 6, 2021 16:34

Remove most UndefineProperties use

6acca56

- should not be necessary

dougbu requested review from a team, BrennanConroy and rainersigwald October 6, 2021 23:38

dougbu requested review from a team, Pilchie, Tratcher, halter73, javiercn and pranavkm as code owners October 6, 2021 23:38

dougbu commented Oct 7, 2021

View reviewed changes

Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 7, 2021

dougbu commented Oct 7, 2021

View reviewed changes

!fixup! Restore some %(UndefineProperties) use

6c4bdcf

- keep using this metadata when referring to .npmproj projects

!fixup! Missed one ReferenceOutputAssemblies

0197a05

Pilchie reviewed Oct 12, 2021

View reviewed changes

Pilchie approved these changes Oct 13, 2021

View reviewed changes

dougbu merged commit c8a3c22 into dotnet:main Oct 14, 2021

ghost added this to the 7.0-preview1 milestone Oct 14, 2021

dougbu deleted the dougbu/reference.consistency branch October 14, 2021 05:01

dougbu mentioned this pull request Nov 2, 2021

Allow Hosting Bundle to be upgraded by MU #37966

Merged

dougbu mentioned this pull request Jan 19, 2022

'The process cannot access the file' during CI build sometimes #32219

Closed

wtgodbe mentioned this pull request Jan 24, 2022

[release/5.0] Improve File access issues #39732

Merged

github-actions Bot locked and limited conversation to collaborators Dec 8, 2023

Conversation

dougbu commented Oct 6, 2021

Uh oh!

dougbu commented Oct 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dougbu commented Oct 7, 2021

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented Oct 7, 2021

Uh oh!

dougbu commented Oct 7, 2021

Uh oh!

dougbu commented Oct 7, 2021

Uh oh!

dougbu commented Oct 7, 2021

Uh oh!

dougbu commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented Oct 8, 2021

Uh oh!

rainersigwald commented Oct 11, 2021

Uh oh!

dougbu commented Oct 11, 2021

Uh oh!

Pilchie left a comment

Choose a reason for hiding this comment

Uh oh!

dougbu commented Oct 13, 2021

Uh oh!

rainersigwald commented Oct 13, 2021

Uh oh!

Pilchie commented Oct 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dougbu commented Oct 7, 2021 •

edited

Loading