[RFC] Windows :terminal support #6383

erw7 · 2017-03-28T10:21:10Z

Implementation based on the work of equalsraf. Changes from the equalsraf work are as follows.

Changed uv_run to be called in pty_process_spawn because uv_connect_cb was not called before calling uv_read_start that caused assert.
Changed to poll using timer until uv_is_readable returns false because it does not stop process output polling until winpty-agent is finished.
Implemented emulate quotiong of libuv's arguments to build cmdline.
Changed by changing return value of pty_process_spawn.
Changed to set proc->pid.
Changed enable some test on windows.
Changed to use utf16_to_utf8 function implemented in mbyte.c.

We do not think that the place to quote the command line is optimal, so we need to consider it.

If the last sleep of tty-test.c is enabled, the following error occurs in the functionaltest.

-- Output to stderr:
Uncaught Error: attempt to call a number value
stack traceback:
        [C]: at 0x67191e2e

In the case of Windows, I disabled it, is it really necessary to sleep for 100 seconds even in other environments?

At the moment there are the following problems.

~~Nvim will crash in the next step.~~
- ~~nvim-qt.exe -- -u NONE~~
- ~~:terminal~~
- :q
~~There is no problem in the case of the following step.~~
- ~~nvim.exe -u NONE~~
- ~~nvim-qt.exe --server \\.\pipe\nvim-xxxx-0~~
- ~~:terminal~~
- :q
~~In this case the errorlevel is also 0.~~
~~In the following procedure E301: Oops, lost the swap file !!! error will occur.~~
- ~~nvim-qt.exe -- -u NONE~~
- ~~:e test~~
- ~~:call termopen("dir")~~

equalsraf · 2017-03-28T11:53:25Z

@erw7 you can remove 2c84479 and 74b6348 they are already in the master branch.

justinmk · 2017-03-28T12:08:03Z

Can you rebase this on master?

equalsraf · 2017-03-28T12:51:31Z

Can you rebase this on master?

Some conflicts but the rebase works away that merge commit, see f877eb6

erw7 · 2017-03-28T14:19:08Z

How can I remove it. I'm sorry I was not used to git etc.

brcolow · 2017-03-29T00:05:29Z

To remove the commits do:

git rebase -i HEAD~7 which will open up the editor you have configured with git.

If you don't have an editor configured yet, check out this stackoverflow

Now change the word pick before the 2c84479 and 74b6348 commits to drop, write the file and quit. Then do git push --force. At least, I think that's what you should do - not a git aficionado by any means and I frequently mess up.

erw7 · 2017-03-29T09:24:54Z

Thank you brcolow.
I tried it. 08661d3 to 439c203 are entering, but is this OK? Should I git rebase -i HEAD~6 and 08661d3 to 439c203 also need to drop?

justinmk · 2017-03-29T10:47:00Z

git rebase -i upstream/master

Or origin/master

erw7 · 2017-03-29T11:14:14Z

I tried it, but this time 8f0aa5d has disappeared. I thought about going back with git reflog, but once I deleted all the local files, it does not remain. May I do as follows

git checkout -b patch-windows-terminal2  equalsraf/patch-windows-terminal
git pull -f origin patch-windows-terminal

justinmk · 2017-03-29T11:23:00Z

With your current branch, just do git cherry-pick 8f0aa5d and it will bring that commit over.

erw7 · 2017-03-29T11:41:31Z

I tried it, but it becomes as follows.

>git cherry-pick 8f0aa5df
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'
On branch patch-windows-terminal
Your branch is up-to-date with 'origin/patch-windows-terminal'.
You are currently cherry-picking commit 8f0aa5df.

nothing to commit, working tree clean
>git status
On branch patch-windows-terminal
Your branch is up-to-date with 'origin/patch-windows-terminal'.
You are currently cherry-picking commit 8f0aa5df.
  (all conflicts fixed: run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

nothing to commit, working tree clean

I created another branch and tried git commit --allow-empty, 8f0aa5d came back, but it was empty.

I am sorry for taking care of it.

erw7 · 2017-03-29T12:25:20Z

After canceling the same change at 57b1dcd git cherry-pick 8f0aa5d, 8f0aa5d returned.
thanks a lot.

justinmk · 2017-03-30T21:51:50Z

This works. ~~Running vim in bash --login -i from git-for-windows 1.8.3 has messed up colors,~~ but this is a good start!

erw7 · 2017-03-30T22:52:16Z

What if you start vim as follows?

bash --login -i
export VIM=/usr/share/vim
export VIMRUNTIME=/usr/share/vim/vim80
vim

I finished this work for now.

justinmk · 2017-03-30T23:00:27Z

The vim colors issue was bogus, same issue happens in normal cmd.exe (I didn't think to check there because I normally run a different version of vim there, not the old git-for-windows vim).

Thanks for working on this!

erw7 · 2017-03-31T08:27:32Z

There are many points that do not follow the coding guide. I'll fix them.

Please remove RFC label temporarily.

ZyX-I · 2017-03-31T12:04:54Z

@erw7 Labels are managed by bot, based on the PR title.

justinmk · 2017-04-03T13:21:50Z

src/nvim/os/pty_process_win.c

@@ -102,6 +107,7 @@ int pty_process_spawn(PtyProcess *ptyproc)
      NULL,  // Optional environment variables
      &err);
  if (spawncfg == NULL) {
+    write_winpty_elog("Faied winpty_spawn_config_new.", &status, &err);


justinmk · 2017-04-03T13:26:45Z

src/nvim/os/pty_process_win.c

+  FUNC_ATTR_NONNULL_ALL
+{
+  PtyProcess *ptyproc =
+    (PtyProcess *)((uv_handle_t *)wait_eof_timer->data);


we don't cast void * pointers, instead just:

PtyProcess *ptyproc = wait_eof_timer->data;

justinmk · 2017-04-04T11:04:26Z

test/functional/fixtures/tty-test.c

-  DWORD dwProcessId;
-  GetWindowThreadProcessId(consoleWnd, &dwProcessId);
-  return GetCurrentProcessId() == dwProcessId;
+  // XXX: We need to make proper detect owns tty


is this planned for this PR, or later?

I do not plan on this PR.
I do not understand what to check here on Windows.

justinmk · 2017-04-04T11:29:50Z

test/functional/fixtures/tty-test.c


  for (int i = 0; i < cnt; i++) {
    if (buf->base[i] == 3) {
      (*interrupted)++;
+    } else if (buf->base[i] == 17) {
+      prsz = true;


could you add a comment mentioning where 17 comes from? Or make it a #define.

justinmk · 2017-08-06T11:41:50Z

src/nvim/CMakeLists.txt

@@ -118,6 +118,10 @@ endforeach()

 list(REMOVE_ITEM NVIM_SOURCES ${to_remove})

+if (NOT WIN32)
+  list(REMOVE_ITEM NVIM_HEADERS "${PROJECT_SOURCE_DIR}/src/nvim/os/pty_process_win.h")
+endif()


@erw7 Do you happen to remember why this was needed? I removed it in b885b93 and it seems to build OK on linux, bsd, and macOS.

I see now it fails the "single includes" test. Addressed that in 90de721

Failed "single include" test as you say.
Will you work #7007 after this? Then I close this pull request.

@erw7 Yes, #7007 is almost ready, it's an effort to merge your work here.

Handling of process exit is still broken. It detects the moment when the child process exits, then quickly stops polling for process output. It should continue polling for output until the agent has scraped all of the process' output. This problem is easy to noticed by running a command like "dir && exit", but even typing "exit<ENTER>" can manifest the problem -- the "t" might not appear. winpty's Cygwin adapter handles shutdown by waiting for the agent to close the CONOUT pipe, which it does after it has scraped the child's last output. AFAIK, neovim doesn't do anything interesting when winpty closes the CONOUT pipe.

Changing to make sure that proc->in, proc-> is not NULL, because nvim crashed when starting a job with pty. Changing to make sure that proc->out is not NULL, because nvim crashed when stopping a job opened with pty.

Remove pty_process_win.h from NVIM_HEADERS on *nix.

tty-test.exe causes abnormal termination with low repeatability, try changing it so as not to use SIGWINCH.

This reverts commit 73b2477.

justinmk · 2017-08-07T23:17:43Z

#7007 will merge this. Many thanks to @erw7 !

References #6383 Closes #4748

erw7 changed the title ~~[WIP/RFC] Windows :terminal support~~ [RFC] Windows :terminal support Mar 30, 2017

marvim added the RFC label Mar 30, 2017

erw7 changed the title ~~[RFC] Windows :terminal support~~ [WIP/RFC] Windows :terminal support Mar 31, 2017

justinmk changed the title ~~[WIP/RFC] Windows :terminal support~~ [WIP] Windows :terminal support Mar 31, 2017

marvim added WIP and removed RFC labels Mar 31, 2017

justinmk added the platform:windows label Mar 31, 2017

erw7 changed the title ~~[WIP] Windows :terminal support~~ [RFC] Windows :terminal support Apr 3, 2017

marvim added RFC and removed WIP labels Apr 3, 2017

justinmk reviewed Apr 3, 2017

View reviewed changes

justinmk reviewed Apr 4, 2017

View reviewed changes

justinmk reviewed Aug 6, 2017

View reviewed changes

rprichard and others added 23 commits August 8, 2017 02:27

Install winpty-agent.exe with nvim

dcbe05b

Windows :terminal support

38b2f13

Fixed problems with jobstart and jobstop

86b7716

Changing to make sure that proc->in, proc-> is not NULL, because nvim crashed when starting a job with pty. Changing to make sure that proc->out is not NULL, because nvim crashed when stopping a job opened with pty.

Fix to fail build on *nix

89cafd1

Remove pty_process_win.h from NVIM_HEADERS on *nix.

Add boundary check to quote_cmd_arg

107985e

Fix clint issues

5ab1e73

Fix Travis CI issue

bd60a95

Changed to output screen size with <C-q> key

64057aa

tty-test.exe causes abnormal termination with low repeatability, try changing it so as not to use SIGWINCH.

Change to follow coding guide

304eedc

Add error log output

b688bab

Change return value of pty_process_win

df37d7b

Fix the issue that reviewer pointed out

505f0a1

Add definition of CTRL_Q, CTRL_C

56be21a

Change to retry from wait

c59f1ce

Improve error code translation

a5630df

Remove write_winpty_elog and write_elog function

7a5e90b

Change to not quoting for cmd.exe

d7f4dfe

Change to use feed_data

91a600b

Increase retry timeout

995fcc1

Change from \13 to \r

a5a4d07

Revert "Fix to fail build on *nix"

0abf3c4

This reverts commit 73b2477.

single-includes: ignore os/pty_process_win.h

331ea27

justinmk closed this Aug 7, 2017

justinmk removed the RFC label Aug 7, 2017

justinmk added a commit that referenced this pull request Aug 16, 2017

Merge #7007 'Windows :terminal'

5b32bce

References #6383 Closes #4748

justinmk mentioned this pull request Aug 22, 2017

Neovim crashes when stopping a PTY job in Windows #6143

Closed

erw7 deleted the patch-windows-terminal branch September 11, 2019 05:37

Uh oh!

[RFC] Windows :terminal support #6383

[RFC] Windows :terminal support #6383

Uh oh!

Conversation

erw7 commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

equalsraf commented Mar 28, 2017

Uh oh!

justinmk commented Mar 28, 2017

Uh oh!

equalsraf commented Mar 28, 2017

Uh oh!

erw7 commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brcolow commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erw7 commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 29, 2017

Uh oh!

erw7 commented Mar 29, 2017

Uh oh!

justinmk commented Mar 29, 2017

Uh oh!

erw7 commented Mar 29, 2017

Uh oh!

erw7 commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erw7 commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Mar 30, 2017

Uh oh!

erw7 commented Mar 31, 2017

Uh oh!

ZyX-I commented Mar 31, 2017

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!

erw7 Aug 7, 2017 • 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!

justinmk commented Aug 7, 2017

Uh oh!

Uh oh!

erw7 commented Mar 28, 2017 •

edited

Loading

erw7 commented Mar 28, 2017 •

edited

Loading

brcolow commented Mar 29, 2017 •

edited

Loading

erw7 commented Mar 29, 2017 •

edited

Loading

erw7 commented Mar 29, 2017 •

edited

Loading

justinmk commented Mar 30, 2017 •

edited

Loading

erw7 commented Mar 30, 2017 •

edited

Loading

erw7 Aug 7, 2017 •

edited

Loading