Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove dependencies on curses/terminfo/termcap C library #1431

Merged
merged 5 commits into from Nov 26, 2017

Conversation

Projects
None yet
6 participants
@xavierleroy
Copy link
Contributor

xavierleroy commented Oct 16, 2017

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.

xavierleroy added some commits Oct 15, 2017

Remove dependencies on curses/terminfo/termcap C library
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.
Remove dependencies on curses/terminfo/termcap C library, continued
Bootstrap, then removal of old primitives from byterun/terminfo.c, then bootstrap again.

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

@@ -152,7 +151,7 @@ while : ; do
-lib*)
cclibs="$2 $cclibs"; shift;;
-no-curses|--no-curses)
with_curses=no;;
;; # Ignored for backward compatibility

This comment has been minimized.

@dbuenzli

dbuenzli Oct 16, 2017

Contributor

A warning that this has no effet could be echoed.

This comment has been minimized.

@xavierleroy

xavierleroy Oct 16, 2017

Author Contributor

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

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

This comment has been minimized.

@dbuenzli

dbuenzli Oct 16, 2017

Contributor

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

This comment has been minimized.

@xavierleroy

xavierleroy Oct 16, 2017

Author Contributor

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.

This comment has been minimized.

@dbuenzli

dbuenzli Oct 16, 2017

Contributor

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

This comment has been minimized.

@xavierleroy

xavierleroy Oct 17, 2017

Author Contributor

OK for stdout. See commit 88c2987.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Oct 16, 2017

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented Oct 16, 2017

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

@avsm

This comment has been minimized.

Copy link
Member

avsm commented Oct 16, 2017

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

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Oct 16, 2017

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

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Oct 16, 2017

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

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Oct 16, 2017

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

This comment has been minimized.

Copy link
Contributor

shindere commented Oct 16, 2017

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Oct 16, 2017

GPR#1431: Remove dependencies on curses/terminfo/termcap C library, c…
…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

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Oct 17, 2017

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.


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

This comment has been minimized.

@dra27

dra27 Oct 20, 2017

Contributor

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.

This comment has been minimized.

@xavierleroy

xavierleroy Oct 21, 2017

Author Contributor

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

external resume : int -> unit = "caml_terminfo_resume";;
| Good_term

val setup : unit -> status

This comment has been minimized.

@dra27

dra27 Oct 20, 2017

Contributor

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?

This comment has been minimized.

@xavierleroy

xavierleroy Oct 21, 2017

Author Contributor

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.

GPR#1431: Remove dependencies on curses/terminfo/termcap C library, c…
…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

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented Nov 18, 2017

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.

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

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Nov 24, 2017

You may want to update dependencies.

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

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@xavierleroy xavierleroy deleted the xavierleroy:no-curses branch Nov 26, 2017

hannesm added a commit to hannesm/ocaml-freestanding that referenced this pull request Jun 20, 2018

OCaml 4.07 support
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)

@hannesm hannesm referenced this pull request Jun 20, 2018

Merged

OCaml 4.07 support #39

hannesm added a commit to hannesm/ocaml-freestanding that referenced this pull request Jul 14, 2018

OCaml 4.07 support
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.