Fix target specific download to not clean the unpack dir

[sdmaclea]

No description provided.

[ghost]

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

[sdmaclea]

The cleanUnpackFolder: false change had issues on Windows where 7-zip was reporting out of memory issues and asking for confirmation of overwrite.

7-Zip [64] 16.00 : Copyright (c) 1999-2016 Igor Pavlov : 2016-05-10
Scanning the drive for archives:
1 file, 1075531 bytes (1051 KiB)
Extracting archive: F:\workspace\_work\1\s\__download__\CoreCLRManagedTestArtifacts_Windows_NT_arm_checked\CoreCLRManagedTestArtifacts_Windows_NT_arm_checked.zip
--
Path = F:\workspace\_work\1\s\__download__\CoreCLRManagedTestArtifacts_Windows_NT_arm_checked\CoreCLRManagedTestArtifacts_Windows_NT_arm_checked.zip
Type = zip
Physical Size = 1075531

Would you like to replace the existing file:
  Path:     F:\workspace\_work\1\s\artifacts\tests\coreclr\Windows_NT.arm.Checked\Common\CoreCLRTestLibrary\CoreCLRTestLibrary\TestLibrary.dll
  Size:     35840 bytes (35 KiB)
  Modified: 2020-05-01 07:17:28
with the file from archive:
  Path:     Common\CoreCLRTestLibrary\CoreCLRTestLibrary\TestLibrary.dll
  Size:     35840 bytes (35 KiB)
  Modified: 2020-05-01 07:42:13
? (Y)es / (N)o / (A)lways / (S)kip all / A(u)to rename all / (Q)uit? 
Archives with Errors: 1
 
stderr: 
ERROR: Can't allocate required memory!
 
error: undefined;
2020-05-01T07:56:30.8679799Z ##[section]Finishing: Unzip managed test artifacts (built on Windows_NT)

There targetSpecific tests are expected to have some files which also exist in the AnyOS AnyCPU tests. Overwrite is intentional.

I revised the patch to download to unpack into a temp directory, then use CopyFiles@2 to copy them to their ultimate destination.

YML has parsed successfully and jobs are now running.

[safern]

Would it make sense to instead port into this PR building the target specific tests as part of the run job, or is that a very big change?

[sdmaclea]

That PR already exists #35645. If it is right it should fix the issue.

It is a bigger change, so it may take longer to review/merge.

[safern]

Ok, since this is a temp workaround, if it fixes the issue I don't want to block on this, so fine with me and we can remove this workaround on #35645

[sdmaclea]

Looks like I need to add an Overwrite: true to the CopyFiles@2 task. Done

[safern]

Also throwing an idea here:
Might be worth adding a Target that validates the count of the workitems to prevent future breaks:

runtime/src/coreclr/tests/helixpublishwitharcade.proj

Line 261 in 2b4935c

<HelixWorkItem Include="@(Payloads->Metadata('PayloadGroup'))">

[sdmaclea]

There is similar code here.

runtime/src/coreclr/tests/src/runtest.proj

Lines 51 to 79 in 2b4935c

	<!-- Target to check the test build, to see if it looks ok. We've had several cases where a change inadvertently and drastically changes
	the set of tests that are built, and that change is unnoticed. The most common case is for a build of the Priority 1 tests
	to only build the Priority 0 tests. This target is run after a test build to verify that the basic number of tests that were
	built is basically what was expected. When this was written, there were about 2500 Priority 0 tests and about 10268 Priority 1
	tests on Windows, 9976 on Ubuntu (it differs slightly based on platform). We currently check that the number of Priority 0 tests
	is greater than 2000 and less than 3000, and the number of Priority 1 tests is greater than 9000.
	-->
	<Target Name="CheckTestBuild" DependsOnTargets="GetListOfTestCmds">
	<Error Condition="!Exists('$(XunitTestBinBase)')"
	Text="$(XunitTestBinBase) does not exist. Please run build-test.cmd from the repo root at least once to get the tests built." />
	<PropertyGroup>
	<TestCount>@(AllRunnableTestPaths->Count())</TestCount>
	</PropertyGroup>
	<Message Text="Found $(TestCount) built tests"/>
	<Error Condition="'$(CLRTestNeedTargetToBuild)' != '' and '$(CLRTestNeedTargetToBuild)' != 'targetGeneric' and '$(CLRTestNeedTargetToBuild)' != 'targetSpecific'" Text="Unknown CLRTestNeedTargetToBuild $(CLRTestNeedTargetToBuild)" />
	<ItemGroup Condition="'$(CLRTestNeedTargetToBuild)' != 'targetSpecific'">
	<Error Condition="'$(CLRTestPriorityToBuild)' == '0' and '$(TestCount)' &lt;= 2000" Text="Unexpected test count. Expected &gt; 2000, found $(TestCount).'" />
	<Error Condition="'$(CLRTestPriorityToBuild)' == '0' and '$(TestCount)' &gt;= 3000" Text="Unexpected test count. Expected &lt; 3000, found $(TestCount).'" />
	<Error Condition="'$(CLRTestPriorityToBuild)' == '1' and '$(TestCount)' &lt;= 9000" Text="Unexpected test count. Expected &gt; 9000, found $(TestCount).'" />
	<Error Condition="'$(CLRTestPriorityToBuild)' != '0' and '$(CLRTestPriorityToBuild)' != '1'" Text="Unknown priority $(CLRTestPriorityToBuild)" />
	</ItemGroup>
	<ItemGroup Condition="'$(CLRTestNeedTargetToBuild)' == 'targetSpecific'">
	<Error Condition="'$(TestCount)' &lt;= 100" Text="Unexpected test count. Expected &lt; 100, found $(TestCount).'" />
	</ItemGroup>
	</Target>

However due to the various test runs it is not clear to me how this should be done for helix with job filtering for runtime and scenario.

Seems like this should be a separate issue.

[safern]

That is testing the number of tests built. Helix is creating workitems which contains all the tests and I believe those are always the same number in coreclr as there's logic to group the tests by workitems depending on the test category. @echesakovMSFT and @jashook should know more about it but that is my understanding.

So WorkItems should essentially have always the same length.

[echesakov]

That is testing the number of tests built. Helix is creating workitems which contains all the tests and I believe those are always the same number in coreclr as there's logic to group the tests by workitems depending on the test category. @echesakovMSFT and @jashook should know more about it but that is my understanding.

So WorkItems should essentially have always the same length.

Yes, @safern is right. We had issues in the past with accidental breaking test build mechanism - it was also hard to notice and that's why this check was added.

Unfortunately, it would not help to identify issues like this with Helix submissions. But I like the idea of adding such check to runtime/src/coreclr/tests/helixpublishwitharcade.proj. Although, the number of Helix workitems is not the same across all architectures/OSs (since some of the test directories are not built/run on non-Windows platforms or not grouped into a separate Helix workitem) we can put 24 as lower bound for WorkItems->Count() (this number is based on educated guess from the Kusto query I sent last night)

[sdmaclea]

#35645 (comment) about failing ready2run tests will probably also affect this fix.

[sdmaclea]

Closing in favor of #35783