-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
All libraries tests failing on Win7 x86 #41217
Comments
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. |
Tagging subscribers to this area: @tommcdon |
Ah, I think I know why that's happening. Looks like the Windows API I used, |
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 ? |
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. |
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? |
We can look into tweaking the mix that runs for changes under 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. |
I think that is a good compromise. Running a full pass on |
That sounds good to me @josalem thanks. |
That's how corefx does release/3.1, as of dotnet/corefx@7a91663#diff-99bf73a793466cf9e21246a87a5fd47c |
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. |
fixed by #41305 |
All tests are failing in "Libraries Test Run release coreclr Windows_NT x86 Debug" legs on Win7:
Hit in #41199, #41200, ...
The text was updated successfully, but these errors were encountered: