-
Notifications
You must be signed in to change notification settings - Fork 19
Fix failing 3C test for Windows #952
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
Comments
Comment from @mattmccutchen-cci: Making sure to notify @mgrang that this has been submitted. |
Comment from @mgrang: I have fired off Windows and Linux validations for this PR. Will approve once those pass. |
Comment from @mgrang: Windows X64 build still fails with this.
|
Comment from @mattmccutchen-cci:
It's odd if this failure did not manifest when you originally filed correctcomputation/checkedc-clang#345. But based on ea1edad, I'm guessing the culprit is the loss of the |
Comment from @mgrang: Several unit tests seem to be still failing.
We are currently discussing ways to enable you to validate your changes on Windows. Because this PR seems to be taking more than a couple of iterations I would suggest we keep the tests disabled until you are able to run your validations. |
Comment from @mwhicks1:
We have some guesses on what the problem what might be, so we could try to push those. But if you can provide us a way to test on Windows, we can do that first. Let us know what you'd prefer. |
Comment from @john-h-kastner: @mgrang , the latest batch of commits fixes all the errors we found when testing 3C on a windows VM. |
Comment from @mgrang: Thanks for fixing this. I verified that the X64 Windows build passes with your latest commit set. LGTM. |
Comment from @mgrang: Could you please also validate the 32bit compiler on Windows? I see that one test is still failing with the 32bit compiler on Windows:
|
Comment from @mgrang: The reason that I could not validate Windows X86 build before merging this PR is because that build never seems to complete if run manually. This is because the build uses 32 bit MSVC linker for building clang. This should get fixed if we switch it to use the 64 bit MSVC linker. But we would only be able to do that after our source upgrade is complete. |
Comment from @mattmccutchen-cci: @mgrang If you could not complete a manual build on Windows x86, I doubt we will be able to either. (John said he tried and ran into a problem; I didn't ask what it was.) Wouldn't it be more reasonable to mark the failing test unsupported on Windows x86 (or if that isn't easy, all Windows) until you fix the linker issue you mentioned, and then we can retry the manual build on Windows x86? As with the previous test failures, I think it will be more efficient if you iterate on getting the right |
Comment from @mgrang:
AFAIK, there is no way to disable a test only for Windows X86. We need to disable for all Windows. You can refer to this (the current) PR for the correct UNSUPPORTED directive (it is the one that John reverted when he re-enabled the tests). |
Comment from @mattmccutchen-cci:
Sure: #961 |
This issue was copied from checkedc/checkedc-clang#956
Our previous PR #930 left some tests failing on windows (correctcomputation#345) . These changes should will hopefully fix those tests. We haven't been able to test these a windows build.
The text was updated successfully, but these errors were encountered: