Add task to download files from helix results container

[safern]

Contributes to: #2076

This pr adds a Task for helix jobs to be able to download a file from the results container after a helix run, i.e testResults.xml or coverage.xml.

1. Every HelixWorkItem should include a metadata entry to specify which files it needs to download:

<HelixWorkItem Include="WorkItem">
   <DownloadFilesFromResults>testResults.xml</DownloadFilesFromResults>
</HelixWorkItem>

2. A destination directory in the host machine can be specified through an MSBuild property -- $(HelixResultsDestinationDir).
   • If this property is not set and BUILD_SOURCESDIRECTORY property is available (predefined variable in Azure DevOps), it defaults to $(BUILD_SOURCESDIRECTORY)/artifacts/helixresults.
   • If BUILD_SOURCESDIRECTORY is empty, it would log a warning and will not execute the task.
3. A directory under this destination folder will be created per each Job created => each HelixTargetQueue for the same payload. This folder will be named with the JobId.
4. Under this Job directory, there will be a metadata.txt file, that will contain the following information:

HelixQueue=%(SentJob.HelixTargetQueue)
Configuration=$(HelixConfiguration)
Architecture=$(HelixArchitecture)

More metadata can be specified to be written into this file, through and ItemGroup => HelixDownloadResultsMetadata
5. Under the Job directory, there will be a directory for every HelixWorkItem that specified the DownloadFilesFromResults metadata, and under that folder it will contain the files that where available for download.
6. If a file is not found within the container, a Warning will be logged through MSBuild.

The structure of the directory will look as follows:

- HelixResultsDestinationDir \
  - JobId \
    - metadata.txt
    - HelixWorkItemName \
       - downloaded files.

cc: @ViktorHofer @danmosemsft @markwilkie @Chrisboh

[jonfortescue]

Overall, I think this looks fine, but it really should be made so that it doesn't undo the work done in #1991.

[echesakov]

cc @RussKeldorph @jashook

[safern]

@alexperovich @jonfortescue this is ready for another round of reviews.

[alexperovich]

I think you actually need to empty these item groups before adding things to them. IIRC this will currently write the metadata for jobs 1 and 2 together in the file for 2 because the item group has been filled with both.

[safern]

I haven't seen this behavior locally, but it doesn't hurt to empty this item group.

Actually the metadata itemgroup was correctly written for multiple jobs. No duplication at all.

[alexperovich]

Hrm, I wonder if there is some msbuild magic with batching that is making the items not show up.

[safern]

Maybe because the ItemGroup is within the scope of the target and since we're batching it creates 2 different copies and evaluations of it? Basically 2 different scopes? @ericstj might know.

[ericstj]

I believe the side-effects of a batched target aren't observable until after all batches of that target run.

Try this:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.0</TargetFramework>
    <_newProperty>unset</_newProperty>
  </PropertyGroup>

  <ItemGroup>
    <TestItem Include="Abc" />
    <TestItem Include="123" />
  </ItemGroup>
  
  <Target Name="BatchTarget" Inputs="%(TestItem.Identity)" Outputs="Unused">
    <Message Text="BatchTarget Property: $(_newProperty)" Importance="High" />
    <ItemGroup>
      <_newItem Include="@(TestItem)" />
    </ItemGroup>
    <PropertyGroup>
      <_newProperty>@(TestItem)</_newProperty>
    </PropertyGroup>
    <Message Text="BatchTarget: @(_newItem)" Importance="High" />
  </Target>
  
  <Target Name="TestTarget" DependsOnTargets="BatchTarget">
    <Message Text="TestTarget Property: $(_newProperty)" Importance="High" />
    <Message Text="TestTarget: @(_newItem)" Importance="High" />
  </Target>

</Project>

Output of this is:

  BatchTarget Property: unset
  BatchTarget: Abc
  BatchTarget Property: unset
  BatchTarget: 123
  TestTarget Property: 123
  TestTarget: Abc;123

[safern]

Since we don't intend to use this items outside the target I guess we're safe on leaving it as is.

[alexperovich]

[ericstj]

DownloadFromResultsContainer.cs(100,36): error CS1501: No overload for method 'DownloadToFileAsync' takes 3 arguments [D:\j\_work\1\s\src\Microsoft.DotNet.Helix\Sdk\Microsoft.DotNet.Helix.Sdk.csproj]

Hmm, do you need a newer Azure storage client? https://docs.microsoft.com/en-us/dotnet/api/microsoft.windowsazure.storage.blob.cloudblob.downloadtofileasync?view=azure-dotnet#Microsoft_WindowsAzure_Storage_Blob_CloudBlob_DownloadToFileAsync_System_String_System_IO_FileMode_System_Threading_CancellationToken_

[safern]

Hmm, do you need a newer Azure storage client?

Yes, they actually split up the new version into multiple packages. They stopped building WindowsAzure.Storage and created multiple packages. So figuring out which ones we need.

[safern]

This is actually what I had to change to consume the new Azure SDK since they had some breaking changes and split the packages: 49cb506

FYI: @Anipik since I changed dependencies and namespaces on the CommitManager.

[safern]

This is actually what I had to change to consume the new Azure SDK since they had some breaking changes and split the packages: 49cb506
FYI: @Anipik since I changed dependencies and namespaces on the CommitManager.

Actually I just deleted the commit, Helix.SDK AzurePipelines task was not happy with the Newtonsoft upgrade required by the new azure SDKs. I will leave it as is, without flowing the cancellation token to DowloadToFileAsync since I don't have much time to invest on doing the upgrades, will revisit later to fix it if need it.

[ericstj]

Helix.SDK AzurePipelines task was not happy with the Newtonsoft upgrade required by the new azure SDKs

Yeah it probably needs the AssemblyResolver

I'm ok with you undoing the cancellation for now, but it's something you should consider moving forward for tasks which do long running things. Not implementing cancellation means a control-C will hang until the task completes. You can kill the process but that doesn't give a flushed log (so if you're working remote you might be missing the info for diagnosing).

[safern]

I don't think it is the AssemblyResolver, since the AzureDevOpsTask has BaseTask as its base class and it actually enables the AssemblyResolver for the desktop implementation: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Helix/Sdk/BaseTask.Desktop.cs#L9

[safern]

I'm ok with you undoing the cancellation for now, but it's something you should consider moving forward for tasks which do long running things. Not implementing cancellation means a control-C will hang until the task completes. You can kill the process but that doesn't give a flushed log (so if you're working remote you might be missing the info for diagnosing).

Yeah, makes sense. Maybe moving the ct.ThrowIfCancellationRequested() inside the for loop so for every file it checks and it doesn't download all the files if a cancellation is requested before finishing?

[alexperovich]

@safern @ericstj #2130 tracks the work to make everything in the helix sdk cancelable. I created it a while back in dotnet/core-eng, but never had time to do it.