Skip to content

Implicit import of Dispatch when importing Foundation? #1206

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

Merged
merged 2 commits into from
Oct 10, 2017

Conversation

johnno1962
Copy link
Contributor

@johnno1962 johnno1962 commented Sep 7, 2017

Hi Apple,

With apologies for the rash of controversial PRs this week, I’m getting to the end the preparation of a release candidate for the Android/Swift toolchain that I’ve been working on and testing with a wide range of Open Source packages (Alamofire, Moya, RxSwift etc.) for portability. One assumption that commonly comes up in these projects is that importing Foundation is enough to have Dispatch classes available to the Swift source i.e it is imported implicitly when you import Foundation.

You must have considered this, and for compatibility with Darwin I would have thought this was the way to go so I’ve added an @_exported import of Dispatch to NSObject.swift in the toolchain for want of a better place to put it. I had also experimented with exporting CoreFoundation but functions like bind() and accept() seem to leak from CoreFoundation.h and are defined in Glibc causing ambiguity errors.

What are your thoughts?

@ianpartridge
Copy link
Contributor

I support the general idea: that as import Foundation on Darwin implies import Dispatch we should behave the same on Linux.

I'll defer to @parkera etc. on whether this is the best way to achieve that.

@parkera
Copy link
Contributor

parkera commented Sep 11, 2017

This gets us the nice Swift interface of Dispatch, right? Not an exported C API?

@johnno1962
Copy link
Contributor Author

I’m pretty sure that’s the case. The C dispatch API would be an import of “CDispatch".

@alblue
Copy link
Contributor

alblue commented Sep 12, 2017

@swift-ci please test

@johnno1962
Copy link
Contributor Author

Interesting failure. Implicit import of Dispatch means XCTest must now know how to link against it when building. You can fix it with this small tweak: SwiftJava/swift-corelibs-xctest@1b73c3e

@johnno1962
Copy link
Contributor Author

johnno1962 commented Sep 12, 2017

I’ve created a PR on XCTest to resolve this: swiftlang/swift-corelibs-xctest#201

@parkera
Copy link
Contributor

parkera commented Sep 12, 2017

Can you create a simple app that is outside the Swift build system and imports Foundation with this change? Does this change what linker flags they need to use as well?

@johnno1962
Copy link
Contributor Author

Sure, I’ll try to create a test. It shouldn’t be a problem as something using a completed toolchain will find libdispatch.so and libFoundation.so in the same directory. It’s only during the build system build they are in different directories: Ninja-ReleaseAssert/foundation-linux-x86_64/Foundation & Ninja-ReleaseAssert/libdispatch-linux-x86_64/src/.libs

@johnno1962
Copy link
Contributor Author

You should be able to try the test again now the tweak to swift-corelibs-xctest was merged.

@briancroom
Copy link
Contributor

@swift-ci please test

@johnno1962
Copy link
Contributor Author

Thanks again @briancroom, @parkera re: creating a test app I’ve been using this to patch run apps for weeks as well as running the foundation tests so I think this won't have an impact. Foundation itself is dependent on Dispatch anyway.

@parkera
Copy link
Contributor

parkera commented Sep 14, 2017

Ok

@xwu
Copy link
Contributor

xwu commented Sep 15, 2017

IMO, this should really be its own file, which IIRC is the same way that swift-package-manager organizes its code that provides for libc shims, etc. There is no reason why NSObject is the correct place to put this important declaration.

@ianpartridge
Copy link
Contributor

A whole new file just for one line seems a bit unnecessary - I'm fine with using NSObject.swift. @parkera - what do you think? Is this OK for merge?

@alblue alblue requested review from parkera and phausler October 6, 2017 11:16
@parkera
Copy link
Contributor

parkera commented Oct 6, 2017

I'm ok with doing it in NSObject - but can we put a comment or something with a bit of explanation of why it's there?

@johnno1962 johnno1962 force-pushed the Implicit-import-Dispatch branch from 84f4628 to e5569b3 Compare October 6, 2017 17:39
@parkera
Copy link
Contributor

parkera commented Oct 6, 2017

@swift-ci test and merge

@alblue
Copy link
Contributor

alblue commented Oct 10, 2017

@swift-ci please test and merge

@swift-ci swift-ci merged commit 2b52994 into swiftlang:master Oct 10, 2017
@johnno1962
Copy link
Contributor Author

Thanks @alblue, @parkera just can’t bring himself to merge my PRs 😀, one down three to go...

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.

7 participants