Remove dependencies on curses/terminfo/termcap C library by xavierleroy · Pull Request #1431 · ocaml/ocaml

xavierleroy · 2017-10-16T14:38:11Z

The toplevel REPL uses terminal escape sequences to display error messages nicely, underlining the location of the error when possible. Currently, this hack with the terminal goes through the tgetent, tgetstr, tgetnum and tputs functions provided by one of the curses, terminfo or termcap C libraries. The standard bytecode runtime system provides OCaml bindings to these functions, and therefore depends on the curses/terminfo/termcap C library used.

This pull request breaks this dependency by

Using ANSI escape sequences instead of querying a termcap/terminfo database. The days of pre-ANSI terminals is gone; now all consoles of interest implement ANSI escape sequences. Besides, for colored error messages, the OCaml compilers already use ANSI escape sequences.
Using an ioctl to query the number of lines on the screen. No extra C library is needed for this. A reasonable default of 24 is used if the ioctl fails or is not available (Win32).

This PR requires a bootstrap (because some primitives are removed). Actually two bootstraps are needed because the boostrapping procedure is somewhat broken currently. The first commit in this PR adds the new primitive but keeps the old ones. The second commit is the result of the double bootstrap with removal of the old primitives in between. I am willing to do the merge and redo the bootstrap "live" when this PR is accepted.

This library was used for displaying error messages from the toplevel. Instead, just use ANSI escape sequences to display, and a ioctl() to get the number of lines of the terminal. This commit is an intermediate state before bootstrap.

Bootstrap, then removal of old primitives from byterun/terminfo.c, then bootstrap again.

dbuenzli · 2017-10-16T14:45:05Z

configure

        cclibs="$2 $cclibs"; shift;;
    -no-curses|--no-curses)
-        with_curses=no;;
+        ;;  # Ignored for backward compatibility


A warning that this has no effet could be echoed.

I just followed the pattern of the -with-pthread* ignored option.

dbuenzli · 2017-10-16T14:54:47Z

utils/terminfo.ml

+  let term = try Sys.getenv "TERM" with Not_found -> "" in
+  (* Same heuristics as in Misc.Color.should_enable_color *)
+  if term <> "" && term <> "dumb" && isatty stderr then begin
+    let rows = terminfo_rows stderr in


Previously the setup analysis was done on stdout as per the given argument. You now do this on stderr is that change intended ?

The change was for consistency with Misc.Color.should_enable_color, which uses stderr.

However, a point could be made that the latter is oriented towards compiler error messages (on stderr) while the present code is targeted at the toplevel REPL error messages (on stdout, if I'm not mistaken).

I don't have a strong feeling either way.

I'd say it's better to invoke isatty and terminfo_rows on the fd that you are actually going to use for output and in this case it seems to be stdout:

> ocaml -noinit 2> /dev/null OCaml version 4.03.0 # let f x = let;; Error: Syntax error

OK for stdout. See commit 88c2987.

shindere · 2017-10-16T14:57:27Z

What a nice idea! Thanks! What exactly is broken in the boostrap procedure? Regarding the default values when the ioctl fails, how about also taking into account environment variables such as LINES and COLUMNS before falling back to hard-coded defaults?

nojb · 2017-10-16T15:12:04Z

Nice! IIUC, this does not account for window resizing, but one could use the SIGWINCH signal for that.

avsm · 2017-10-16T16:13:58Z

Just a thank you note for this PR, since it removes yet more C stubs from the embedded unikernel builds of MirageOS :-)

xavierleroy · 2017-10-16T17:30:22Z

this does not account for window resizing

I'm pretty sure the ioctl does take window resizing into account. So, it would be enough to stop cacheing the result of Terminfo.setup in Location.highlight_locations. Then the ioctl would run every time an error message needs highlighting.

Added 2017-10-17: commit 88c2987 does just what I outlined above.

xavierleroy · 2017-10-16T17:31:56Z

Regarding the default values when the ioctl fails, how about also taking into account environment variables such as LINES and COLUMNS before falling back to hard-coded defaults?

These variables are defined by shells but not exported to processes.

$ echo $LINES
24
$ echo 'Sys.getenv "LINES";;' | ocaml
        OCaml version 4.06.0+beta1

# Exception: Not_found.

xavierleroy · 2017-10-16T17:36:12Z

What exactly is broken in the boostrap procedure?

I'll submit a Mantis bug report, eventually. In a nutshell: executables generated by ./ocamlc or by boot/ocamlc -use-prims byterun/primitives (as opposed to plain boot/ocamlc) should be executed by byterun/ocamlrun and not by boot/ocamlrun, because they may use primitives that are provided by the former but not by the latter.

shindere · 2017-10-16T17:54:18Z

Xavier Leroy (2017/10/16 10:26 -0700):

The change was for consistency with `Misc.Color.should_enable_color`, which uses `stderr`. However, a point could be made that the latter is oriented towards compiler error messages (on stderr) while the present code is targeted at the toplevel REPL error messages (on stdout, if I'm not mistaken). I don't have a strong feeling either way.

Would it make sense to make the output configurable somehow? Then, no matter the default, it would not hurt at all.

shindere · 2017-10-16T17:57:14Z

Xavier Leroy (2017/10/16 17:32 +0000):

> Regarding the default values when the ioctl fails, how about also taking into account environment variables such as LINES and COLUMNS before falling back to hard-coded defaults? These variables are defined by shells but not exported to processes.

OK. I guess my concern is that I'd like to have a way to be able to use my good old 80x25 terminal.

…ontinued - byterun/terminfo.c: it's ws_row we are interested in, not ws_col! - Terminfo.setup: test stdout instead of stderr - Add a Terminfo.num_lines function and simplify interface to Terminfo.setup - Query num_lines every time an error message needs to be highlighted, so as to react to windows resizing.

xavierleroy · 2017-10-17T14:27:49Z

I guess my concern is that I'd like to have a way to be able to use my good old 80x25 terminal.

Just use it, everything should be fine, for several reasons:

The number of rows N (that is queried with ioctl and defaults to 24) can be less than the actual number of rows. N is used to give up on the fancy underlining if the area to highlight is more than N-2 lines long, meaning that some of it may have scrolled off the top of the screen and reprinting it highlighted would mess the display. So, if you have 25 rows and OCaml thinks you have 24, the heuristic above will be a tad conservative, but you won't notice.
There is a very good chance that ioctl returns the correct number of columns and rows for your device. I wouldn't be surprised if the shell derives its LINES and COLUMNS variables from ioctl data. So, if the former have the correct values, the latter should also have them.
Once more, the LINES variable is for shell script use only: the 4 full-screen applications I tried (Emacs, Vim, top and mutt) just ignore it and determine screen size by other means.

If you tried the original pull request and it didn't work as expected, this is probably due to a bug in my ioctl code that confused rows and columns... Today's version is better.

dra27 · 2017-10-20T09:13:49Z

byterun/terminfo.c

+#endif

-CAMLexport value caml_terminfo_resume (value lines)
+CAMLprim value caml_terminfo_rows(value vchan)


Given that terminfo.c is now reduced to a single Unix-only function, might unix.c be a better place? I say this because there is a Windows implementation of this (for Windows 10+) which I'm happy (and intending, if @nojb doesn't get there first!) to work on, so if win32.c gains a stub version of caml_terminfo_resume for now, then it would be updated later.

That's a good suggestion. We can do this now or later.

dra27 · 2017-10-20T09:19:17Z

utils/terminfo.mli

-external resume : int -> unit = "caml_terminfo_resume";;
+  | Good_term
+
+val setup : unit -> status


The old mechanism of caching the channel in C was a bit ugly, but it feels similarly wrong to be hardcoding this to stdout. Given such a "big" change, couldn't this either become a functor or have type status converted to a "terminal" or something which gets passed around to the other functions just so that the possibility exists to reuse this for stderr in future?

Of course the functions from the Terminfo module could (should?) take an out_channel so that they are not specific to stdout. However, stdout is hardcoded one level up, in Location.highlight_terminfo, which takes a ppf: Format.formatter as parameter, but has no way to extract the underlying out_channel (if any). Hence it assumes that ppf = Format.stdout and works from there.

…ontinued - Remove byterun/terminfo.c and move the functionality to io.c with OS-dependent code in unix.c and win32.c - utils/terminfo.mli: take out_channel as argument instead of always using stdout

xavierleroy · 2017-11-18T16:56:54Z

I just pushed some simplifications and cleanups following @dra27 suggestions.

Travis seems broken but this branch passed CI "precheck" at Inria.

I am now requesting a formal yes/no review.

shindere · 2017-11-24T16:28:44Z

You may want to update dependencies.

Relevant changes in 4.07 - unix.c now #include <sys/ioctl.h> (since 852b595ff3), but an empty one is fine (only use is in caml_nun_rows_fd in an #ifdef TIOCGWINSZ), see ocaml/ocaml#1431 -> provide an empty ioctl via nolibc - bigarray is now part of the stdlib ocaml/ocaml#1685 -> libotherlibs.a is no longer needed (neither built nor linked)

Relevant changes in 4.07 - unix.c now #include <sys/ioctl.h> (since 852b595ff3), but an empty one is fine (only use is in caml_nun_rows_fd in an #ifdef TIOCGWINSZ), see ocaml/ocaml#1431 -> provide an empty ioctl via nolibc - bigarray is now part of the stdlib ocaml/ocaml#1685 -> libotherlibs.a is no longer needed (neither built nor linked) To support these changes, an empty sys/ioctl.h is added to nolibc. Also, `ocaml-freestanding.pc.in` now contains in `Libs`: -L${libdir} -lasmrum -lnolibc -lopenlibm (instead of ${libdir}/lib for each library). `configure.sh` adds -lotherlibs to these flags in the case that OCAML_GTE_4_07_0 is false. The `Makefile` treats libotherlibs.a as dependency only if OCAML_GTE_4_07_0 is false.

"Your First Hour with OCaml" is replaced by "Tour of OCaml" and "How to Write an OCaml Program". This splits the language part of the tutorial from the tooling part of the tutorial to allow people to focus on one at a time. "Get Up and Running" is replaced by "Installing OCaml", "Configuring your Editor", and "Introduction to opam Switches". To help people troubleshoot a homebrew issue on Apple M1 machines that frequently occurs when installing OCaml, a document providing a fix has been added as well. --------- Co-authored-by: Cuihtlauac ALVARADO <cuihtmlauac@tarides.com> Co-authored-by: Christine Rose <christinerose@users.noreply.github.com> Co-authored-by: sabine <sabine@users.noreply.github.com> Co-authored-by: Sabine Schmaltz <sabineschmaltz@gmail.com> Co-authored-by: Dmitrii Kosarev <Kakadu@pm.me> Co-authored-by: Kakadu <Kakadu@users.noreply.github.com> Co-authored-by: tuohy <g.tuohy@outlook.com>

xavierleroy added 2 commits October 15, 2017 10:56

Remove dependencies on curses/terminfo/termcap C library, continued

8a8ba16

Bootstrap, then removal of old primitives from byterun/terminfo.c, then bootstrap again.

xavierleroy added the after-next-release label Oct 16, 2017

xavierleroy added this to the 4.07-or-later milestone Oct 16, 2017

xavierleroy removed the after-next-release label Oct 16, 2017

dbuenzli reviewed Oct 16, 2017

View reviewed changes

dra27 reviewed Oct 20, 2017

View reviewed changes

GPR#1431: Remove dependencies on curses/terminfo/termcap C library, c…

8c4b3ed

…ontinued - Remove byterun/terminfo.c and move the functionality to io.c with OS-dependent code in unix.c and win32.c - utils/terminfo.mli: take out_channel as argument instead of always using stdout

xavierleroy added the review-needed label Nov 18, 2017

xavierleroy modified the milestones: 4.07-or-later, 4.07 Nov 18, 2017

shindere approved these changes Nov 24, 2017

View reviewed changes

Remove dependencies for terminfo, which was removed

9b1cca3

xavierleroy merged commit 852b595 into ocaml:trunk Nov 26, 2017

xavierleroy deleted the no-curses branch November 26, 2017 14:49

dra27 mentioned this pull request Dec 2, 2017

Small corrections to configure. #1507

Merged

ejgallego mentioned this pull request Dec 5, 2017

[default.nix] needs ncurses for the test-suite rocq-prover/rocq#6309

Merged

hannesm mentioned this pull request Jun 20, 2018

OCaml 4.07 support mirage/ocaml-solo5#39

Merged

anmonteiro mentioned this pull request Nov 4, 2018

Add support for OCaml 4.07 jaredly/reason-language-server#167

Merged

This was referenced Mar 14, 2019

ocamlc -custom should not link to curses #6735

Closed

missing -ltinfo when linking bytecode with -custom #7164

Closed

bobot mentioned this pull request May 24, 2019

Compiling custom bytecode timbertson/opam2nix-packages#49

Closed

Conversation

xavierleroy commented Oct 16, 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!

dbuenzli Oct 16, 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!

shindere commented Oct 16, 2017 via email

Uh oh!

nojb commented Oct 16, 2017

Uh oh!

avsm commented Oct 16, 2017

Uh oh!

xavierleroy commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xavierleroy commented Oct 16, 2017

Uh oh!

xavierleroy commented Oct 16, 2017

Uh oh!

shindere commented Oct 16, 2017 via email

Uh oh!

shindere commented Oct 16, 2017 via email

Uh oh!

xavierleroy commented Oct 17, 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!

xavierleroy commented Nov 18, 2017

Uh oh!

shindere commented Nov 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dbuenzli Oct 16, 2017 •

edited

Loading

xavierleroy commented Oct 16, 2017 •

edited

Loading