[RDY] Job control fixes and pseudo terminal support #2037

tarruda · 2015-02-21T19:01:05Z

The end goal is to spawn bang commands connected to pseudo terminals, which will fix issues such as #1716. For windows an alternative implementation of pty_process.c will be required, probably on top of winpty

ghost · 2015-02-21T21:50:56Z

src/nvim/os/pipe_process.c

+} UvProcess;
+
+void pipe_process_init(Job *job, char **argv, bool enable_stdin,
+    bool enable_stdout, bool enable_stderr)


These args could be vertically aligned to help readability. I usually end up doing that if they don't all fit on one line.

Job autocommands will no longer buffer data chunks that don't end in newlines characters.

Send sigterm immediately since it can be caught by processes. If they don't respond and are still alive after a while, SIGKILL will be sent.

- process spawning was decoupled from the rest of the job control logic. The goal is reusing it for spawning processes connected to pseudo terminal file descriptors. - job_start now receives a JobOptions structure containing all the startup options.

ghost · 2015-02-24T06:07:04Z

My build is failing with this while linking tty-test:

[224/224] Linking C executable bin/tty-test
FAILED: : && /usr/lib/hardening-wrapper/bin/cc  -Wconversion -g  -Wl,--no-undefined test/functional/job/CMakeFiles/tty-test.dir/tty-test.c.o  -o bin/tty-test  -rdynamic ../.deps/usr/lib/libuv.a -ldl -lnsl -lrt && :
/usr/bin/ld: ../.deps/usr/lib/libuv.a(libuv_la-thread.o): undefined reference to symbol 'pthread_rwlock_trywrlock@@GLIBC_2.2.5'
/usr/lib/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
[224/224] Linking C executable bin/nvim
ninja: build stopped: subcommand failed.
Makefile:52: recipe for target 'nvim' failed
make: *** [nvim] Error 1

I get the error with or without hardening-wrapper, and this is with a clean build. It might have something to do with the fact that I'm on Arch Linux, which uses glibc 2.21

tarruda · 2015-02-24T10:17:42Z

@pyrohh strange that you would get this error now, all I did was add a custom executable target for testing the pty integration. What happens if you apply this patch:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index caf345d..19ed0e7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -276,7 +276,7 @@ get_compile_flags(NVIM_VERSION_CFLAGS)

 add_subdirectory(test/includes)
 add_subdirectory(config)
-add_subdirectory(test/functional/job) # compile pty test program
+# add_subdirectory(test/functional/job) # compile pty test program


 # Setup some test-related bits.  We do this after going down the tree because we

fwalch · 2015-02-24T13:11:04Z

Similar error happens in the Ubuntu PPA build:

make[5]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
[100%] Built target translations
make[5]: Entering directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
Scanning dependencies of target tty-test
make[5]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
make[5]: Entering directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
[100%] Building C object test/functional/job/CMakeFiles/tty-test.dir/tty-test.c.o
Linking C executable ../../../bin/tty-test
/usr/bin/ld: ../../../../.deps/usr/lib/libuv.a(libuv_la-thread.o): undefined reference to symbol 'pthread_create@@GLIBC_2.1'
/lib/i386-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status
test/functional/job/CMakeFiles/tty-test.dir/build.make:86: recipe for target 'bin/tty-test' failed
make[5]: *** [bin/tty-test] Error 1
make[5]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
CMakeFiles/Makefile2:2807: recipe for target 'test/functional/job/CMakeFiles/tty-test.dir/all' failed
make[4]: *** [test/functional/job/CMakeFiles/tty-test.dir/all] Error 2
make[4]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
Makefile:117: recipe for target 'all' failed
make[3]: *** [all] Error 2
make[3]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1/build'
Makefile:52: recipe for target 'nvim' failed
make[2]: *** [nvim] Error 2
make[2]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1'
dh_auto_build: make -j1 returned exit code 2
debian/rules:4: recipe for target 'override_dh_auto_build' failed
make[1]: *** [override_dh_auto_build] Error 2
make[1]: Leaving directory '/build/buildd/neovim-0.0.0+git201502240125+6~ubuntu15.04.1'
debian/rules:11: recipe for target 'build-arch' failed
make: *** [build-arch] Error 2
dpkg-buildpackage: error: debian/rules build-arch gave error exit status 2

tarruda · 2015-02-24T13:23:38Z

undefined reference to symbol 'pthread_create

This error usually happens when linking against libuv without also linking to pthread, which is normally handled by the pkgconfig(or else even the travis build would fail). I have no clue why this error was introduced in the AUR/PPA builds. cc @jszakmeister

ajpaulson · 2015-02-24T14:24:26Z

Hi - I'm getting the same issue when doing a build (even a fresh build) on Debian Jessie.

According to the output, pthreads is found:

-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE

I can't find confirmation that it is even attempting to link aside from that.

Linking C executable ../../../bin/tty-test
/usr/bin/ld: ../../../../.deps/usr/lib/libuv.a(libuv_la-thread.o): undefined reference to symbol           'pthread_rwlock_trywrlock@@GLIBC_2.2.5'
//lib/x86_64-linux-gnu/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

fwalch · 2015-02-24T16:19:06Z

The PPA build for 12.04 worked, by the way.. guess that explains why Travis doesn't complain.

tarruda · 2015-02-24T16:29:51Z

@fwalch is the build error present before this PR? I ask because I can't see what I could have changed to affect libuv linking

fwalch · 2015-02-24T17:13:43Z

I can't seem to find the revision used to build the packages in the logs :-/ The last successful build was at 2015-02-22 19:55, the one after that is the failing one (there's only one build per day).

There are two different errors for x86 and x64:

x86:

libuv.a(libuv_la-thread.o): undefined reference to symbol 'pthread_create@@GLIBC_2.1'

x64:

libuv.a(libuv_la-thread.o): undefined reference to symbol 'pthread_rwlock_trywrlock@@GLIBC_2.2.5'

Let's maybe move the discussion to #2052? This PR is already closed..

ajpaulson · 2015-02-24T18:29:28Z

After running git bisect and double checking manually - the first build breaking commit is:

0b8d3cb

Which breaks with a different error and is fixed in the following commit.

The first commit that breaks with this libuv undefined reference to symbol... error is:

d7e560e

Which is a refactoring commit.

EDIT
After a little searching -
test/functional/job/CMakeLists.txt
has target_link_libraries(tty-test ${LIBUV_LIBRARIES})
I'm wondering if it might need an explicit linkage for pthread?

jszakmeister · 2015-02-24T19:06:11Z

@tarruda See #2053 and #2054.

ajpaulson · 2015-02-25T12:16:21Z

Confirmed fixed on Debian 7

justinmk · 2015-03-04T04:47:09Z

src/nvim/os/pty_process.c

+  }
+
+  if (job->opts.stderr_cb) {
+    uv_pipe_open(&ptyproc->proc_stderr, dup(master));


coverity thinks these 3 dup() calls are leaks.

I forgot to close the original fd. It has been fixed in #2076: https://github.com/tarruda/neovim/blob/builtin-terminal-emulation/src/nvim/os/pty_process.c#L108-L122

Coverity complains about not closing the (three additional) handles created by dup(master). Closing master won't close these handles.

They are closed by libuv when the stream is closed: https://github.com/libuv/libuv/blob/v1.x/src/unix/stream.c#L1563-L1568

tarruda force-pushed the job-pty branch from 0bd602b to af8ea00 Compare February 21, 2015 19:21

ghost reviewed Feb 21, 2015
View reviewed changes

tarruda force-pushed the job-pty branch 11 times, most recently from 4cc4737 to c3fbf48 Compare February 23, 2015 13:11

tarruda mentioned this pull request Feb 23, 2015

[WIP] doc: job control: mention buffering considerations #1593

Closed

2 tasks

tarruda force-pushed the job-pty branch from c3fbf48 to dd440d3 Compare February 23, 2015 13:42

tarruda changed the title ~~[WIP] Extend job control to support spawning processes via ptys~~ [WIP] Extend job control to support spawning interactive programs Feb 23, 2015

tarruda force-pushed the job-pty branch 6 times, most recently from 4606585 to 67f1d1f Compare February 23, 2015 17:02

tarruda changed the title ~~[WIP] Extend job control to support spawning interactive programs~~ [WIP] Job control fixes and enhancements Feb 23, 2015

tarruda force-pushed the job-pty branch 5 times, most recently from f38eaba to be25111 Compare February 24, 2015 00:18

eval: Fix buffering of data in job autocommands

d28011e

Job autocommands will no longer buffer data chunks that don't end in newlines characters.

tarruda added 3 commits February 23, 2015 21:43

job: Send SIGTERM before calling job_stop in job_teardown

0b8d3cb

Send sigterm immediately since it can be caught by processes. If they don't respond and are still alive after a while, SIGKILL will be sent.

job: Allow spawning jobs connected to pseudo terminals

d7e560e

tarruda force-pushed the job-pty branch from be25111 to d7e560e Compare February 24, 2015 00:48

tarruda changed the title ~~[WIP] Job control fixes and enhancements~~ [RDY] Job control fixes and pseudo terminal support Feb 24, 2015

tarruda merged commit d7e560e into neovim:master Feb 24, 2015

tarruda added a commit that referenced this pull request Feb 24, 2015

Merge PR #2037 'Job control fixes and pseudo terminal support'

56f371c

fwalch mentioned this pull request Feb 24, 2015

Ubuntu 14.10 Build Fails #2052

Closed

ghost mentioned this pull request Feb 25, 2015

":w :term ..." (WAS: "w !sudo ..." does not work) #1716

Closed

bfredl mentioned this pull request Feb 28, 2015

[RFC] job control: allow not sending final newline in jobsend #2073

Merged

justinmk reviewed Mar 4, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] Job control fixes and pseudo terminal support #2037

[RDY] Job control fixes and pseudo terminal support #2037

tarruda commented Feb 21, 2015

ghost Feb 21, 2015

ghost commented Feb 24, 2015

tarruda commented Feb 24, 2015

fwalch commented Feb 24, 2015

tarruda commented Feb 24, 2015

ajpaulson commented Feb 24, 2015

fwalch commented Feb 24, 2015

tarruda commented Feb 24, 2015

fwalch commented Feb 24, 2015

ajpaulson commented Feb 24, 2015

jszakmeister commented Feb 24, 2015

ajpaulson commented Feb 25, 2015

justinmk Mar 4, 2015

tarruda Mar 5, 2015

oni-link Mar 5, 2015

tarruda Mar 5, 2015

[RDY] Job control fixes and pseudo terminal support #2037

[RDY] Job control fixes and pseudo terminal support #2037

Conversation

tarruda commented Feb 21, 2015

ghost Feb 21, 2015

Choose a reason for hiding this comment

ghost commented Feb 24, 2015

tarruda commented Feb 24, 2015

fwalch commented Feb 24, 2015

tarruda commented Feb 24, 2015

ajpaulson commented Feb 24, 2015

fwalch commented Feb 24, 2015

tarruda commented Feb 24, 2015

fwalch commented Feb 24, 2015

ajpaulson commented Feb 24, 2015

jszakmeister commented Feb 24, 2015

ajpaulson commented Feb 25, 2015

justinmk Mar 4, 2015

Choose a reason for hiding this comment

tarruda Mar 5, 2015

Choose a reason for hiding this comment

oni-link Mar 5, 2015

Choose a reason for hiding this comment

tarruda Mar 5, 2015

Choose a reason for hiding this comment