-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Fix build failure for importing NSUInteger as Int instead of the previously tested ok UInt #7979
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
Fix build failure for importing NSUInteger as Int instead of the previously tested ok UInt #7979
Conversation
|
@swift-ci please test and merge |
|
@jrose-apple Why would the previous PR build and test ok with the failure? It seems rather fishy. |
|
Terrible reasons. I tried to stir up interest to fix this but there didn't seem to be any takers. |
|
You may be able to avoid this in the future by adding |
|
Do I need to add [system] to the shim module map? |
|
...ah. Well, drat. Can you make a PR for that? |
|
for adding |
afcd410 to
7d51602
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
No, to change the importer to always import shims as system modules. The only reason we didn't was to catch warnings, but we can add a test that just uses -Wsystem-headers. |
|
so you are saying instead that I should change them to be: #define SHIMS_INCLUDE_FLAG "-isystem"unconditionally no matter if it is NDEBUG or not? |
|
Yes, I think that's the right thing to do going forward. |
|
so to be clear; the |
7d51602 to
2712a3c
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
647b686 to
125ef78
Compare
|
@swift-ci please test and merge |
|
@phausler I think the OS X platform build failed again. The UI is just a bit messed up I believe. See: https://ci.swift.org/job/swift-PR-osx/5888/ It looks like it is testing your latest commit and it failed. |
…ead of conditionally marking them in debug versus non debug builds.
125ef78 to
00f3908
Compare
|
@swift-ci please test and merge |
that is the wrong sha1; it should be 00f3908 |
|
the latest build is showing a build of 00f3908 which is correct @jrose-apple should I just revert this whole ball of wax? |
|
It's possible we have a non-deterministic test. We should really try to get this in because it's blocking all our no-asserts builds. Let's just merge by hand once we pass a test. @swift-ci Please smoke test macOS |
|
(I blame Past Me for leaving this trap in there.) |
|
@jrose-apple I can force the merge even with tests failing if you think that's the right thing to do. |
|
It passed the smoke tests, so no force needed. Thanks, Philippe! |
fixes rdar://problem/30899471