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

Overbuild caused by PackageReference support #5448

Closed
rainersigwald opened this issue Oct 5, 2021 · 5 comments · Fixed by #6071
Closed

Overbuild caused by PackageReference support #5448

rainersigwald opened this issue Oct 5, 2021 · 5 comments · Fixed by #6071
Assignees

Comments

@rainersigwald
Copy link
Member

The _wpftmp build inside a normal project is built with a global property set: $(_WpfTempProjectNuGetFilePathNoExt). It also now runs ResolveProjectReferences by default. This combines to cause overbuild of the referenced projects, because MSBuild treats projects as distinct when they have different global properties.

This can be observed in build output:

dotnet build --no-restore -bl
Microsoft (R) Build Engine version 17.0.0-preview-21477-04+3a1e456fe for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

C:\Program Files\dotnet\sdk\6.0.100-rc.2.21478.25\MSBuild.dll -bl -consoleloggerparameters:Summary -distributedlogger:Microsoft.DotNet.Tools.MSBuild.MSBuildLogger,C:\Program Files\dotnet\sdk\6.0.100-rc.2.21478.25\dotnet.dll*Microsoft.DotNet.Tools.MSBuild.MSBuildForwardingLogger,C:\Program Files\dotnet\sdk\6.0.100-rc.2.21478.25\dotnet.dll -maxcpucount -verbosity:m .\wpf_incremental.sln
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  Library -> S:\play\wpf_incremental\Library\bin\Debug\net5.0\Library.dll
  Library -> S:\play\wpf_incremental\Library\bin\Debug\net5.0\Library.dll
  WpfApp -> S:\play\wpf_incremental\WpfApp\bin\Debug\net5.0-windows\WpfApp.dll

Note the doubled Library -> S:\play\wpf_incremental\Library\bin\Debug\net5.0\Library.dll: that project is building twice.

The global property for the _wpftmp project is set here:

// Disable conflicting Arcade SDK workaround that imports NuGet props/targets
Hashtable globalProperties = new Hashtable(1);
globalProperties["_WpfTempProjectNuGetFilePathNoExt"] = "";

Timeline:

  1. Workaround for WPF temp project not importing NuGet props/targets files arcade#1581 introduced $(_WpfTempProjectNuGetFilePathNoExt) to .NET's Arcade SDK.
  2. Support PackageReferences in WPF projects #3585 introduced $(IncludePackageReferencesDuringMarkupCompilation) (defaulted to false). This introduced this race condition.
  3. [release/5.0] Support Source Generators in WPF projects #3846 backported it to .NET 5.0 SDKs. This included ee91876 that unconditionally set _WpfTempProjectNuGetFilePathNoExt to empty.
  4. Opt into IncludePackageReferencesDuringMarkupCompilation sdk#15465 turned $(IncludePackageReferencesDuringMarkupCompilation) on by default in the SDK.
  5. Revert 'Opt into IncludePackageReferencesDuringMarkupCompilation' sdk#15695 turned it off again, so this isn't noticeable in the default configurations under .NET SDK 5.0.yxx .
  6. Make source generator and package reference support opt-out (on by default) #4089 flipped the default in WPF.
@ryalanms
Copy link
Member

ryalanms commented Oct 5, 2021

This was missed as part of 6.0. Both the Arcade workaround and WPF's global property should be removed.

#4119

@rainersigwald
Copy link
Member Author

@ryalanms Do you think we can get both in for GA? I'd really like to avoid shipping this way.

@ryalanms
Copy link
Member

ryalanms commented Oct 5, 2021

@mmitche: This will require an Arcade change. We will need to disable (or remove) the Arcade WPF temp project workaround at the same time WPF's global _WpfTempProjectNuGetFilePathNoExt is removed. Is it still possible to get this in?

The property is IsWpfTempProject:

https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/Workarounds.props#L11

Or the whole change:

dotnet/arcade#1581

edit: We can disable IsWpfTempProject temporarily in our Directory.Build.props, but other WPF projects outside of the WPF repo that use Arcade will still be hit by the collision unless those repos also set IsWpfTempProject to false before Arcade props/targets are imported.

@mmitche
Copy link
Member

mmitche commented Oct 5, 2021

Yes, it can make GA. Can you make a tell mode for the arcade change, and then an ask mode for the WPF change?

@ryalanms
Copy link
Member

ryalanms commented Oct 5, 2021

Yes, it can make GA. Can you make a tell mode for the arcade change, and then an ask mode for the WPF change?

Here is the Arcade change:

dotnet/arcade#7994

@ghost ghost locked as resolved and limited conversation to collaborators Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants