Run POSIX pipes to interactive programs as the terminal's foreground job - #1765
Open
tleonhardt wants to merge 3 commits into
Open
tleonhardt wants to merge 3 commits into
tleonhardt wants to merge 3 commits into
Conversation
A pipe process always ran in its own session, so it never received the terminal. Piped to an interactive program such as less, Ctrl-Z and fg did not suspend and resume it together with cmd2, and the program could not take the keyboard as a shell's foreground job would. When a pipe's output goes to cmd2's controlling terminal and the pipe is started on the main thread, run the pipe process in a new process group and lend it the terminal as it starts, while cmd2 writes to it, and while cmd2 waits for it. Between writes the terminal returns to cmd2, so command code can still read the keyboard. ProcReader watches the group on a thread, relays its job-control stops to cmd2's job, and forwards Ctrl-C without signalling cmd2's own group again. A shell command piped to such a program joins the pipeline's job for as long as it runs. Pipes started off the main thread, or whose output cmd2 captures, still run in their own session. The terminal tests drive cmd2 under an interactive bash on a pseudo-terminal, using pyte to read the screen, and coverage now follows the applications those tests start. Extracted from the reserved-row toolbar work (PR #1761) without the toolbar.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
==========================================
+ Coverage 99.66% 99.74% +0.07%
==========================================
Files 23 23
Lines 5974 6232 +258
==========================================
+ Hits 5954 6216 +262
+ Misses 20 16 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
test_history_file_bad_compression and test_history_file_bad_json wrote to a fixed /tmp/doesntmatter, which on the Windows runner is D:\tmp and does not exist on a fresh machine. They passed only when the preceding permission-error test had already run: mocking builtins.open there does not stop the history setup from creating the file's parent directory, so that test created D:\tmp as a side effect. Under xdist the order is not fixed, and adding tests elsewhere shifted the schedule so both writers reached a worker first and failed on every Windows job. All three tests now use tmp_path, so none depends on a directory existing or on another test having run. Each passes alone on a single worker. Validation: 2587 passed, 6 skipped with coverage; make check, make test, make docs-test and git diff --check passed.
On Ubuntu 3.11 CI the outer shell once printed the old size after the test resized the pseudo-terminal while the job was stopped. Record each resize with the size read back straight after it, and include the size at the timeout, to tell a resize that never took effect from one undone later.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On POSIX, a command piped to an interactive program, such as
help -v | less, now runs that program as the terminal's foreground job, as a shell pipeline does.Today every pipe process starts in its own session and never receives the terminal. Ctrl-Z and
fgtherefore can't suspend and resume it together with cmd2, and it can't take the keyboard the way a shell's foreground job would.This work was developed on the reserved-row toolbar branch (#1761). It is extracted here, without the toolbar, so it can be reviewed and released on its own.
What changes
Terminal pipelines. When a pipe's output goes to cmd2's controlling terminal and the pipe starts on the main thread, the pipe process gets its own process group. cmd2 lends it the terminal:
Between writes the terminal returns to cmd2, so command code can still read the keyboard with
read_input(),select(),input(),getpassor raw reads.Job control.
ProcReaderwatches the pipeline's group on a thread and relays its Ctrl-Z stops to cmd2's own job, so the shell suspends and resumes both together. Ctrl-C is forwarded to the pipeline without signalling cmd2's group a second time.shellproducers. Ashellcommand piped to such a program (shell git log | less) joins the pipeline's job for as long as it runs. If the pipeline exits first, the command runs in cmd2's group as before.Unchanged cases. Pipes started off the main thread, or whose output cmd2 captures (for example in pyscripts), still run in their own session. Windows is unchanged.
Testing
tests/test_pipeline_job_control.pyruns cmd2 under an interactive bash on a pseudo-terminal and reads the screen with pyte. It covers Ctrl-C, Ctrl-Z andfg, wrapper shells, orphaned sessions, nested prompts, pagers setting modes at startup, andshellproducers.fgand interrupt cases fail. In the Ctrl-Z case,psshows the pager in its own session while the terminal's foreground group stays cmd2's.ProcReaderandPipelineWriterwere added totests/test_utils.py, and redirection edge cases totests/test_cmd2.py.pyteis added to thedevandtestdependency groups; it was already inuv.lock. Coverage now follows the cmd2 applications these tests start (patch = ["subprocess"]).--maxschedchunk=1spreads the slow terminal tests across workers.make check,make testandmake docs-testpass locally on macOS.Notes
/tmp/doesntmatter. With--maxschedchunk=1spreading tests across workers, those tests raced on Linux and failed on Windows, where that directory does not exist.