Add workload restore command #18910

wli3 · 2021-07-14T18:16:54Z

Adding _FindAllReferenceWorkload target to find workloads in project.
Discover the projects and then pipe all arguements to workload install
command

dotnet-issue-labeler · 2021-07-14T18:16:56Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

wli3 · 2021-07-14T18:17:47Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloads.targets

+    </PropertyGroup>
+  </Target>
+
+  <Target Name="_FindAllReferenceWorkload" DependsOnTargets="_SetSkipResolvePackageAssets;GetSuggestedWorkloads;PrepareProjectReferences" Returns="@(_ResolvedSuggestedWorkloads)">


@dsplaisted help me double check the RemoveProperties are right

wli3 · 2021-07-14T18:18:13Z

...Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloadsCrossTarget.targets

+============================================================
+-->
+
+  <Target Name="_FindAllReferenceWorkload"


@dsplaisted I end up adding a outer build version instead using the same one

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloads.targets

rainersigwald · 2021-07-14T19:46:19Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloads.targets

+    Projects="@(_MSBuildProjectReferenceExistent)"
+    Targets="GetSuggestedWorkloads"
+    BuildInParallel="$(BuildInParallel)"
+    Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration); %(_MSBuildProjectReferenceExistent.SetPlatform); GenerateErrorsForMissingWorkloads=false; SkipResolvePackageAssets=true"


Likewise here, are these properties avoidable?

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs

...Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloadsCrossTarget.targets

src/Cli/dotnet/Parser.cs

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommandParser.cs

sfoslund · 2021-07-14T21:08:35Z

src/Tests/dotnet-workload-restore.Tests/DiscoverAllProjectsTests.cs

+
+        [Fact]
+        public void WhenCallWithDirectoryWith2ProjectItShouldFindAll()
+        {


I don't see any tests confirming we can get from the project/sln arg in the parse result to a resolved list of projects

WhenCallWithSlnOrProjectArgumentItCollectProjectsFromSolution is the pass in example

But it's not covering the parsing/ reading the parse result value

wli3 · 2021-07-14T23:04:01Z

@dsplaisted Rainer checked msbuild part too, so I think it won't be too bad. I'll get it in once test passes.

dsplaisted · 2021-07-15T01:30:38Z

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs

+        internal static List<string> DiscoverAllProjects(string currentDirectory,
+            IEnumerable<string> slnOrProjectArgument = null)


I don't think we should discover all projects here. We should do the same thing as MSBuild, which is more or less to expect that there is a single project or solution file in the directory, and error out if not. The actual logic is in ProcessProjectSwitch: https://github.com/dotnet/msbuild/blob/1d845f30213e9ba4f36d4d5a366c0cc8285eed6e/src/MSBuild/XMake.cs#L2763

Ideally we wouldn't have to duplicate the logic. It looks like RunCommand has a simple form of it (FindSingleProjectInDirectory) which only looks for project files, not solution files.

I thought about the same thing, but why make it difficult. I started with refactoring RunCommand. But when there are 2 projects in the folder what the user should do? It is valid, and workload restore can handle it (unlike runcommand), why fail and ask the user to input one by one?

All other MSBuild and dotnet commands require you to specify the project if there is more than one in the folder. It seems weird to make dotnet workload restore the one exception.

src/Cli/dotnet/commands/dotnet-workload/restore/LocalizableStrings.resx

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs

dsplaisted · 2021-07-15T01:41:07Z

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs

+        {
+            var globalProperties = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
+            {
+                {Constants.MSBuildExtensionsPath, AppContext.BaseDirectory},


This was a bit confusing to me. I see that RunCommand does the same thing, but it would be nice to have a comment explaining this.

Removed the line. It still works.

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommandParser.cs

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloads.targets

dsplaisted · 2021-07-15T02:16:14Z

src/Tests/Microsoft.NET.Build.Tests/WorkloadTests.cs

+
+            getValuesCommand.GetValues()
+                .Should()
+                .BeEquivalentTo("microsoft-android-sdk-full", "microsoft-net-runtime-mono-tooling");


I think these names are changing, you may want to update stage 0 and see if that includes the rename so that this test doesn't start failing when it happens.

due to #19021 I will update stage 0 in a different PR

src/Tests/Microsoft.NET.Build.Tests/WorkloadTests.cs

Nirmal4G

Some suggestions for the targets!

...rosoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.GetRequiredWorkloads.CrossTargeting.targets

...Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloadsCrossTarget.targets

Nirmal4G · 2021-07-19T14:58:07Z

...Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommonCrossTargeting.targets

@@ -13,5 +13,6 @@ Copyright (c) .NET Foundation. All rights reserved.

  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.DefaultAssemblyInfo.targets" />
  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.DefaultOutputPaths.targets" />
+  <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.GetRequiredWorkloads.CrossTargeting.targets" />


This targets file is meant to have multi-targeting/outer-build logic for Workloads, right?
So, the following name below might make more sense!

Suggested change

<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.GetRequiredWorkloads.CrossTargeting.targets" />

<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.Workloads.CrossTargeting.targets" />

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.PackageDependencyResolution.targets

...rosoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.GetRequiredWorkloads.CrossTargeting.targets

wli3 · 2021-07-19T21:36:31Z

@dsplaisted all issue addressed

wli3 · 2021-07-19T22:55:43Z

@dsplaisted I'll merge once tests pass

Adding _FindAllReferenceWorkload target to find workloads in project. Discover the projects and then pipe all arguements to workload install command

wli3 requested review from dsplaisted and sfoslund July 14, 2021 18:16

wli3 commented Jul 14, 2021

View reviewed changes

wli3 force-pushed the workload-restore2 branch from 5f128b2 to d2aeceb Compare July 14, 2021 18:20

wli3 commented Jul 14, 2021

View reviewed changes

src/Cli/dotnet/commands/dotnet-workload/restore/WorkloadRestoreCommand.cs Outdated Show resolved Hide resolved

rainersigwald reviewed Jul 14, 2021

View reviewed changes

wli3 commented Jul 14, 2021

View reviewed changes

...Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.ImportWorkloadsCrossTarget.targets Outdated Show resolved Hide resolved

wli3 force-pushed the workload-restore2 branch 2 times, most recently from 3fe20dd to 3912692 Compare July 14, 2021 21:03

sfoslund reviewed Jul 14, 2021

View reviewed changes

marcpopMSFT added the Urgency-Soon label Jul 14, 2021

wli3 force-pushed the workload-restore2 branch from 3912692 to efd9c34 Compare July 14, 2021 22:54

sfoslund approved these changes Jul 14, 2021

View reviewed changes

wli3 force-pushed the workload-restore2 branch from c883636 to 9c80ba9 Compare July 14, 2021 23:09

wli3 enabled auto-merge July 14, 2021 23:09

wli3 force-pushed the workload-restore2 branch 2 times, most recently from efd9c34 to ad609bf Compare July 14, 2021 23:23

dsplaisted reviewed Jul 15, 2021

View reviewed changes

dsplaisted disabled auto-merge July 15, 2021 02:25

wli3 force-pushed the workload-restore2 branch from ad609bf to 10169f0 Compare July 18, 2021 21:15

Nirmal4G reviewed Jul 19, 2021

View reviewed changes

wli3 force-pushed the workload-restore2 branch from d3814a9 to df633b7 Compare July 19, 2021 21:36

wli3 force-pushed the workload-restore2 branch from df633b7 to c95fa4b Compare July 19, 2021 22:38

wli3 enabled auto-merge July 19, 2021 22:55

wli3 force-pushed the workload-restore2 branch from c95fa4b to 87b09f9 Compare July 19, 2021 23:29

wli3 force-pushed the workload-restore2 branch from 87b09f9 to bbd4211 Compare July 20, 2021 02:15

wli3 mentioned this pull request Jul 20, 2021

Add workload restore command #19062

Merged

Add workload restore command

5d2faf1

Adding _FindAllReferenceWorkload target to find workloads in project. Discover the projects and then pipe all arguements to workload install command

wli3 force-pushed the workload-restore2 branch from bbd4211 to 5d2faf1 Compare July 20, 2021 04:36

wli3 merged commit 17181d3 into dotnet:main Jul 20, 2021

wli3 deleted the workload-restore2 branch July 20, 2021 15:59

marcpopMSFT mentioned this pull request Jul 21, 2021

Workload restore #17432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workload restore command #18910

Add workload restore command #18910

wli3 commented Jul 14, 2021

dotnet-issue-labeler bot commented Jul 14, 2021

wli3 Jul 14, 2021

wli3 Jul 14, 2021

rainersigwald Jul 14, 2021

sfoslund Jul 14, 2021

wli3 Jul 14, 2021

sfoslund Jul 14, 2021

wli3 commented Jul 14, 2021

dsplaisted Jul 15, 2021

wli3 Jul 15, 2021 •

edited

Loading

dsplaisted Jul 15, 2021

dsplaisted Jul 15, 2021

wli3 Jul 19, 2021

dsplaisted Jul 15, 2021

wli3 Jul 19, 2021

Nirmal4G left a comment

Nirmal4G Jul 19, 2021 •

edited

Loading

wli3 commented Jul 19, 2021

wli3 commented Jul 19, 2021

		internal static List<string> DiscoverAllProjects(string currentDirectory,
		IEnumerable<string> slnOrProjectArgument = null)

	<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.GetRequiredWorkloads.CrossTargeting.targets" />
	<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.Workloads.CrossTargeting.targets" />

Add workload restore command #18910

Add workload restore command #18910

Conversation

wli3 commented Jul 14, 2021

dotnet-issue-labeler bot commented Jul 14, 2021

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

wli3 commented Jul 14, 2021

Choose a reason for hiding this comment

wli3 Jul 15, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nirmal4G left a comment

Choose a reason for hiding this comment

Nirmal4G Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

wli3 commented Jul 19, 2021

wli3 commented Jul 19, 2021

wli3 Jul 15, 2021 •

edited

Loading

Nirmal4G Jul 19, 2021 •

edited

Loading