NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

Switch batching method in order to get all build errors from multi-targeted projects #10872

Open dsplaisted opened 3 years ago

dsplaisted commented 3 years ago

When you try to build a project that requires a workload you don’t have, you will get an error like the following:

NETSDK1147: To build this project, the following workloads must be installed: microsoft-android-sdk-full

For multi-targeted projects, we should report all the missing workloads. Currently, a project multi-targeting to net6.0-android and net6.0-ios will generate separate errors for each inner build:

NETSDK1147: To build this project, the following workloads must be installed: microsoft-android-sdk-full NETSDK1147: To build this project, the following workloads must be installed: microsoft-ios-sdk-full

This is probably OK, though it might be nice to combine these into a single error that lists both missing workloads.

However, you have to restore before you can build, and currently restore errors out when the first inner build fails and so doesn’t report both missing workloads:

NETSDK1147: To build this project, the following workloads must be installed: microsoft-android-sdk-full

We should fix this so that you get both errors from restore.

Looking at binlogs, I think the reason for the difference is that the _GenerateProjectRestoreGraphAllFrameworks uses the MSBuild task’s Properties parameter with a batched value:

    <MSBuild
      BuildInParallel="$(RestoreBuildInParallel)"
      Projects="$(MSBuildProjectFullPath)"
      Targets="_GenerateProjectRestoreGraphPerFramework"
      Properties="TargetFramework=%(_RestoreTargetFrameworkItems.Identity);
                  $(_GenerateRestoreGraphProjectEntryInputProperties)">

      <Output
          TaskParameter="TargetOutputs"
          ItemName="_RestoreGraphEntry" />
    </MSBuild>

On the other hand, to do the inner builds, the DispatchToInnerBuilds target doesn’t use the Properties parameter at all:

    <MSBuild Projects="@(_InnerBuildProjects)"
             Condition="'@(_InnerBuildProjects)' != '' "
             Targets="$(InnerTargets)"
             BuildInParallel="$(BuildInParallel)">
      <Output ItemName="InnerOutput" TaskParameter="TargetOutputs" />

Rather, the Projects items it passes in have AdditionalProperties metadata, which is set in the _ComputeTargetFrameworkItems target:

      <_TargetFramework Include="$(TargetFrameworks)" />
      <!-- Make normalization explicit: Trim; Deduplicate by keeping first occurrence, case insensitive -->
      <_TargetFrameworkNormalized Include="@(_TargetFramework->Trim()->Distinct())" />
      <_InnerBuildProjects Include="$(MSBuildProjectFile)">
        <AdditionalProperties>TargetFramework=%(_TargetFrameworkNormalized.Identity)</AdditionalProperties>
      </_InnerBuildProjects>

Could we update the NuGet targets to use AdditionalProperties metadata instead of the Properties MSBuild task parameter, and see if that results in errors from all of the inner builds being reported?

zkat commented 3 years ago

Pushing this into https://github.com/NuGet/Home/issues/10846 but we'll likely get it done before then. It just won't go into net6.0 GA, most likely.