startup: Make "-s -" read from stdin #6299

ZyX-I · 2017-03-16T21:05:59Z

No description provided.

ZyX-I · 2017-03-16T21:12:23Z

test/functional/core/main_spec.lua

+      end
+      neq(0, max_sec)
+    end)
+    it('does not expand $VAR', function()


This test is there because -S is not working well with such names (-S arg uses a concat of source and arg without any escaping used). -s is treated different and does not have the problem, but this option looks similar to -S.

Check whether `repeated_read_cmd` returned nil and did not actually run nvim or it did, but still returned nil for whatever reason.

oni-link · 2017-03-19T11:35:24Z

With nvim -u NONE -s - and :qa I get this:

nvim: /home/oni-link/git/neovim/.deps/build/src/libuv/src/unix/core.c:521: uv__close: Assertion `fd > STDERR_FILENO' failed.
Aborted

ZyX-I · 2017-03-19T12:57:33Z

@oni-link Interesting, I can reproduce this in tmux. I can’t reproduce it when running in Neovim terminal with exactly the same arguments (nvim_prog_abs(), '-u', 'NONE', '-i', 'NONE', '--cmd', 'set noswapfile shortmess+=IFW fileformats=unix', '-s', '-'). Note: it crashes on :, before I can type q.

Solution is obvious currently: use dup() to clone STDIN_FILENO and use the resulting fd. Though will need to use os/fileio.c in place of f* functions: fdopen is not in C99.

ZyX-I · 2017-03-19T13:01:50Z

A bit different: I can reproduce this in termopen(), but either if I run it in an interactive shell or if I actually close inner Neovim, in the last case problem appears not when typing :, but during shutdown.

This variant uses `fdopen()` which is not standard, but it fixes problem on my system. In next commit `scriptin` will use `FileDescriptor*` from os/fileio in place of `FILE*`.

Problem: as fileio is cached and reads blocks this is going to wait until either EOF or reading enough characters to fill rbuffer. This is not good when reading user input from stdin as script.

justinmk · 2017-03-23T09:30:42Z

Windows build hangs, termopen() won't work there, yet.

justinmk · 2017-03-23T09:31:35Z

test/functional/core/main_spec.lua

+      local attrs = lfs.attributes(fname)
+      eq(#('100500\n'), attrs.size)
+    end)
+    it('does not crash after reading from stdin in non-headless mode', function()


Add

if helpers.pending_win32(pending) then return end

here, since this test uses termopen().

termopen() works on Windows now, so the above comment is outdated. Though reading from stdin on Windows may be broken (#6890) so maybe it's expedient to skip it for now.

ZyX-I · 2017-03-23T18:02:47Z

It hangs on the very first test, and that does not use termopen(), it uses system().

Assume something with system() if second test hangs as well. Assume something with reading stdin if not.

Code imported from neovim#6299

justinmk · 2017-12-03T13:12:41Z

src/nvim/os/fileio.c

+/// @param[in]  fd  File descriptor to wrap.
+/// @param[in]  flags  Flags, @see FileOpenFlags. Currently reading from and
+///                    writing to the file at once is not supported, so either
+///                    FILE_WRITE_ONLY or FILE_READ_ONLY is required.


should this be:

kFileWriteOnly or kFileReadOnly is required.

justinmk · 2017-12-03T13:12:58Z

src/nvim/os/fileio.h

@@ -32,6 +33,8 @@ typedef enum {
                       ///< kFileCreateOnly.
  kFileAppend = 64,  ///< Append to the file. Implies kFileWriteOnly. Cannot
                     ///< be used with kFileCreateOnly.
+  kFileNonBlocking = 128,  ///< Do not restart read() or write() syscall if
+                           ///< EAGAIN was encountered.


"EAGAIN or EINTR" ?

EAGAIN only.

ZyX-I · 2017-12-03T14:46:36Z

Looks like

[ RUN      ] Command-line option -s does not expand $VAR:

hang again.

ZyX-I · 2017-12-03T14:50:31Z

If I recall correctly that is “using -s always hangs” rather then something specific to $fname.

justinmk · 2017-12-03T15:06:58Z

Is it a regression or is it just a new test that can be skipped for now?

ZyX-I · 2017-12-03T15:39:57Z

It is a new test, but I am not aware of the similar test that checked something before .

BTW, is there some script to build Neovim for wine? E.g. like I did for Vim once: https://bitbucket.org/ZyX_I/vim-dev-tools/src/342d10f67bbbb072288b80d0c7effb2507efe21b/winesetup.pl?at=default prepares WINEPREFIX (also builds Vim), specifically build.zsh near that uses mingw to build vim.

justinmk · 2017-12-04T06:40:26Z

BTW, is there some script to build Neovim for wine?

Not that I know.

ZyX-I · 2017-12-08T21:19:26Z

I started working on that, but soon found luarocks/luarocks#749. Most likely that means that quick “just make luarocks compile things for me” is out and all needed packages would be cross-compiled, or alternatives (like lullpeg) found, though I will try to determine why os.execute is incompatible on wine to that extent. Currently I can say that

luajit -e "print(os.execute(\"\\\"C:\\luarocks\\tools\\mkdir\\\" -p \\\"abc\\\"\"))"

fails, but succeeds without one pair of quotes. Reported two bugs to wine as a result: https://bugs.winehq.org/show_bug.cgi?id=44133 and https://bugs.winehq.org/show_bug.cgi?id=44132.

janlazo · 2017-12-09T00:10:58Z

Is the following the intended command?

> "C:\luarocks\tools\mkdir\\" -p "abc"

Invoking cmd.exe (without explicit escaping) should be

cmd.exe /s/c ""C:\luarocks\tools\mkdir\\" -p "abc""

Seems to be a bug in os.execute if it's using /c

justinmk · 2017-12-09T01:19:17Z

@ZyX-I If you have a RDP client you can connect to appveyor for 1-2 hours, useful for sanity checks. Requires a temporary change to appveyor.yml, a bit annoying but it works well. https://www.appveyor.com/docs/how-to/rdp-to-build-worker/

justinmk · 2017-12-10T13:19:21Z

After ignoring the "does not expand $VAR" test in #7713 now the "old tests" hang (since #7705 , the Vim "old tests" run on AppVeyor).

This hangs:

VIMRUNTIME=../../../runtime; export VIMRUNTIME;  ../../../build/bin/nvim.exe -u unix.vim -U NONE -i viminfo --headless --noplugin -s dotest.in test1.in

So there is a regression in the behavior of -s on Windows.

justinmk · 2018-04-13T01:25:21Z

src/nvim/main.c

-              mch_errmsg("\"\n");
+            int error;
+            if (STRCMP(argv[0], "-") == 0) {
+              const int stdin_dup_fd = os_dup(OS_STDIN_FILENO);


@ZyX-I maybe #8267 is helpful to implement os_dup on Windows.

justinmk · 2018-04-13T01:28:20Z

test/functional/core/main_spec.lua

+  end
+end
+
+describe('Command-line option', function()


this could go in startup_spec.lua

marvim added the RFC label Mar 16, 2017

ZyX-I commented Mar 16, 2017

View reviewed changes

ZyX-I added 9 commits March 19, 2017 14:13

main: Make -s - read from stdin

65c41e6

functests: Check that -s works as expected

0320a58

tests: Fix CI failures

0e9286a

tests: Fix repeated_popen_r usage, rename the function

3cd7bf3

functests: Alter the order of checks

1ea7541

Check whether `repeated_read_cmd` returned nil and did not actually run nvim or it did, but still returned nil for whatever reason.

functests: Make sure that line ending is LF and not CRLF

38687ee

functests: Use Neovim instance and system() in place of lua io.popen

d4639ea

functests: Fix testlint errors

29654cf

functests: Do not check stdout, it is different on Windows

d2268d5

ZyX-I force-pushed the s-dash-stdin branch from e9bc2ed to d2268d5 Compare March 19, 2017 11:13

ZyX-I added 8 commits March 19, 2017 16:09

main: Temporary fix assertion error

fdfa1ed

This variant uses `fdopen()` which is not standard, but it fixes problem on my system. In next commit `scriptin` will use `FileDescriptor*` from os/fileio in place of `FILE*`.

getchar: Use fileio instead of fdopen

bd798a3

Problem: as fileio is cached and reads blocks this is going to wait until either EOF or reading enough characters to fill rbuffer. This is not good when reading user input from stdin as script.

fileio,main: Do not restart syscall at EAGAIN when reading for -s

e78e75d

*: Fix linter errors

88e1a17

os/fileio: Fix QB failure

83b39a4

functests: Test -s errors

7df4fc8

unittests: Add tests for new fileio functions

ae4fae9

main: Translate full -s error message, not part of it

ca116df

justinmk reviewed Mar 23, 2017

View reviewed changes

ZyX-I added 2 commits March 23, 2017 21:04

functests: Do not run termopen test on Windows

99b4f25

functests: Disable system(-s -) test on Windows

62108c3

Assume something with system() if second test hangs as well. Assume something with reading stdin if not.

ZyX-I changed the title ~~[RFC] Make -s - read from stdin~~ [WIP] Make -s - read from stdin Apr 9, 2017

marvim removed the RFC label Apr 9, 2017

marvim added the WIP label Apr 9, 2017

ZyX-I added a commit to ZyX-I/neovim that referenced this pull request Jul 4, 2017

os/fileio: Add ability to use os/fileio.c for file descriptors

f296845

Code imported from neovim#6299

ZyX-I added a commit to ZyX-I/neovim that referenced this pull request Jul 4, 2017

os/fileio: Add ability to use os/fileio.c for file descriptors

72b3fd9

Code imported from neovim#6299

justinmk mentioned this pull request Dec 3, 2017

startup: treat stdin as text instead of commands #7679

Merged

justinmk reviewed Dec 3, 2017

View reviewed changes

ZyX-I added 2 commits December 3, 2017 16:49

Merge branch 'master' into s-dash-stdin

c49e22d

getchar: Move REMAP_… values into a enum

78bc52e

justinmk mentioned this pull request Dec 10, 2017

[test] #7713

Closed

justinmk mentioned this pull request Apr 13, 2018

[RDY] win: Fix reading content from stdin #8267

Merged

justinmk reviewed Apr 13, 2018

View reviewed changes

test/functional/core/main_spec.lua

end

end

describe('Command-line option', function()

Copy link

Member

justinmk Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could go in startup_spec.lua

b-r-o-c-k mentioned this pull request Apr 14, 2018

Make -s - read from stdin #8276

Merged

justinmk changed the title ~~[WIP] Make -s - read from stdin~~ Make -s - read from stdin Apr 16, 2018

justinmk merged commit 78bc52e into neovim:master Apr 17, 2018

justinmk removed the WIP label Apr 17, 2018

ZyX-I deleted the s-dash-stdin branch May 1, 2018 17:45

justinmk changed the title ~~Make -s - read from stdin~~ startup: Make "-s -" read from stdin May 21, 2018

Uh oh!

startup: Make "-s -" read from stdin #6299

startup: Make "-s -" read from stdin #6299

Uh oh!

Conversation

ZyX-I commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oni-link commented Mar 19, 2017

Uh oh!

ZyX-I commented Mar 19, 2017

Uh oh!

ZyX-I commented Mar 19, 2017

Uh oh!

justinmk commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZyX-I commented Mar 23, 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!

ZyX-I commented Dec 3, 2017

Uh oh!

ZyX-I commented Dec 3, 2017

Uh oh!

justinmk commented Dec 3, 2017

Uh oh!

ZyX-I commented Dec 3, 2017

Uh oh!

justinmk commented Dec 4, 2017

Uh oh!

ZyX-I commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janlazo commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Dec 9, 2017

Uh oh!

justinmk commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZyX-I commented Dec 8, 2017 •

edited

Loading

janlazo commented Dec 9, 2017 •

edited

Loading