-
Notifications
You must be signed in to change notification settings - Fork 606
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
[ci] Disable tests with MSVC for now #2688
Conversation
They have been broken for enough time now and they just make all changes have broken CI builds and people are habitually ignoring other failures.
Definitely a good idea. I meant to ask about this while working on #2678 (which could use your review still by the way) because it was really confusing. If they aren't expected to work and people should ignore them, then they should be disabled. Another step in this direction would be use use the repository settings to require a PR workflow that includes specific checks that must pass in order to merge. |
They are expected to work, but they broke recently and no one was able to debug it (@dscorbett couldn’t reproduce locally), so I’m disabling them as a last resort. |
Hold on. |
Yeah, I'm currently debugging this. |
I've got it building on an Azure instance and can reproduce the problem. hb-shape is dying in the dagger in
A debugging printf I added in a filter before that gets executed, one in the next hb_filter does not. So it's crashing in |
Yeah, confirmed that if I comment out that line it goes through. |
But yes, this cannot go into a release. |
Closing this then, though I still don’t like the fact that all our changes have failing CI builds. |
Reopening this to track and fix the corruption / compiler issue. |
I though I reopened, and the reopen button is inactive for me now, but still says closed. Weird. |
Does reopen not work once the branch has been deleted, maybe? |
You can't re-open PR's when the source branch with commits either doesn't exist or has been changed so that the latest commit is the same as the proposed merge target. I don't think this PR would be the right place to track the fix anyway. If the fix is going to be different that disabling the CI job, then just a regular issue stating the problem would be a better tracking point. Then whatever mechanics end up in a PR can take care of it but the issue is tracking the problem not a potential solution. |
They have been broken for enough time now and they just make all changes have broken CI builds and people are habitually ignoring other failures.