-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[release/5.0] Set isFullMatrix to true for release/5.0 #41310
[release/5.0] Set isFullMatrix to true for release/5.0 #41310
Conversation
Tagging subscribers to this area: @ViktorHofer |
If someone knowledgeable signs off on this, it's approved to merge as it reduces 5.0 risk eg of Windows 7 issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Please get a cr. I will follow-up on when to merge.
@safern can you please have a look at this? |
I think we should do a more surgical change here. This enables also more helix queues for libraries tests which could slow down builds and clog queues, specially since those are intentionally only set for full matrix either due to limited resources or because they have a lower priority. Why not just add Windows 7 to the libraries tests runs against a Checked runtime instead? |
The intention was to mirror what we have set up for release/3.1. This change only sets |
I think 5.0 is still having enough traffic to not do it yet, but I don't know what other folks think. |
Btw I agree we should do this, however I don't know if we should do it yet. |
Then it seems like we're in consensus on this patch being merged, but we need to agree on the timing. I'm content to wait till traffic dies down a little bit if validation and CI time is a concern. Let me look into what the surgical version would look like. If we are specifically looking to enable Windows 7, we may need to modify platform-matrix.yml and then plumb the change back up to the pipelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve as the change makes sense, but we should wait on timing consensus.
cc: @jkotas @danmosemsft
I agree. At least wait until after we are in tactics ask mode. |
The test failures seem valid/real though. |
Presumably any failures here are also showing up in CI builds — cc @Anipik |
So every test is failing on Windows7 with HRESULT: 0x8007007F, ERROR_PROC_NOT_FOUND. This appears to be #41217. @josalem perhaps you just need to rerun to ensure this was built with #41305. Looks like most Linux builds are seeing a failure of
|
72a6536
to
dfe65d5
Compare
@ericstj rebasing to make sure it has all the newest changes including the revert of that PR. |
The ssl should be fixed with #41372 for 5.0. If we still see failures I can investigate. |
osx failure is #36466 |
Let's make sure to rerun CI and get fresh results. |
dfe65d5
to
c0d5388
Compare
rebased to get the lastest runtime & running the ci |
Failures are known #43166 |
As discussed on #41217 and in the hopes of avoiding something like #41217 happening again, these changes set the Azure Pipeline variable
isFullMatrix
to true. This should cause all platform variants to run for PRs on the release/5.0 branch. Specifically, this change will cause all the jobs inruntime.yaml
to evaluate their trigger condition totrue
.Customer Impact
N/A
Testing
N/A
Risk
This is a purely infrastructure change that should mitigate further risk.
CC - @dotnet/runtime-infrastructure @tommcdon