Runtime: All libraries tests failing on Win7 x86

[jkotas]

All tests are failing in "Libraries Test Run release coreclr Windows_NT x86 Debug" legs on Win7:

C:\h\w\B60309FC\w\AC1508F7\e>"C:\h\w\B60309FC\p\dotnet.exe" exec --runtimeconfig Invariant.Tests.runtimeconfig.json --depsfile Invariant.Tests.deps.json xunit.console.dll Invariant.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
Failed to load the dll from [C:\h\w\B60309FC\p\shared\Microsoft.NETCore.App\6.0.0\coreclr.dll], HRESULT: 0x8007007F
Failed to bind to CoreCLR at 'C:\h\w\B60309FC\p\shared\Microsoft.NETCore.App\6.0.0\'
Failed to create CoreCLR, HRESULT: 0x80008088
----- end Sat 08/22/2020 15:54:30.84 ----- exit code -2147450743 ----------------------------------------------------------
 Sat 08/22/2020-15:54:30.87

Hit in #41199, #41200, ...

[Dotnet-GitSync-Bot]

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

[msftbot[bot]]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

[jkotas]

@josalem I have traced this down using CI history to your change https://github.com/dotnet/runtime/pull/41008 . I have done a trial run with the change reverted and it fixed the problem (#41216). I am going to revert the change to unblock the CI.

[josalem]

Ah, I think I know why that's happening. Looks like the Windows API I used, GetOverlappedResultEx, is only available from Win8 forward (https://docs.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-getoverlappedresultex). Hmm okay, let's revert it and I'll adjust the code to use the GetOverlappedResult+WaitForSingleObject approach from the previous version.

[danmosemsft]

Should we be compiling against Windows 7 headers or with the right define so that is not possible to compile code that uses functions not available in our lowest supported Windows version? Is it possible we're using some elsewhere ?

[jkotas]

We are doing light up for newer OSes as needed in number of places. If we restricted the SDK headers to Windows 7, we would have to manually duplicate definitions required to make the light up work.

[josalem]

Then should we at least have a win7 PR validation leg that always runs? The reverted patch had all green PR validation for both master and release branches before it merged. If we care about not using APIs newer than win7, we should make sure we prevent them from making it in, or we'll have to hunt down the change after the fact as we did here. The error message doesn't make it immediately obvious what went wrong and had the patch been bigger I'm not sure I would have noticed the API version issue after the fact and connected the dots. It looks like we already have a libraries win7 leg, could we just make sure that runs on coreclr changes?

[jkotas]

We can look into tweaking the mix that runs for changes under src/coreclr/.... There are downsides with switching one of the existing runs to Windows 7: We may lose coverage for some of the hardware intrinsics (the OS does not support them). Also, Windows 7 is more flaky and harder to debug. It is unclear whether it would be an improvement.

Note that unless we run all CI legs on all PRs, we are always going to have an occasional situation like this one when green PR is merged and it makes other PRs red. The fact that this failure sneaked into master is "by design".

The one change that I think we should consider is to change the CI configuration in release branches to run all legs on all PRs. The cost and risk profile of release and servicing branches is different and it is more important to catch regressions in release branches earlier.

[josalem]

I think that is a good compromise. Running a full pass on release/* PRs seems like a worthwhile change. How simple would it be to add the existing Windows 7 Libraries leg to the src/coreclr/... collection? I can look on Monday and see if it is feasible without too much risk.

[danmosemsft]

That sounds good to me @josalem thanks.

[bartonjs]

The one change that I think we should consider is to change the CI configuration in release branches to run all legs on all PRs.

That's how corefx does release/3.1, as of https://github.com/dotnet/corefx/commit/7a91663d049161af0705383c59a80e7418f70a1c#diff-99bf73a793466cf9e21246a87a5fd47c

[josalem]

Thanks @bartonjs, I created a draft PR on release/5.0 that does an equivalent change. I'll flip it to "ready for review" after validating that kicks off all the variants.

[josalem]

fixed by #41305