Skip to content
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

Centralise test imports and add os(Android) #1528

Merged
merged 2 commits into from
Apr 26, 2018
Merged

Centralise test imports and add os(Android) #1528

merged 2 commits into from
Apr 26, 2018

Conversation

johnno1962
Copy link
Contributor

Hi Apple,

This PR adds os(Android) to the conditional imports and centralises them to reduce some boilerplate.

@parkera
Copy link
Contributor

parkera commented Apr 20, 2018

@swift-ci please test


#if os(macOS) || os(iOS)
import Darwin
#elseif os(Linux)
#elseif os(Linux) || os(Android)
import Glibc
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth simplifying this to:

#if canImport(Darwin)
    import Darwin
#elseif canImport(Glibc)
    import (Glibc)
#endif

(and also in HTTPServer.swift)

@johnno1962
Copy link
Contributor Author

The test failure should be fixed now.

@alblue
Copy link
Contributor

alblue commented Apr 20, 2018

There was a failure but it was fixed before I found this out:

13:57:57 
TestFoundation/XDGTestHelper.swift:13:23: error: use of unresolved identifier 'HTTPCookieStorage'
13:57:57         let storage = HTTPCookieStorage.shared
13:57:57                       ^~~~~~~~~~~~~~~~~
13:57:57 TestFoundation/XDGTestHelper.swift:14:26: error: use of undeclared type 'HTTPCookiePropertyKey'
13:57:57         let properties: [HTTPCookiePropertyKey: String] = [
13:57:57                          ^~~~~~~~~~~~~~~~~~~~~
13:57:57 TestFoundation/XDGTestHelper.swift:20:34: error: use of unresolved identifier 'HTTPCookie'
13:57:57         guard let simpleCookie = HTTPCookie(properties: properties) else {
13:57:57                                  ^~~~~~~~~~

@alblue
Copy link
Contributor

alblue commented Apr 20, 2018

@swift-ci please test

@johnno1962
Copy link
Contributor Author

Sorry @alblue you might need to start the test again. I had to amend the XDG changes so Linux works.

@spevans
Copy link
Contributor

spevans commented Apr 20, 2018

@swift-ci please test

@parkera
Copy link
Contributor

parkera commented Apr 20, 2018

@johnno1962 can you also confirm this works when running the tests in Xcode on Darwin?

@johnno1962
Copy link
Contributor Author

I wish I could but I’m still running Xcode 9.3 and can’t get past Foundation. Downloading the beta now..

@millenomi
Copy link
Contributor

@johnno1962 Make sure you're using a current Swift compiler toolchain (either built with ninja, or via a swift.org snapshot.)

@millenomi
Copy link
Contributor

@swift-ci please test

@johnno1962
Copy link
Contributor Author

johnno1962 commented Apr 21, 2018

I’m up to date using master from swift.org now. I’m seeing a seemingly unrelated crash on macOS here: https://github.com/apple/swift-corelibs-foundation/blob/master/CoreFoundation/String.subproj/CFStringEncodings.c#L639

@spevans
Copy link
Contributor

spevans commented Apr 21, 2018

@johnno1962 See #1498 for details of that crash.

@johnno1962
Copy link
Contributor Author

johnno1962 commented Apr 21, 2018

Thanks @spevans It’s a strange crash, a buffer overrun that always crashes at the same place. Anyways, I think this is good to go now. Sorry about the XDG problems. Can I ask, what is XDG?

@millenomi
Copy link
Contributor

@swift-ci please test and merge

@swift-ci swift-ci merged commit 3872595 into swiftlang:master Apr 26, 2018
@johnno1962
Copy link
Contributor Author

Thanks @millenomi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants