Porting to Cygwin #381
Porting to Cygwin #381
Conversation
| #include <unistd.h> | ||
| #if !DEPLOYMENT_TARGET_CYGWIN |
There was a problem hiding this comment.
Generally we avoid the negative-sense deployment target macros. Keeping them consistently positive-sense and using #if/#else instead makes it easier to reason about them (and there are lots).
|
I like the general direction of this, but I wonder if we can simplify the patch a bit by lumping cygwin under linux in general (since it seems to closely related) and then having just a few places where they differ. Including the Windows SDK (I'm assuming this is the "windows.h" we imported) is a pretty big change though. Is there any compatibility shim provided by cygwin that will allow us to skip that? |
|
@parkera, Thanks your reviews. I will reply your review or modify the code, as soon as I fix my machine which does not boot from two days ago. |
tinysun212
force-pushed
the
pr-foundation-cygwin-1
branch
3 times, most recently
from
966ce1d
to
67f67a1
Compare
|
@parkera, |
| #define TARGET_OS_CYGWIN 0 | ||
| #elif __CYGWIN__ | ||
| #define TARGET_OS_DARWIN 0 | ||
| #define TARGET_OS_LINUX 0 |
There was a problem hiding this comment.
So - how different is cygwin from linux? cygwin's idea is to provide a Linux-like environment for Windows.
I think we should have TARGET_OS_LINUX here set to 1 for cygwin, and also define DEPLOYMENT_TARGET_LINUX when building cygwin.
We can also define TARGET_OS_CYGWIN here (as you have done) and use that in the rare places where cygwin is truly different from Linux. That should dramatically simplify this patch, and also keep us from having to specify that deployment target everywhere (or forgetting to, and breaking the build for Cygwin).
There was a problem hiding this comment.
Cygwin provides the POSIX system calls and environment, and is similar to Linux because Linux also confirms to POSIX. But some functions like eventfd, epoll, timerfd_create seems to be never implemented in Cygwin because they are not POSIX standard.
Anyway, I agree your method will simplify this patch and I will modify if others has no opposing opinion, although there was similar opposing discussions for compiler. swift #2351
tinysun212
force-pushed
the
pr-foundation-cygwin-1
branch
from
67f67a1
to
c66be53
Compare
|
@parkera, I changed definitions for Cygwin, |
|
ping @parkera |
tinysun212
force-pushed
the
pr-foundation-cygwin-1
branch
from
c66be53
to
97e7fea
Compare
|
@parkera, I think the requested changes are applied. |
|
Thanks - we'll take a look at this in the post 3.0 timeframe. |
tinysun212
force-pushed
the
pr-foundation-cygwin-1
branch
2 times, most recently
from
0f7b9e4
to
45d2111
Compare
|
We should see about merging this now. @tinysun212 can you rebase against master? |
This is the initial porting. IS_CYGWIN might be changed to some other macro function. A simple function like below code is working in Cygwin. --- import Foundation let swifty = NSURLComponents(string: "https://swift.org")! print("\(swifty.host!)") ---
tinysun212
force-pushed
the
pr-foundation-cygwin-1
branch
from
45d2111
to
caf1aa6
Compare
|
@parkera, I rebased and squashed to one commit. |
|
@swift-ci please test |
|
@swift-ci please test and merge |
|
Don't know why the CI won't kick this off, but it passed before so I don't see what the problem would be now. Merging manually. |
|
Thanks for your review and merging. |
See apple/swift-corelibs-foundation#381 for why this isn't os(Cygwin).
See apple/swift-corelibs-foundation#381 for why this isn't os(Cygwin).
This is the initial porting.
A simple function like below code is working in Cygwin.
Maybe there are many bugs in this port, but it is sufficient us to review and taste the Foundation in Cygwin.
The Swift compiler option "-DCYGWIN" will be added if companion PR #541 is merged.
The macro might be changed to some other macro function like os(Cygwin) later.