Restore PackageReferences in temporary assembly projects to import build extensions #1056

ryalanms · 2019-06-21T21:29:10Z

The temporary project is not getting props and targets files (build extension files) from PackageReferences defined in their parent project. This is causing failures in referenced packages that rely on build extensions (e.g., Google Protocol Buffers) to perform pre-compile tasks, such as code generation.

Several options were considered, including consuming the generated props and targets that include the PackageReferences props/targets from the outer parent project, as the parent project Restore has already run and includes identical PackageReferences. However, we opted to run Restore directly on the temporary project instead, which allowed a fix without changes to external build files (other than a minor change to the Arcade SDK). There are now two Restore operations: one for the parent project and one for the temporary project. Note that Restore will not re-download packages but will generate the assets file, targets file, and props file. These files include imports for build extensions in the PackageReferences.

These tasks could easily be condensed in to a single parameterized task, but breaking the steps in to three made a logs easier to read and improved debugging. There are now three tasks:

CreateTemporaryTargetAssemblyProject
RestoreTemporaryTargetAssemblyProject (which calls RunProjectBuildTarget with Restore)
GenerateTemporaryTargetAssembly

The first task creates a temporary project with random component in the name from the outer parent project. RestoreTemporaryTargetAssemblyProject consumes the temporary project and runs Restore, generating assets, .props, and .targets files that include imports to any build extensions inside PackageReferences. These files are then consumed in GenerateTemporaryTargetAssembly as part of the
normal build process for the temporary project.

Note that there is an Arcade SDK targets file change that needs to be coordinated with this change. Workaround.props needs to be modified to remove a temporary workaround for the lack of PackageReference support to prevent duplicate import errors. dotnet/arcade#3127

(APICompat was re-baselined to account for the public API surface changes in PresentationBuildTasks.)

walterlv · 2019-06-22T01:17:48Z

@ryalanms Thanks for fixing this issue.
I have some NuGet packages which extend the building so should I have to check target building SDK version to determine whether should I hack to fix or use your fix?

walterlv

There is some debug-only code in the three related building tasks.

I use this method to debug my custom building tasks, too. But I think it shouldn't be merged into the release-version code.

...PresentationBuildTasks/Microsoft/Build/Tasks/Windows/CreateTemporaryTargetAssemblyProject.cs

.../src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/GenerateTemporaryTargetAssembly.cs

...DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/RunProjectBuildTarget.cs

ryalanms · 2019-06-22T01:33:08Z

Thanks for reviewing, @walterlv. After this fix is in, there shouldn't be any need to use custom code to get the build extensions in PackageReferences to work in user code.

ryalanms · 2019-06-22T16:18:04Z

Note that this will fail to build with duplicate imports until the Arcade SDK change is merged and picked up.

rladuca

I think this looks good, I like 3 tasks better than pivoting on params.

rladuca · 2019-06-24T17:18:45Z

Note that this will fail to build with duplicate imports until the Arcade SDK change is merged and picked up.

Arcade just merged the change.

vatsan-madhavan · 2019-06-24T17:50:23Z

src/Microsoft.DotNet.Wpf/ApiCompat/Baselines/PresentationBuildTasks-Net48.baseline.txt

@@ -1,3 +1,13 @@
 Compat issues with assembly PresentationBuildTasks:
+MembersMustExist : Member 'Microsoft.Build.Tasks.Windows.GenerateTemporaryTargetAssembly.CompileTypeName.get()' does not exist in the implementation but it does exist in the contract.


There is a larger unrelated questions we should try to address - namely whether ref-assembly for PresentationBuildTasks needs to be rolled into the ref-pack for .NET Core 3.0.

@nguerrera?

vatsan-madhavan · 2019-06-24T17:51:44Z

src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft.WinFx.targets

@@ -20,6 +20,8 @@
 <UsingTask TaskName="Microsoft.Build.Tasks.Windows.FileClassifier" AssemblyFile="$(_PresentationBuildTasksAssembly)" />
 <UsingTask TaskName="Microsoft.Build.Tasks.Windows.MarkupCompilePass2" AssemblyFile="$(_PresentationBuildTasksAssembly)" />
 <UsingTask TaskName="Microsoft.Build.Tasks.Windows.GenerateTemporaryTargetAssembly" AssemblyFile="$(_PresentationBuildTasksAssembly)" />
+ <UsingTask TaskName="Microsoft.Build.Tasks.Windows.CreateTemporaryTargetAssemblyProject" AssemblyFile="$(_PresentationBuildTasksAssembly)" />


Is there a reason you chose CreateTemporaryTargetAssemblyProject, instead of calling the task GenerateTemporaryTargetAssemblyProject and keep naming in line with previous task names?

vatsan-madhavan · 2019-06-24T17:52:57Z

src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft.WinFx.targets

@@ -380,6 +382,8 @@
 <PropertyGroup>

 <MarkupCompilePass2ForMainAssemblyDependsOn>
+ CreateTemporaryTargetAssemblyProject;
+ RestoreTemporaryTargetAssemblyProject;


I would recommend renaming this target as "RunRestoreTargetOnTemporaryTargetAssemblyProject"

vatsan-madhavan · 2019-06-24T18:02:10Z

src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/Microsoft.WinFx.targets

+
+ <RunProjectBuildTarget 
+ ProjectName="$(_TemporaryTargetAssemblyProjectName)"
+ BuildTarget="Restore"


Use a pattern similar to that in <Target Name="_CompileTemporaryAssembly" DependsOnTargets="BuildOnlySettings;ResolveKeySource;CoreCompile" /> and <_CompileTargetNameForLocalType Condition="'$(_CompileTargetNameForLocalType)' == ''">_CompileTemporaryAssembly</_CompileTargetNameForLocalType> used in GenerateTemporaryTargetAssembly

vatsan-madhavan · 2019-06-24T18:02:40Z

...PresentationBuildTasks/Microsoft/Build/Tasks/Windows/CreateTemporaryTargetAssemblyProject.cs

+using MS.Utility;
+using MS.Internal.Tasks;
+
+// Since we disable PreSharp warnings in this file, PreSharp warning is unknown to C# compiler.


I don't think we need this and related suppressions - we don't really use PreSharp any more

vatsan-madhavan · 2019-06-24T18:06:29Z

...PresentationBuildTasks/Microsoft/Build/Tasks/Windows/CreateTemporaryTargetAssemblyProject.cs

+ //
+ if (nodeGroup.HasChildNodes)
+ {
+ ArrayList itemToRemove = new ArrayList();


Don't use ArrayList - instead use List<object>

vatsan-madhavan · 2019-06-24T18:06:57Z

...PresentationBuildTasks/Microsoft/Build/Tasks/Windows/CreateTemporaryTargetAssemblyProject.cs

+ //
+ // Continue the loop for the next ItemGroup.
+ }
+


excess blank line

vatsan-madhavan · 2019-06-24T18:07:09Z

...PresentationBuildTasks/Microsoft/Build/Tasks/Windows/CreateTemporaryTargetAssemblyProject.cs

+ }
+
+ } // end of "for i" statement.
+


excess blank line

vatsan-madhavan · 2019-06-24T18:08:33Z

...DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/RunProjectBuildTarget.cs

+ /// </summary>
+ public sealed class RunProjectBuildTarget : Task
+ {
+ #region Constructors


Don't use regions in new code

vatsan-madhavan · 2019-06-24T18:08:47Z

...DotNet.Wpf/src/PresentationBuildTasks/Microsoft/Build/Tasks/Windows/RunProjectBuildTarget.cs

+using MS.Utility;
+using MS.Internal.Tasks;
+
+// Since we disable PreSharp warnings in this file, PreSharp warning is unknown to C# compiler.


We don't use PreSharp - remove comment and related warning suppressions

vatsan-madhavan · 2019-06-24T18:38:43Z

The approach and the fix looks good to me overall. Given the overall level of churn, I'm hesitant to merge this so close to Preview 7 (check-ins close at noon tomorrow for WPF).

I don't see clear reasons why this change should cause regressions. The approach is sound and I think the change looks good to me overall. That said, the general complexity of the fix makes me think that it is likely to bring with it a slightly higher possibility of bugs. I'd rather fork off master to preview/3.0 (which happens on in two days on 6/26), and and then merge this into master for Preview 8 (so we would get early feedback on P8 builds).

I've spoken with @ryalanms and he agrees that waiting for 2 days is reasonable.

Marking as NO MERGE for now.

vatsan-madhavan · 2019-06-26T23:59:20Z

Removing NO MERGE since #1100 is completed.

nguerrera · 2019-06-27T00:06:19Z

Are you initiating a restore during the build? That is not allowed. It breaks the promise of build --no-restore, and will also mess with VS that handles restore differently than restore target.

nguerrera · 2019-06-27T00:07:01Z

cc @davkean

nguerrera · 2019-06-27T00:09:21Z

Note that Restore will not re-download packages but will generate the assets file, targets file, and props file.

This seems risky. If there's any evaluation change in generated project that ends up changing a PackageReference it could still hit the network.

ryalanms · 2021-01-05T00:18:59Z

Closing this, since we ended up taking a different implementation. Thanks.

ghost requested review from vatsan-madhavan, rladuca and stevenbrix June 21, 2019 21:29

ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 21, 2019

ryalanms mentioned this pull request Jun 21, 2019

When including any Page item via NuGet package props/targets files, all included Compile items will lost. #811

Closed

walterlv suggested changes Jun 22, 2019

View reviewed changes

ryalanms added 2 commits June 21, 2019 18:52

Restore PackageReferences in temporary assembly projects

9f6cc81

Remove debugger calls

1e974a0

ryalanms force-pushed the dev/ryalanms/pbt_packageref_proto branch from 69adf43 to 1e974a0 Compare June 22, 2019 02:14

rladuca approved these changes Jun 23, 2019

View reviewed changes

vatsan-madhavan requested a review from nguerrera June 24, 2019 17:47

vatsan-madhavan requested changes Jun 24, 2019

View reviewed changes

vatsan-madhavan added * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) and removed * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) labels Jun 24, 2019

walterlv mentioned this pull request Jun 30, 2019

添加对 WPF 的支持 dotnet-campus/SourceYard#41

Merged

vatsan-madhavan requested a review from rladuca October 2, 2019 17:49

vatsan-madhavan added the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 2, 2019

vatsan-madhavan mentioned this pull request Feb 8, 2020

PackageReference broken in WPF projects due to tmp_proj not importing Package-supplied build authoring NuGet/Home#5894

Closed

ryalanms closed this Jan 5, 2021

mmitche deleted the dev/ryalanms/pbt_packageref_proto branch January 13, 2021 14:46

ghost locked as resolved and limited conversation to collaborators Apr 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore PackageReferences in temporary assembly projects to import build extensions #1056

Restore PackageReferences in temporary assembly projects to import build extensions #1056

ryalanms commented Jun 21, 2019

walterlv commented Jun 22, 2019

walterlv left a comment

ryalanms commented Jun 22, 2019

ryalanms commented Jun 22, 2019

rladuca left a comment

rladuca commented Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan Jun 24, 2019

vatsan-madhavan commented Jun 24, 2019

vatsan-madhavan commented Jun 26, 2019

nguerrera commented Jun 27, 2019

nguerrera commented Jun 27, 2019

nguerrera commented Jun 27, 2019

ryalanms commented Jan 5, 2021

		@@ -1,3 +1,13 @@
		Compat issues with assembly PresentationBuildTasks:
		MembersMustExist : Member 'Microsoft.Build.Tasks.Windows.GenerateTemporaryTargetAssembly.CompileTypeName.get()' does not exist in the implementation but it does exist in the contract.

Restore PackageReferences in temporary assembly projects to import build extensions #1056

Restore PackageReferences in temporary assembly projects to import build extensions #1056

Conversation

ryalanms commented Jun 21, 2019

walterlv commented Jun 22, 2019

walterlv left a comment

Choose a reason for hiding this comment

ryalanms commented Jun 22, 2019

ryalanms commented Jun 22, 2019

rladuca left a comment

Choose a reason for hiding this comment

rladuca commented Jun 24, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vatsan-madhavan commented Jun 24, 2019

vatsan-madhavan commented Jun 26, 2019

nguerrera commented Jun 27, 2019

nguerrera commented Jun 27, 2019

nguerrera commented Jun 27, 2019

ryalanms commented Jan 5, 2021