[stdlib] Fixed for Cygwin #3886

tinysun212 · 2016-07-30T07:21:17Z

What's in this pull request?

I submitted PR #2351 which has stdlib patch for Cygwin, but it is not merged since os(Cygwin) is not widely accepted. Now the conditional variable CYGWIN can be used for the files of *.swift and *.swift.gyb.

This PR uses CYGWIN instead of the previous unaccepted symbol and has more patch.

Resolved bug number: (SR-)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

tinysun212 · 2016-07-30T07:22:43Z

@gribozavr, @compnerd, Please review this PR.

gribozavr · 2016-07-30T08:23:20Z

stdlib/private/SwiftPrivateLibcExtras/Subprocess.swift

+
+  if waitpid(pid, statusPtr, 0) < 0 {
+    preconditionFailure("waitpid() failed")
+  }


Sorry, could you explain how does this work? Why does the system waitpid() function need a Swift box? It would help if you could paste the imported signature of waitpid() on your system.

I captured the signature in /usr/include/sys/wait.h on Cygwin.

/* Allow `int' and `union wait' for the status. */ typedef union { int *__int_ptr; union wait *__union_wait_ptr; } __wait_status_ptr_t __attribute__ ((__transparent_union__)); pid_t waitpid (pid_t __pid, __wait_status_ptr_t __status, int __options);

I'll be happy if someone suggests any simpler way.

withUnsafePointer(to: status) { statusPtr in let statusPtrWrapper = __wait_status_ptr_t(__int_ptr: statusPtr) waitpid(pid, statusPtrWrapper, 0) }

should work I think.

It is successfully compiled after 1st line is changed to withUnsafeMutablePointer(&status) {.
The compiler taught me how to change. Thank you.

compnerd · 2016-07-30T16:56:07Z

stdlib/public/Platform/glibc.modulemap.gyb

@@ -320,6 +320,13 @@ module SwiftGlibc [system] {
      }
 % end

+% if CMAKE_SDK in ["CYGWIN"]:


Why limit this to CYGWIN?

Foundation's ./Foundation/NSFileHandle.swift uses Glibc's L_SET, L_INCR, L_XTND. In Linux, it is enough only include unistd.h, but Cygwin's unistd.h doesn't define them.

To support L_xxx macro, Cygwin should include sys/file.h which defines them. But other platforms which already support Foundation would already have the definitions.

I think that we can just include module on all targets which have the header.

OK. I'll change this.

tinysun212 · 2016-07-31T05:54:04Z

stdlib/private/SwiftPrivateLibcExtras/SwiftPrivateLibcExtras.swift

+#if CYGWIN
+        func asFdSetPtr(
+          _ p: UnsafeMutablePointer<UInt>?
+        ) -> UnsafeMutablePointer<_types_fd_set>? {


Updating to 7/29 snapshot, I added new patches.

BTW, I don't know why this is differ to Linux.
The definition part in sys/select.h is as follows.

typedef struct _types_fd_set { fd_mask fds_bits[_howmany(FD_SETSIZE, NFDBITS)]; } _types_fd_set; #define fd_set _types_fd_set

fd_set did not work, but _types_fd_set worked.

Probably because we are dropping the aliasing in the clang import. Can you please file a SR and send it my way with a tiny test case (something like creating a function which uses fd_set)?

Could you explain some more detail, please? (dropping aliasing, tiny test case ...)

Sure. The clang-importer will ignore the #define based aliasing as its not a real type alias, but rather a token replacement via the preprocessor. The result is that the module does not contain a definition for fd_set although, we should. We could handle simple token replacements which replace type aliased names with an additional type alias. This would allow us to use the definition.

After writing out this explanation, Im left wondering, why don't we work around this for now with:
typealias fd_set = _types_fd_set

As to the test case, a simple swift file which demonstrates the lost type alias so that we could have a test case for this is what I was asking for. Something like this should do the trick:

#ifndef MODULE_H
#define MODULE_H
typedef struct s { int i; } _s;
#define s _s
#endif

import module
let _ = s(32)

This would currently fail as the type alias of s would be dropped, and is entirely standalone.

gribozavr · 2016-08-10T01:36:09Z

stdlib/private/StdlibUnittest/StdlibUnittest.swift.gyb

@@ -1437,6 +1438,8 @@ public enum OSVersion : CustomStringConvertible {
      return "PS4"
    case .android:
      return "Android"
+    case .windows:
+      return "Windows"


For this change to StdlibUnittest, would you mind adding a test like validation-test/StdlibUnittest/Linux.swift?

gribozavr · 2016-08-10T01:37:06Z

@swift-ci Please test

gribozavr · 2016-08-10T01:37:25Z

@tinysun212 Thank you! The changes LGTM modulo a small request to add a test for StdlibUnittest changes.

tinysun212 · 2016-08-12T21:09:34Z

@gribozavr I changed for StdlibUnittest.

CodaFi · 2016-08-12T21:41:10Z

Dmitri is on vacation for a while. In the meantime, is there a reason you called this windowsCygnus and not cygwin or windowsCygwin?

CodaFi · 2016-08-12T21:43:12Z

@swift-ci please test OS X platform.

tinysun212 · 2016-08-12T22:41:26Z

It is only for the clear expression that Cygwin is related to Windows.
windowsCygnus is from the llvm target string, *-*-windows-cygnus.

I'm preparing a PR for MinGW, it will have windowsGnu which is analogous to *-*-windows-gnu, and they evaluate the TestRunPredicate .windowsAny to be true .

CodaFi · 2016-08-14T17:40:20Z

Ah, thanks. LGTM.

@swift-ci please test.

tinysun212 · 2016-08-14T22:00:47Z

validation-test/StdlibUnittest/Cygwin.swift was failed and patched.

CodaFi · 2016-08-14T22:07:25Z

Ok...

@swift-ci please smoke test OS X platform.

tinysun212 · 2016-08-14T22:59:21Z

@compnerd I applied your review, thanks.
@CodaFi Now I applied all of the reviews.

compnerd · 2016-08-16T00:09:25Z

validation-test/StdlibUnittest/ChildProcessShutdown/FailIfChildCrashesDuringShutdown.swift

@@ -2,7 +2,7 @@
 // REQUIRES: executable_test

 import StdlibUnittest
-#if os(Linux) || os(FreeBSD) || os(PS4) || os(Android)
+#if os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Windows)


Doesn't this break on MSVC hosts?

Not tested on windows-msvc. Only tested on Cygwin, and I believe it will be passed for MinGW.

Did you build an alternatives to Glibc for MSVC ?

Yes, msvcrt :-). Its a swift wrapper for ucrt and visualc.

Would you change after this PR?
I think the code would be like this.

#if os(Windows) && MSVC import Ucrt import VisualC #elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Windows) import Glibc #else import Darwin #endif

@compnerd Could you share the build script (or manual) for MSVC ?

I build on Linux. Just set INCLUDE and LIB as per Windows (adjust them for the movement from C:\ to whatever you're using) and add WINDOWS to the targets to build. I can give you my cmake/ninja invocation later when I'm near a computer.

I just do the following:

add -DSWIFT_SDKS="LINUX;WINDOWS" -DCMAKE_AR=$(which llvm-ar) -DCMAKE_RANLIB=$(which llvm-ranlib) -DSWIFT_WINDOWS_ICU_I18N_INCLUDE=${SWIFT_TOP_SRCDIR}/build/icu/source/i18n -DSWIFT_WINDOWS_ICU_UC_INCLUDE=${SWIFT_TOP_SRCDIR}/build/icu/source/common via --extra-cmake-options

You need to make sure that ld.lld is available to your toolchain, as building for Windows (MSVC) on !Windows will enable lld.

Additionally, I have the following environment variables setup:
INCLUDE='${SDK_TOP_SRCDIR}/Microsoft Visual Studio 14.0/VC/include;${SDK_TOP_SRCDIR}/Windows Kits/10/Include/10.0.10586.0/ucrt;${SDK_TOP_SRCDIR}/Windows Kits/10/Include/10.0.10586.0/um;${SDK_TOP_SRCDIR}/Windows Kits/10/Include/10.0.10586.0/shared'
LIB='${SDK_TOP_SRCDIR}/Microsoft Visual Studio 14.0/VC/lib;${SDK_TOP_SRCDIR}/Windows Kits/10/Lib/10.0.10586.0/ucrt/x86;${SDK_TOP_SRCDIR}/Windows Kits/10/Lib/10.0.10586.0/um/x86'

Within those paths which are directly lifted from a windows SDK installation, I have two symlinks constructed: ${SDK_TOP_SRCDIR}/Microsoft Visual Studio 14.0/VC/include/module.map -> ${SWIFT_TOP_SRCDIR}/swift/stdlib/public/Platform/visualc.modulemap
${SDK_TOP_SRCDIR}/Windows Kits/10/Include/10.0.10586.0/ucrt -> ${SWIFT_TOP_SRCDIR}/swift/stdlib/public/Platform/ucrt.modulemap

The import libraries for ICU are in the build directory as well. That tends to be sufficient to cross-compile from Linux x86_64 to Windows x86.

Thanks you.
Did you use the swift/clang compiler for Linux ?
I'll run those on my Linux machine.

Yeah, Ive been doing the entire build on Linux (and should be pretty trivial to reproduce on Darwin).

This still breaks the non-cygwin case :/ You never addressed this!

CodaFi · 2016-08-19T05:20:35Z

In the meantime @tinysun212, could you please rebase again?

Patched for compnerd review in PR swiftlang#3886.

CodaFi · 2016-08-23T17:39:25Z

@swift-ci please smoke test macOS platform.

swift-ci · 2016-10-04T18:25:08Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 29af0ca6b110eac3f8531510a15faaac5fcd6c31
Test requested by - @moiseev

tinysun212 · 2016-10-04T22:37:44Z

In the building console of Swift Test OS X Platform, there is a liking error for Benchmark_Onone-arm64-apple-ios8.0.
Does it caused by this changes?

moiseev · 2016-10-04T23:08:22Z

@tinysun212 I don't think this change is responsible for the failure. Let's try again.
@swift-ci Please smoke test macOS platform

moiseev · 2016-10-05T21:14:58Z

@swift-ci Please smoke test macOS platform

tinysun212 · 2016-10-07T23:46:17Z

I found a strange message in the CI test log.

pull request #35 from tinysun212/pr-cygwin-1, what is this?, I don't have the PR of number 35.

11:21:11 + git fetch --recurse-submodules=yes
11:21:12 + git clean -fdx
11:21:12 + git submodule foreach --recursive git clean -fdx
11:21:12 + git submodule foreach --recursive git reset --hard HEAD
11:21:12 + git reset --hard HEAD
11:21:12 HEAD is now at c324ee3 Merge pull request #35 from tinysun212/pr-cygwin-1
11:21:12 + git checkout master
11:21:12 Already on 'master'
11:21:12 Your branch is up-to-date with 'origin/master'.
11:21:12 + git reset --hard origin/master
11:21:12 HEAD is now at c324ee3 Merge pull request #35 from tinysun212/pr-cygwin-1
11:21:12 + git fetch --recurse-submodules=yes
11:21:12 + git clean -fdx
11:21:13 + git submodule foreach --recursive git clean -fdx
11:21:13 + git submodule foreach --recursive git reset --hard HEAD
11:21:13 + git reset --hard HEAD
11:21:13 HEAD is now at 5af77f3 Merge pull request #95 from kainjow/master

tinysun212 · 2016-10-07T23:50:06Z

Anyway, I rebased to most recent commit.
I tested on macOS with build-script.sh -R -t, couldn't see any failed message.
Please test again ?

shahmishal · 2016-10-08T00:08:51Z

@tinysun212 Do you have the CI build link where you saw this message?

tinysun212 · 2016-10-08T00:22:27Z

Yes, here.

shahmishal · 2016-10-08T00:37:53Z

@tinysun212 this came from ToT llbuild repo, looks like this was merged recently. (Not really recently, but ToT)

swiftlang/swift-llbuild#35

tinysun212 · 2016-10-08T01:55:10Z

That was my PR, I can remember now, I'm sorry I forgot it. The CI build log displayed it under the line
From https://github.com/apple/swift-llvm, so I couldn't think about that.

Although that was not the cause of the fail of tests, please smoke test again?

shahmishal · 2016-10-10T06:06:54Z

@swift-ci Please smoke test

tinysun212 · 2016-10-10T06:54:27Z

This time, the error is caused by duplicating glibc modulemap changes with #5131 which was merged 1 hour ago. I'll soon resolve this.

tinysun212 · 2016-10-10T07:24:52Z

Resolving the duplicate, I rebased and merged to one commit.

tinysun212 · 2016-10-13T05:57:55Z

please test

gparker42 · 2016-10-13T09:14:03Z

@swift-ci Please test

slavapestov · 2017-01-11T06:52:49Z

@tinysun212 Sorry for dropping the ball on this PR. Do you mind rebasing and fixing the merge conflicts so that we can get this merged in?

tinysun212 · 2017-01-12T05:05:17Z

@slavapestov I will resolve this PR this weekend.

slavapestov · 2017-01-12T06:50:09Z

Thanks!

- CYGWIN symbol is used to distinguish Cygwin environment from other OS and other environment in Windows. - Added windows and windowsCygnus to OSVersion in StdlibUnittest

slavapestov · 2017-01-17T07:49:18Z

@swift-ci Please test

compnerd · 2017-01-18T02:32:13Z

validation-test/StdlibUnittest/ChildProcessShutdown/FailIfChildExitsDuringShutdown.swift

@@ -2,7 +2,7 @@
 // REQUIRES: executable_test

 import StdlibUnittest
-#if os(Linux) || os(FreeBSD) || os(PS4) || os(Android)
+#if os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Windows)


Doesnt os(Windows) evaluate true for MSVC builds? This breaks Windows.

os(Windows) is true for MSVC, MinGW, Cygwin.

compnerd · 2017-01-18T02:32:23Z

validation-test/StdlibUnittest/ChildProcessShutdown/PassIfChildCrashedDuringTestExecution.swift

@@ -2,7 +2,7 @@
 // REQUIRES: executable_test

 import StdlibUnittest
-#if os(Linux) || os(FreeBSD) || os(PS4) || os(Android)
+#if os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Windows)


compnerd · 2017-01-18T02:32:47Z

validation-test/StdlibUnittest/Stdin.swift

@@ -6,7 +6,7 @@ import StdlibUnittest

 #if os(OSX) || os(iOS) || os(tvOS) || os(watchOS)
 import Darwin
-#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android)
+#elseif os(Linux) || os(FreeBSD) || os(PS4) || os(Android) || os(Windows)


gribozavr reviewed Jul 30, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from e03a455 to 2d5cf62 Compare July 30, 2016 12:17

compnerd reviewed Jul 30, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from 2d5cf62 to daca8f4 Compare July 31, 2016 05:41

tinysun212 reviewed Jul 31, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from daca8f4 to bbe021e Compare August 6, 2016 21:39

gribozavr reviewed Aug 10, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch 3 times, most recently from 1c90d4d to 1bb1cd5 Compare August 12, 2016 21:07

tinysun212 mentioned this pull request Aug 14, 2016

[Compiler] Cygwin is identified by os(Cygwin) instead of os(Windows) #2351

Closed

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from 1bb1cd5 to 229d968 Compare August 14, 2016 21:58

compnerd reviewed Aug 16, 2016
View reviewed changes

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from c341902 to f400376 Compare August 19, 2016 19:22

tinysun212 added a commit to tinysun212/swift-windows that referenced this pull request Aug 19, 2016

[stdlib] Refactoring some ifdef code

f400376

Patched for compnerd review in PR swiftlang#3886.

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from 29af0ca to a78eb04 Compare October 7, 2016 23:39

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from a78eb04 to f43b97e Compare October 10, 2016 07:21

slavapestov self-assigned this Jan 11, 2017

[stdlib] Fixed for Cygwin

a8dec7f

- CYGWIN symbol is used to distinguish Cygwin environment from other OS and other environment in Windows. - Added windows and windowsCygnus to OSVersion in StdlibUnittest

tinysun212 force-pushed the pr-stdlib-cygwin-3 branch from f43b97e to a8dec7f Compare January 16, 2017 17:51

slavapestov merged commit 928a319 into swiftlang:master Jan 17, 2017

compnerd reviewed Jan 18, 2017

View reviewed changes

tinysun212 deleted the pr-stdlib-cygwin-3 branch October 21, 2018 01:11

[stdlib] Fixed for Cygwin #3886

[stdlib] Fixed for Cygwin #3886

Uh oh!

Conversation

tinysun212 commented Jul 30, 2016

What's in this pull request?

Resolved bug number: (SR-)

Uh oh!

tinysun212 commented Jul 30, 2016

Uh oh!

gribozavr Jul 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Aug 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Aug 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Aug 10, 2016

Uh oh!

gribozavr commented Aug 10, 2016

Uh oh!

tinysun212 commented Aug 12, 2016

Uh oh!

CodaFi commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Aug 12, 2016

Uh oh!

tinysun212 commented Aug 12, 2016

Uh oh!

CodaFi commented Aug 14, 2016

Uh oh!

tinysun212 commented Aug 14, 2016

Uh oh!

CodaFi commented Aug 14, 2016

Uh oh!

tinysun212 commented Aug 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinysun212 Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gribozavr Jul 30, 2016 •

edited

Loading

compnerd Aug 13, 2016 •

edited

Loading

compnerd Aug 14, 2016 •

edited

Loading

CodaFi commented Aug 12, 2016 •

edited

Loading

tinysun212 Aug 16, 2016 •

edited

Loading

shahmishal commented Oct 8, 2016 •

edited

Loading