[Batch mode] Restore -### functionality for batch-mode by extending OutputLevel. #14961

davidungar · 2018-03-04T04:52:02Z

The current implementation of the -### does not produce the right results for batch mode. This PR reimplements that drive option by extended the OutputLevel enumeration in the driver. It reuses the machinery that is already there.

davidungar · 2018-03-04T04:53:09Z

@swift-ci please smoke test

davidungar · 2018-03-04T21:23:59Z

@swift-ci please smoke test

davidungar · 2018-03-04T23:32:51Z

At least one of these tests is failing because -### printing to stdout, but with these changes, it prints to stderr. I'm assuming that I need to fix this to preserve the old behavior.

davidungar · 2018-03-05T00:15:29Z

@swift-ci please smoke test

davidungar · 2018-03-05T01:15:36Z

@swift-ci please smoke test

davidungar · 2018-03-05T04:55:54Z

@swift-ci please smoke test

graydon

LGTM

davidungar · 2018-03-05T17:29:08Z

@jrose-apple I’m assuming you want to look at this, let me know if not.

jrose-apple

Nice, glad it worked out this easily. Can you add a test for it doing the right thing with batch mode?

jrose-apple · 2018-03-05T19:00:08Z

lib/Driver/Compilation.cpp

-  if (Level == OutputLevel::Verbose)
-    Cmd->printCommandLine(llvm::errs());
+  switch (Level) {
+    case OutputLevel::Normal:


Nitpick: accidentally-indented cases.

Tx. I'm on it! And will do a test, too.

Indentation fixed! On to the test.

davidungar · 2018-03-05T19:50:36Z

@swift-ci please smoke test

jrose-apple · 2018-03-05T20:00:57Z

test/Driver/batch_mode_print_jobs.swift

+//
+// CHECK-COMBINED: -primary-file {{.*}}/file-01.swift -primary-file {{.*}}/file-02.swift {{.*}}/file-03.swift {{.*}}/main.swift
+//
+// CHECK-ANYTHING-NOT: {{.+}}


Looks like you're not using this anymore.

Thanks for the catch. Removed.

jrose-apple · 2018-03-05T20:02:33Z

test/Driver/batch_mode_print_jobs.swift

+//
+// RUN: %swiftc_driver -enable-batch-mode -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift  -driver-print-jobs | %FileCheck %s -check-prefix=CHECK-COMBINED
+// RUN: %swiftc_driver -enable-batch-mode -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift  -###  | %FileCheck %s  -check-prefix=CHECK-COMBINED
+// RUN: %swiftc_driver -enable-batch-mode -driver-print-jobs -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift 2>%t/shouldBeEmpty >/dev/null && test -z `cat %t/shouldBeEmpty`


Why is this empty?

In the implementation, it is important to get -### to output on the expected file descriptor. Many existing tests depend on it. So I test for the right output on the right fd, and for no output on the wrong fd.

Other output flags write on the other fd.

Ahhh, I missed that. Maybe leave a comment? Or just do one invocation that writes both output streams to separate files, which makes it more obvious that they're being treated differently.

jrose-apple · 2018-03-05T20:02:37Z

test/Driver/batch_mode_print_jobs.swift

+// RUN: %swiftc_driver -enable-batch-mode -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift  -driver-print-jobs | %FileCheck %s -check-prefix=CHECK-COMBINED
+// RUN: %swiftc_driver -enable-batch-mode -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift  -###  | %FileCheck %s  -check-prefix=CHECK-COMBINED
+// RUN: %swiftc_driver -enable-batch-mode -driver-print-jobs -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift 2>%t/shouldBeEmpty >/dev/null && test -z `cat %t/shouldBeEmpty`
+// RUN: %swiftc_driver -enable-batch-mode -c -emit-module -module-name main -j 2 %t/file-01.swift %t/file-02.swift %t/file-03.swift %t/main.swift  -### 2>%t/shouldBeEmpty > /dev/null && test -z `cat %t/shouldBeEmpty`


Please put quotes around the sub-command, so that the argument doesn't get dropped entirely with a different shell.

Done! Thanks.

davidungar · 2018-03-05T21:26:46Z

@swift-ci please smoke test

davidungar · 2018-03-05T21:27:10Z

@swift-ci please smoke test

davidungar · 2018-03-06T00:15:31Z

@swift-ci please smoke test

davidungar · 2018-03-06T00:16:10Z

I've added a comment to explain the test better; it did need it, thanks. Also streamlined the test.
Edit: looks like I hadn't pushed it, till just now (7:15 PM).

davidungar · 2018-03-06T03:14:23Z

@swift-ci please smoke test

davidungar · 2018-03-06T17:53:59Z

@jrose-apple Thanks!

davidungar requested review from graydon, jrose-apple and abertelrud and removed request for abertelrud March 4, 2018 04:52

davidungar force-pushed the PR-18-10-HHH branch 4 times, most recently from d60eb6e to 524e59c Compare March 4, 2018 21:23

davidungar force-pushed the PR-18-10-HHH branch from 524e59c to cb42acd Compare March 5, 2018 00:15

davidungar force-pushed the PR-18-10-HHH branch from cb42acd to 3c36ce3 Compare March 5, 2018 01:15

davidungar force-pushed the PR-18-10-HHH branch from 3c36ce3 to f387381 Compare March 5, 2018 04:55

graydon approved these changes Mar 5, 2018

View reviewed changes

jrose-apple reviewed Mar 5, 2018

View reviewed changes

Restore -### functionality for batch-mode by extending OutputLevel.

17a81f6

davidungar force-pushed the PR-18-10-HHH branch from f387381 to 17a81f6 Compare March 5, 2018 19:07

jrose-apple reviewed Mar 5, 2018

View reviewed changes

davidungar force-pushed the PR-18-10-HHH branch from 8ccf9c1 to c4419d8 Compare March 5, 2018 21:27

Added a test.

1dd49c8

davidungar force-pushed the PR-18-10-HHH branch from c4419d8 to 1dd49c8 Compare March 6, 2018 03:14

jrose-apple approved these changes Mar 6, 2018

View reviewed changes

davidungar merged commit 90d2740 into swiftlang:master Mar 6, 2018

davidungar deleted the PR-18-10-HHH branch March 6, 2018 17:54

[Batch mode] Restore -### functionality for batch-mode by extending OutputLevel. #14961

[Batch mode] Restore -### functionality for batch-mode by extending OutputLevel. #14961

Uh oh!

Conversation

davidungar commented Mar 4, 2018

Uh oh!

davidungar commented Mar 4, 2018

Uh oh!

davidungar commented Mar 4, 2018

Uh oh!

davidungar commented Mar 4, 2018

Uh oh!

davidungar commented Mar 5, 2018

Uh oh!

davidungar commented Mar 5, 2018

Uh oh!

davidungar commented Mar 5, 2018

Uh oh!

graydon left a comment

Choose a reason for hiding this comment

Uh oh!

davidungar commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

davidungar commented Mar 5, 2018

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!

davidungar commented Mar 5, 2018

Uh oh!

davidungar commented Mar 5, 2018

Uh oh!

davidungar commented Mar 6, 2018

Uh oh!

davidungar commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented Mar 6, 2018

Uh oh!

davidungar commented Mar 6, 2018

Uh oh!

Uh oh!

davidungar commented Mar 5, 2018 •

edited

Loading

davidungar commented Mar 6, 2018 •

edited

Loading