-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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.
@@ -152,7 +151,7 @@ while : ; do | |||
-lib*) | |||
cclibs="$2 $cclibs"; shift;; | |||
-no-curses|--no-curses) | |||
with_curses=no;; | |||
;; # Ignored for backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A warning that this has no effet could be echo
ed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just followed the pattern of the -with-pthread*
ignored option.
utils/terminfo.ml
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the setup
analysis was done on stdout
as per the given argument. You now do this on stderr
is that change intended ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for stdout. See commit 88c2987.
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?
|
Nice! IIUC, this does not account for window resizing, but one could use the |
Just a thank you note for this PR, since it removes yet more C stubs from the embedded unikernel builds of MirageOS :-) |
I'm pretty sure the Added 2017-10-17: commit 88c2987 does just what I outlined above. |
These variables are defined by shells but not exported to processes.
|
I'll submit a Mantis bug report, eventually. In a nutshell: executables generated by |
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.
|
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.
Just use it, everything should be fine, for several reasons:
If you tried the original pull request and it didn't work as expected, this is probably due to a bug in my |
byterun/terminfo.c
Outdated
|
||
CAMLexport value caml_terminfo_resume (value lines) | ||
CAMLprim value caml_terminfo_rows(value vchan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion. We can do this now or later.
utils/terminfo.mli
Outdated
external resume : int -> unit = "caml_terminfo_resume";; | ||
| Good_term | ||
|
||
val setup : unit -> status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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 <[email protected]> Co-authored-by: Christine Rose <[email protected]> Co-authored-by: sabine <[email protected]> Co-authored-by: Sabine Schmaltz <[email protected]> Co-authored-by: Dmitrii Kosarev <[email protected]> Co-authored-by: Kakadu <[email protected]> Co-authored-by: tuohy <[email protected]>
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
andtputs
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
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 theioctl
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.