[Basic] Add TerminalController class #502

aciidgh · 2016-07-15T14:49:17Z

I was able to implement a lit-style progress bar without using curses (yay!). Currently how it works is TerminalController takes any object conforming to OutputStream and exposes APIs to perform operations like write with colour and cursor movement on it. If the output stream is not a tty then TerminalController will ignore the tty operations and write in plain text. This PR is not fully complete yet, needs tests and some calculation fixing but works. I want to get feedback for the APIs and direction.

aciidgh · 2016-07-15T14:53:36Z

Will need to refactor ColorWrap and Verbosity utilities once this is in

ddunbar · 2016-07-15T15:24:48Z

Sources/Basic/TerminalController.swift

+
+    /// Returns the color code which can be prefixed on a string to display it in that color.
+    /// Note: To avoid unexpected behavior the color codes are private and not exposed to clients
+    /// other than TerminalController.


What is the unexpected behavior you are worried about here?

Err that comment came out wrong, I meant to write there is no need for clients to know about this string because they shouldn't be manually inserting the color code in string. I'll reword

ddunbar · 2016-07-15T15:53:15Z

This is very cool.

I have one major annoying request, which is that we have some test coverage for this. It's annoying enough that I fear if we don't do it initially, it won't get done and this is tricky enough that I'd like to know we have some coverage.

NOTE: You can open a PTY (openpty) in order to be able to pass a TTY capable file descriptor to a client. That may be enough to fake it out into doing somewhat more complicated things which can be tested against.

aciidgh · 2016-07-15T15:56:31Z

Yeah I was planning to write tests, at least writing to a string stream tests, as mentioned in the PR desc. I wanted to know if the class makes sense first.
Thanks, will look into openpty

aciidgh · 2016-07-18T09:29:30Z

@ddunbar Updated this with tests

aciidgh · 2016-07-18T09:33:44Z

Sources/Basic/TerminalController.swift

+}
+
+/// Creates colored or simple progress bar based on output stream provided stream.
+public func createProgressBar<Target: OutputStream>(forStream stream: UnsafeMutablePointer<Target>, header: String) -> ProgressBarProtocol {


@ddunbar Not sure if this is right place for this method

aciidgh · 2016-07-18T20:49:45Z

Sources/Basic/TerminalController.swift

    private let header: String
    private var isClear: Bool
-    private var stream: UnsafeMutablePointer<Target>
+    private var stream: OutputByteStream


@ddunbar I am not sure which is better here, I think this operating on OutputByteStream makes more sense but a generic OutputStream will allow us to stream to things like strings too. I'll fixup or remove this commit once you review.

ddunbar · 2016-07-19T15:52:20Z

Sources/Basic/TerminalController.swift

+    /// Constructs the instance if the stream is a tty.
+    public init?(stream: LocalFileOutputByteStream) {
+        // Make sure this file stream is tty.
+        guard !(isatty(fileno(stream.fp)) == 0) else {


aciidgh · 2016-08-01T10:28:51Z

@ddunbar updated

ddunbar · 2016-08-01T16:07:47Z

Sources/Basic/TerminalController.swift

+    }
+
+    /// Moves the cursor y columns up.
+    public func moveCursor(y: Int) {


BTW, we should probably find a better label here, if this is relative. It reads like an absolute position. I don't have any particular suggestion, the best I could come up with is moveCursor(deltaY y:) or moveCursor(up y: Int).

moveCursor(up y: Int) seems good

ddunbar · 2016-08-01T16:09:26Z

LGTM!!

ddunbar · 2016-08-01T16:15:49Z

I think this is missing some imports in the relocated tests:

Compile Swift Module 'UtilityTests' (8 sources)
/home/ddunbar/public/swift-project/swiftpm/Tests/UtilityTests/ProgressBarTests.swift:27:12: error: use of unresolved identifier 'openpty'
        if openpty(&master, &slave, nil, nil, nil) != 0 {
           ^~~~~~~
Glibc.openat:1:13: note: did you mean 'openat'?
public func openat(_ fd: Int32, _ path: UnsafePointer<CChar>, _ oflag: Int32) -> Int32
            ^
Glibc.openat:1:13: note: did you mean 'openat'?
public func openat(_ fd: Int32, _ path: UnsafePointer<CChar>, _ oflag: Int32, _ mode: mode_t) -> Int32
            ^
SwiftGlibc.openat:2:13: note: did you mean 'openat'?
public func openat(_ __fd: Int32, _ __file: UnsafePointer<Int8>, _ __oflag: Int32, _ varargs: Any...) -> Int32
            ^
/home/ddunbar/public/swift-project/swiftpm/Tests/BasicTests/TerminalControllerTests.swift:23:12: error: use of unresolved identifier 'openpty'
        if openpty(&master, &slave, nil, nil, nil) != 0 {
           ^~~~~~~
Glibc.openat:1:13: note: did you mean 'openat'?
public func openat(_ fd: Int32, _ path: UnsafePointer<CChar>, _ oflag: Int32) -> Int32
            ^
Glibc.openat:1:13: note: did you mean 'openat'?
public func openat(_ fd: Int32, _ path: UnsafePointer<CChar>, _ oflag: Int32, _ mode: mode_t) -> Int32
            ^
SwiftGlibc.openat:2:13: note: did you mean 'openat'?
public func openat(_ __fd: Int32, _ __file: UnsafePointer<Int8>, _ __oflag: Int32, _ varargs: Any...) -> Int32
            ^
<unknown>:0: error: build had 2 command failures
swift-build-stage1: error: exit(1): /home/ddunbar/public/swift-project/build/Ninja-ReleaseAssert/swiftpm-linux-x86_64/debug/swift-build-tool -f /home/ddunbar/public/swift-project/build/Ninja-ReleaseAssert/swiftpm-linux-x86_64/debug.yaml test
--- bootstrap: error: build failed with exit status 1

aciidgh · 2016-08-01T16:16:54Z

huh, checking

aciidgh · 2016-08-02T08:59:59Z

This will start building on liunx after swiftlang/swift#3926 is in

jdhealy · 2016-08-02T17:04:19Z

Sources/Utility/ProgressBar.swift

+        let barWidth = term.width - prefix.utf8.count
+        let n = Int(Double(barWidth) * Double(percent)/100.0)
+
+        term.write("=".repeating(n: n) + "-".repeating(n: barWidth - n), inColor: .green)


Fatal errors from subtraction resulting in negative Int arguments being < lowerBounds in String.repeating have me nervous.

Especially when prefix is always at least a couple of characters long, precautions against one column terminals seem necessary. (Of course, doubt that anyone would desire a one column terminal 😛 , but one might result briefly from a resize — more so from tmux or similar than from Terminal.app.)

I’d even reason that super small terminals need a different strategy — just clamping the Ints to a floor of 0 might produce unattractive results.

Love the direction of this PR though, cool stuff!

Good point, this does sometimes come up especially if we honor the emacs COLUMNS since that can sometimes drop to 0. I know we fixed an issue in Clang once where that crashed at some point.

I think returning same string (or empty string) from repeating should be good enough for such cases.

aciidgh · 2016-08-05T17:10:28Z

@swift-ci please test

aciidgh · 2016-08-05T19:11:43Z

The swiftpm tests succeeded but looks like there is some mistake in the linux ci script (unrelated to this patch)

[xUnit] [INFO] - [JUnit] - No test report file(s) were found with the pattern 'buildbot_incremental/swift-macosx-x86_64/swift-test-results/**/*.xml' relative to '/Users/buildnode/jenkins/workspace/swift-package-manager-PR-osx' for the testing framework 'JUnit'.  Did you enter a pattern relative to the correct directory?  Did you generate the result report(s) for 'JUnit'?
[xUnit] [ERROR] - No test reports found for the metric 'JUnit' with the resolved pattern 'buildbot_incremental/swift-macosx-x86_64/swift-test-results/**/*.xml'. Configuration error?.
[xUnit] [INFO] - Failing BUILD.
[xUnit] [INFO] - There are errors when processing test results.

ddunbar · 2016-08-06T18:09:02Z

I hit:

FAIL: swiftpm :: SwiftPMPackageTests.xctest/UtilityTests.ProgressBarTests/testProgressBar (277 of 292)
******************** TEST 'swiftpm :: SwiftPMPackageTests.xctest/UtilityTests.ProgressBarTests/testProgressBar' FAILED ********************
Command: /home/ddunbar/public/swift-project/swiftpm/../build/Ninja-ReleaseAssert/swiftpm-linux-x86_64/debug/SwiftPMPackageTests.xctest UtilityTests.ProgressBarTests/testProgressBar
Test Suite 'Selected tests' started at 17:48:47.840
Test Suite 'ProgressBarTests' started at 17:48:47.848
Test Case 'ProgressBarTests.testProgressBar' started at 17:48:47.849
/home/ddunbar/public/swift-project/swiftpm/Tests/UtilityTests/ProgressBarTests.swift:80: error: ProgressBarTests.testProgressBar : XCTAssertTrue failed - 
Test Case 'ProgressBarTests.testProgressBar' failed (0.001 seconds).
Test Suite 'ProgressBarTests' failed at 17:48:47.850
     Executed 1 test, with 1 failure (0 unexpected) in 0.001 (0.001) seconds
Test Suite 'Selected tests' failed at 17:48:47.850
     Executed 1 test, with 1 failure (0 unexpected) in 0.001 (0.001) seconds
Exit Status: 1
********************

with this running locally.

aciidgh · 2016-08-06T18:25:00Z

I think it ran the wrong bundle -> SwiftPMPackageTests.xctest (or both bundles) since this commit didn't have that change, but rebased this now. Though just to be sure, clean your build folder 😈

ddunbar · 2016-08-07T06:20:22Z

I think it is a race condition, the thread.join should be before the output is checked, no?

aciidgh · 2016-08-08T08:33:46Z

ah yes. fixed
@swift-ci please test

aciidgh · 2016-08-08T13:46:49Z

this time OSX script has some issue, all tests succeeded tho. @ddunbar can you recheck locally again

ddunbar · 2016-08-09T05:07:16Z

I still see Linux test failures sometimes... after looking at the code, I think the problem is that the pty can be closed before all the data is read off the pipe. As written, the client side needs to be closed before the thread join.

aciidgh · 2016-08-09T08:38:07Z

Thanks! you're right, closing master fd before reading from it was causing the failure on linux. I wonder why CI din catch this..

aciidgh · 2016-08-09T08:38:14Z

@swift-ci please test

aciidgh assigned abertelrud and ddunbar Jul 15, 2016

ddunbar reviewed Jul 15, 2016
View reviewed changes

aciidgh force-pushed the term-controller branch 2 times, most recently from 36569c5 to 531e8c1 Compare July 18, 2016 09:28

aciidgh changed the title ~~[WIP][Basic] Add TerminalController class~~ [Basic] Add TerminalController class Jul 18, 2016

aciidgh reviewed Jul 18, 2016
View reviewed changes

aciidgh force-pushed the term-controller branch 2 times, most recently from 1b0c270 to 5fa2c28 Compare July 18, 2016 20:39

aciidgh reviewed Jul 18, 2016
View reviewed changes

aciidgh force-pushed the term-controller branch from da757f7 to c23274c Compare July 18, 2016 20:59

ddunbar reviewed Jul 19, 2016
View reviewed changes

ddunbar added the pending label Jul 19, 2016

aciidgh force-pushed the term-controller branch from 1681d75 to 3f1a504 Compare August 1, 2016 10:28

aciidgh assigned ddunbar and unassigned aciidgh Aug 1, 2016

ddunbar reviewed Aug 1, 2016
View reviewed changes

ddunbar removed their assignment Aug 1, 2016

ddunbar assigned aciidgh Aug 1, 2016

jdhealy reviewed Aug 2, 2016
View reviewed changes

ddunbar added the pending label Aug 6, 2016

aciidgh force-pushed the term-controller branch 2 times, most recently from d1c39d5 to 2d046ed Compare August 6, 2016 18:24

aciidgh force-pushed the term-controller branch from 2d046ed to 344e113 Compare August 8, 2016 08:33

[Basic] Add TerminalController class

b2a392d

aciidgh force-pushed the term-controller branch from 344e113 to b2a392d Compare August 9, 2016 08:33

ddunbar merged commit b2a392d into swiftlang:master Aug 10, 2016

aciidgh deleted the term-controller branch September 2, 2016 16:18

[Basic] Add TerminalController class #502

[Basic] Add TerminalController class #502

Uh oh!

Conversation

aciidgh commented Jul 15, 2016

Uh oh!

aciidgh commented Jul 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddunbar commented Jul 15, 2016

Uh oh!

aciidgh commented Jul 15, 2016

Uh oh!

aciidgh commented Jul 18, 2016

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!

aciidgh commented Aug 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddunbar commented Aug 1, 2016

Uh oh!

ddunbar commented Aug 1, 2016

Uh oh!

aciidgh commented Aug 1, 2016

Uh oh!

aciidgh commented Aug 2, 2016

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!

aciidgh commented Aug 5, 2016

Uh oh!

aciidgh commented Aug 5, 2016

Uh oh!

ddunbar commented Aug 6, 2016

Uh oh!

aciidgh commented Aug 6, 2016

Uh oh!

ddunbar commented Aug 7, 2016

Uh oh!

aciidgh commented Aug 8, 2016

Uh oh!

aciidgh commented Aug 8, 2016

Uh oh!

ddunbar commented Aug 9, 2016

Uh oh!

aciidgh commented Aug 9, 2016

Uh oh!

aciidgh commented Aug 9, 2016

Uh oh!

Uh oh!