Fixed Driver Dependency Tests on Windows #22638

gmittert · 2019-02-15T02:03:38Z

All Driver/Dependency tests now pass on Windows!

Requires #22596

-- Testing: 52 of 4486 tests, 12 threads --                                                                                   
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..                                                                    
Testing Time: 150.70s                                                                                                         
  Expected Passes    : 43                                                                                                     
  Expected Failures  : 2                                                                                                      
  Unsupported Tests  : 7

The two expected failures are due to tests which expect parallel execution. The unsupported tests are due to the use of $(subshells), mostly to get the version of swift compiler that is running.

As part of this, I've also implemented output buffering for the default TaskQueue implementation since it was required for a few of the tests. Are there platforms besides Windows that use the default implementation that I need to be aware of? @compnerd had mentioned rewriting the Windows TasksQueue implementation so that it supports parallel execution soon, so this might be a temporary thing anyways.

@jrose-apple

jrose-apple

Clever! But I'm worried about the files being left behind on the filesystem—we'll have to do something about that.

jrose-apple · 2019-02-15T02:20:19Z

lib/Basic/Default/TaskQueue.inc

@@ -56,7 +58,7 @@ public:

 bool TaskQueue::supportsBufferingOutput() {
  // The default implementation does not support buffering output.


Comment needs updating!

jrose-apple · 2019-02-15T02:22:01Z

test/Driver/Dependencies/embed-bitcode-parallel.swift

@@ -14,7 +16,7 @@
 // CHECK-FIRST: {{^{$}}
 // CHECK-FIRST: "kind": "finished"
 // CHECK-FIRST: "name": "compile"
-// CHECK-FIRST: "output": "Handled main.swift\n"
+// CHECK-FIRST: "output": "Handled main.swift"


Why the change in behavior here?

jrose-apple · 2019-02-15T02:23:30Z

test/Driver/Dependencies/one-way-parallel.swift

@@ -14,7 +16,7 @@
 // CHECK-FIRST: {{^{$}}
 // CHECK-FIRST: "kind": "finished"
 // CHECK-FIRST: "name": "compile"
-// CHECK-FIRST: "output": "Handled main.swift\n"
+// CHECK-FIRST: "output": "Handled main.swift{{\r?}}\n"


This backslash isn't going to bind correctly, is it?

jrose-apple · 2019-02-15T02:26:15Z

lib/Basic/Default/TaskQueue.inc

+    SmallVector<Optional<StringRef>, 3> redirects;
+    redirects.push_back(llvm::None);
+    redirects.push_back(Optional<StringRef>(stdoutFile));
+    redirects.push_back(Optional<StringRef>(stderrFile));


No need to do any of this type conversion, or to use a SmallVector instead of a plain array. There's an implicit conversion from SmallString (a subclass of SmallVector) to StringRef, and from StringRef to Optional<StringRef>. So you should be able to just pass stdoutPath and stderrPath directly here. (We've also brought None into scope, so you don't need the llvm:: qualification.)

That way is much more pleasant!

jrose-apple · 2019-02-15T02:26:42Z

lib/Basic/Default/TaskQueue.inc

+    llvm::SmallVector<char, 64> stdoutPath;
+    llvm::SmallVector<char, 64> stderrPath;
+    if (fs::createTemporaryFile("stdout", "tmp",  stdoutPath)
+        || fs::createTemporaryFile("stderr", "tmp",  stderrPath)) {


This is clever, but what deletes the files after the run is finished?

Hrm, I think ideally it would register with the compilation via addTemporaryFile but that would involve passing the compilation context all the way down which seem unideal. Would just removing the temp files after calling the callback be reasonable? Perhaps also registering with RemoveFileOnSignal()?

:-/ Maybe? I suppose we don't expect them to be so big (on the offchance we can't clean them up), and they are going to be in a system temporary directory.

jrose-apple · 2019-02-15T02:27:46Z

lib/Basic/Default/TaskQueue.inc

+
+    StringRef stdoutContents =
+        stdoutBuffer ? StringRef(stdoutBuffer.get()->getBufferStart(),
+                                 stdoutBuffer.get()->getBufferSize())


This is just stdoutBuffer.get()->getBuffer(). :-)

gmittert · 2019-02-19T23:29:18Z

Cleaned up both the code and the temp files!

jrose-apple · 2019-02-19T23:43:23Z

lib/Basic/Default/TaskQueue.inc

@@ -100,10 +103,27 @@ bool TaskQueue::execute(TaskBeganCallback Began, TaskFinishedCallback Finished,
        T->Env.empty() ? decltype(Envp)(None)
                       : decltype(Envp)(llvm::toStringRefArray(T->Env.data()));

+    llvm::SmallVector<char, 64> stdoutPath;
+    llvm::SmallVector<char, 64> stderrPath;


Ah, this is where to use SmallString, so that you don't need to explicitly make StringRefs later!

gmittert · 2019-02-20T00:06:21Z

TIL, list initialization is really neat.

     SmallVector<Optional<StringRef>, 4> redirects = {None, {stdoutPath}, {stderrPath}};

They did need the extra braces though, clang wasn't able to go from the stdoutPath: SmallString directly to Optional

jrose-apple · 2019-02-20T00:15:08Z

lib/Basic/Default/TaskQueue.inc

+    llvm::sys::RemoveFileOnSignal(stdoutPath);
+    llvm::sys::RemoveFileOnSignal(stderrPath);
+
+    SmallVector<Optional<StringRef>, 4> redirects = {None, {stdoutPath}, {stderrPath}};


Last nitpick: This repo has an 80-column limit. But you can solve that anyway by using a plain fixed-sized array instead of a SmallVector.

Optional<StringRef> redirects[] = {None, {stdoutPath}, {stderrPath}};

jrose-apple · 2019-02-20T00:22:51Z

(not running full tests because the code being changed isn't used on POSIXy platforms…)

@swift-ci Please smoke test

jrose-apple · 2019-02-20T00:49:14Z

Oops, I forgot that this one requires the other one.

jrose-apple · 2019-02-20T19:20:39Z

@swift-ci Please smoke test

jrose-apple · 2019-02-20T21:14:14Z

Looks like something's not quite right even after merging the other PR. Can you take a look?

gmittert · 2019-02-21T02:06:41Z

I'm not entirely sure what happened. I'm unable to reproduce it on either a Linux (CentOS) or macOS machine locally. Is there something special about the Jenkins environment that I should know about?

The expected stdout and stderr look okay: the ls error is expected, but fake-ld.py never ran. The test overrides ld with fake-ld.py then expects swift to call it which then does a few assertions and prints out "Handled link" It seems like one of the tasks the driver spawned got killed before fake-ld was able to print...

13:39:28 --
13:39:28 Exit Code: 1
13:39:28 
13:39:28 Command Output (stdout):
13:39:28 --
13:39:28 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/filelists.swift.tmp/tmp/outputs-bee66b
13:39:28 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/filelists.swift.tmp/tmp/sources-7c25b1
13:39:28 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/filelists.swift.tmp/tmp-crash/outputs-ce9afc
13:39:28 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/filelists.swift.tmp/tmp-crash/sources-f43471
13:39:28 
13:39:28 --
13:39:28 Command Output (stderr):
13:39:28 --
13:39:28 ls: /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/filelists.swift.tmp/tmp-fail/sources-*: No such file or directory
13:39:28 ls: /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/filelists.swift.tmp/tmp-fail/outputs-*: No such file or directory
13:39:28 

<unknown>:0: error: compile command failed due to signal 9 (use -v to see invocation)
13:39:28 
/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/test/Driver/filelists.swift:68:16: error: CHECK-LINK: expected string not found in input
13:39:28 // CHECK-LINK: Handled link
13:39:28                ^
13:39:28 <stdin>:1:1: note: scanning from here
13:39:28 Handled a.swift
13:39:28 ^

jrose-apple · 2019-02-21T17:30:12Z

…Well, I don't get it either, but to not block this PR, how about you just revert the changes to test/Driver/filelists.swift for now? The test is still using symlinks so it's already marked unsupported on win32.

All Driver/Dependency Tests now pass on Windows!

gmittert · 2019-02-21T19:32:58Z

Reverted the filelists.swift changes. Let's see if it's happier now.

compnerd · 2019-02-21T23:43:48Z

@swift-ci please test

swift-ci · 2019-02-22T01:04:18Z

Build failed
Swift Test OS X Platform
Git Sha - ac0d9ef

compnerd · 2019-02-22T01:08:56Z

@swift-ci please test macOS platform

swift-ci · 2019-02-22T02:25:12Z

Build failed
Swift Test OS X Platform
Git Sha - ac0d9ef

gmittert · 2019-02-22T02:34:51Z

Build failed
Swift Test OS X Platform
Git Sha - ac0d9ef

Same as the previous run, During iphone simulator tests:
xcodebuild: error: The authorization was denied since no user interaction was possible.

jrose-apple · 2019-02-22T17:03:38Z

That might have been an Xcode update. Let's try one more time. (If not, it's a test change again and we'll have to figure out why.)

@swift-ci Please test macOS

swift-ci · 2019-02-22T19:49:44Z

Build failed
Swift Test OS X Platform
Git Sha - ac0d9ef

jrose-apple · 2019-02-22T21:28:37Z

@swift-ci Please test macOS

compnerd · 2019-02-23T05:41:13Z

Yay, gonna merge this, as this prevents the test harness from dying!

jrose-apple reviewed Feb 15, 2019

View reviewed changes

gmittert force-pushed the DrivingProgress branch from 8c2c5b8 to 3c40d08 Compare February 19, 2019 23:28

jrose-apple reviewed Feb 19, 2019

View reviewed changes

gmittert force-pushed the DrivingProgress branch from 3c40d08 to 0c7303d Compare February 20, 2019 00:02

jrose-apple reviewed Feb 20, 2019

View reviewed changes

gmittert force-pushed the DrivingProgress branch from 0c7303d to f78387b Compare February 20, 2019 00:21

jrose-apple approved these changes Feb 20, 2019

View reviewed changes

jrose-apple self-assigned this Feb 20, 2019

Fixed Driver Dependency Tests on Windows

ac0d9ef

All Driver/Dependency Tests now pass on Windows!

gmittert force-pushed the DrivingProgress branch from f78387b to ac0d9ef Compare February 21, 2019 19:31

compnerd merged commit 1a949ea into swiftlang:master Feb 23, 2019

compnerd mentioned this pull request Feb 23, 2019

Exterminate tests #21052

Closed

gmittert deleted the DrivingProgress branch February 23, 2019 06:10

		@@ -56,7 +58,7 @@ public:

		bool TaskQueue::supportsBufferingOutput() {
		// The default implementation does not support buffering output.

Fixed Driver Dependency Tests on Windows #22638

Fixed Driver Dependency Tests on Windows #22638

Uh oh!

Conversation

gmittert commented Feb 15, 2019

Uh oh!

jrose-apple left a comment

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

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!

gmittert commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmittert commented Feb 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Feb 20, 2019

Uh oh!

jrose-apple commented Feb 20, 2019

Uh oh!

jrose-apple commented Feb 20, 2019

Uh oh!

jrose-apple commented Feb 20, 2019

Uh oh!

gmittert commented Feb 21, 2019

Uh oh!

jrose-apple commented Feb 21, 2019

Uh oh!

gmittert commented Feb 21, 2019

Uh oh!

compnerd commented Feb 21, 2019

Uh oh!

swift-ci commented Feb 22, 2019

Uh oh!

compnerd commented Feb 22, 2019

Uh oh!

swift-ci commented Feb 22, 2019

Uh oh!

gmittert commented Feb 22, 2019

Uh oh!

jrose-apple commented Feb 22, 2019

Uh oh!

swift-ci commented Feb 22, 2019

Uh oh!

jrose-apple commented Feb 22, 2019

Uh oh!

compnerd commented Feb 23, 2019

Uh oh!

Uh oh!